[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kfm-devel
Subject:    Re: [PATCH] CSSMediaRule/CSSRuleList implementation
From:       Martijn Klingens <mklingens () ism ! nl>
Date:       2001-09-26 8:34:44
[Download RAW message or body]

On Tuesday 25 September 2001 17:37, Lars Knoll wrote:
> > If I understood the DOM/KHTML API correctly I should never delete
> > objects, but deref() them instead, which I am doing here.
>
> You'll also need to ref() them at construction time. This might be a design
> error I did, but DomShared objects get created with a refcount of 0.

Ah... I see.

> -- snip --
>
> You should take care you do the right thing with refcounting. Inserting
> into the list should add a ref(), removing from the list needs to call
> deref(). Otherwise we will get crashes at some point that are very hard to
> trace back. In this case it looks to me as if insertRule() needs a
> rule->ref() in the first line.

Hmm... actually insertRule() doesn't create objects but instead gets a 
CSSRuleImpl from parseRule(). Doesn't parseRule add a reference for me?

OTOH, the CSSMediaRuleImpl constructor creates two objects manually using 
new, I added calls to ref() there.

A diff against the old diff and a new new diff gives the attached relevant 
changes (two new lines and one moved up). Besides the lacking support for DOM 
exceptions, is this version ok for commit?

For convenience I also attached the full diff, though that is 99% identical 
to the old diff.

Martijn


["css2.diff" (text/x-diff)]

Index: css_ruleimpl.cpp
===================================================================
RCS file: /home/kde/kdelibs/khtml/css/css_ruleimpl.cpp,v
retrieving revision 1.20
diff -u -3 -p -r1.20 css_ruleimpl.cpp
--- css_ruleimpl.cpp	2001/09/25 09:16:24	1.20
+++ css_ruleimpl.cpp	2001/09/26 08:25:19
@@ -185,13 +185,18 @@ CSSMediaRuleImpl::CSSMediaRuleImpl(Style
     : CSSRuleImpl(parent)
 {
     m_type = CSSRule::MEDIA_RULE;
-    m_lstMedia = 0;
-    m_cssRules = 0;
+    m_lstMedia = new MediaListImpl( this );
+    m_lstMedia->ref();
+    m_lstCSSRules = new CSSRuleListImpl();
+    m_lstCSSRules->ref();
 }
 
 CSSMediaRuleImpl::~CSSMediaRuleImpl()
 {
-    if(m_lstMedia) m_lstMedia->deref();
+    if( m_lstMedia )
+        m_lstMedia->deref();
+    if( m_lstCSSRules )
+        m_lstCSSRules->deref();
 }
 
 MediaListImpl *CSSMediaRuleImpl::media() const
@@ -201,18 +206,24 @@ MediaListImpl *CSSMediaRuleImpl::media()
 
 CSSRuleListImpl *CSSMediaRuleImpl::cssRules()
 {
-    return m_cssRules;
+    return m_lstCSSRules;
 }
 
-unsigned long CSSMediaRuleImpl::insertRule( const DOMString &/*rule*/, unsigned long /*index*/ )
+unsigned long CSSMediaRuleImpl::insertRule( const DOMString &rule,
+                                            unsigned long index )
 {
-    // ###
-    return 0;
+    const QChar *curP = rule.unicode();
+    CSSRuleImpl *newRule = parseRule( curP, curP + rule.length() );
+                                   
+    if( newRule )
+        return m_lstCSSRules->insertRule( newRule, index );
+    else
+        return 0;
 }
 
-void CSSMediaRuleImpl::deleteRule( unsigned long /*index*/ )
+void CSSMediaRuleImpl::deleteRule( unsigned long index )
 {
-    // ###
+    m_lstCSSRules->deleteRule( index );
 }
 
 // ---------------------------------------------------------------------------
@@ -333,5 +344,26 @@ unsigned long CSSRuleListImpl::length() 
 CSSRuleImpl *CSSRuleListImpl::item ( unsigned long index )
 {
     return m_lstCSSRules.at( index );
+}
+
+void CSSRuleListImpl::deleteRule ( unsigned long index )
+{
+    CSSRuleImpl *rule = m_lstCSSRules.at( index );
+    if( rule )
+    {
+        m_lstCSSRules.remove( index );
+        rule->deref();
+    }
+}
+
+unsigned long CSSRuleListImpl::insertRule( CSSRuleImpl *rule,
+                                           unsigned long index )
+{
+    if( m_lstCSSRules.insert( index, rule ) )
+        return index;
+    else
+        rule->deref();    // insertion failed
+        
+    return 0;
 }
 
Index: css_ruleimpl.h
===================================================================
RCS file: /home/kde/kdelibs/khtml/css/css_ruleimpl.h,v
retrieving revision 1.10
diff -u -3 -p -r1.10 css_ruleimpl.h
--- css_ruleimpl.h	2001/09/25 09:16:24	1.10
+++ css_ruleimpl.h	2001/09/26 08:25:19
@@ -140,7 +140,7 @@ public:
     virtual bool isMediaRule() { return true; }
 protected:
     MediaListImpl *m_lstMedia;
-	CSSRuleListImpl *m_cssRules;
+	CSSRuleListImpl *m_lstCSSRules;
 };
 
 
@@ -211,6 +211,12 @@ public:
 
     unsigned long length() const;
     CSSRuleImpl *item ( unsigned long index );
+   
+    /**
+     * Internal API, _NOT_ to be exposed outside! (Used by CSSMediaRuleImpl)
+     */
+    unsigned long insertRule ( CSSRuleImpl *rule, unsigned long index );
+    void deleteRule ( unsigned long index );
     
 protected:
     QList<CSSRuleImpl> m_lstCSSRules;

["css.diff.diff" (text/x-diff)]

--- css.diff	Wed Sep 26 10:23:26 2001
+++ css2.diff	Wed Sep 26 10:27:23 2001
@@ -1,22 +1,20 @@
 Index: css_ruleimpl.cpp
 ===================================================================
 RCS file: /home/kde/kdelibs/khtml/css/css_ruleimpl.cpp,v
 retrieving revision 1.20
 diff -u -3 -p -r1.20 css_ruleimpl.cpp
 --- css_ruleimpl.cpp	2001/09/25 09:16:24	1.20
-+++ css_ruleimpl.cpp	2001/09/25 14:05:43
-@@ -185,13 +185,16 @@ CSSMediaRuleImpl::CSSMediaRuleImpl(Style
++++ css_ruleimpl.cpp	2001/09/26 08:25:19
+@@ -185,13 +185,18 @@ CSSMediaRuleImpl::CSSMediaRuleImpl(Style
      : CSSRuleImpl(parent)
  {
      m_type = CSSRule::MEDIA_RULE;
 -    m_lstMedia = 0;
 -    m_cssRules = 0;
 +    m_lstMedia = new MediaListImpl( this );
++    m_lstMedia->ref();
 +    m_lstCSSRules = new CSSRuleListImpl();
++    m_lstCSSRules->ref();
  }
  
  CSSMediaRuleImpl::~CSSMediaRuleImpl()
@@ -71,8 +69,8 @@
 +    CSSRuleImpl *rule = m_lstCSSRules.at( index );
 +    if( rule )
 +    {
-+        rule->deref();
 +        m_lstCSSRules.remove( index );
++        rule->deref();
 +    }
 +}
 +


[prev in list] [next in list] [prev in thread] [next in thread]