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

List:       kfm-devel
Subject:    Re: patch: refcounting fixes
From:       Martijn Klingens <mklingens () ism ! nl>
Date:       2001-09-27 9:24:23
[Download RAW message or body]

On Wednesday 26 September 2001 19:47, Dirk Mueller wrote:
> please have a look.

Ok, I integrated your patch in my local copy, so it will be part of a _BIG_ 
patch to get @media working. Since nobody will use these classes currently 
anywhere in KHTML (there are not even instances of these objects created yet 
IIRC) I won't commit this.

I have the following changes/questions regarding your patch:

> RCS file: /home/kde/kdelibs/khtml/css/css_ruleimpl.cpp,v
> retrieving revision 1.22
> diff -u -3 -r1.22 css_ruleimpl.cpp
> --- css/css_ruleimpl.cpp	2001/09/26 13:02:05	1.22
> +++ css/css_ruleimpl.cpp	2001/09/26 17:46:34
> @@ -196,10 +196,8 @@
> 
>  CSSMediaRuleImpl::~CSSMediaRuleImpl()
>  {
> -    if( m_lstMedia )
> -        m_lstMedia->deref();
> -    if( m_lstCSSRules )
> -        m_lstCSSRules->deref();
> +    m_lstMedia->deref();
> +    m_lstCSSRules->deref();
>  }

I only applied this for m_lstCSSRules, since my local version is changed from 
CVS in such a way that m_lstMedia can be NULL now.

>  unsigned long CSSRuleListImpl::insertRule( CSSRuleImpl *rule,
>                                             unsigned long index )
>  {
> -    if( m_lstCSSRules.insert( index, rule ) )
> +    if( rule && m_lstCSSRules.insert( index, rule ) ) {
> +        rule->ref();
>          return index;
> -    else
> -        rule->deref();    // insertion failed
> -
> +    }
> +
> +    // ### return count()+1 here ?
>      return 0;
>  }

No, not return count() + 1, but throw DOM exception. This is not implemented 
yet, will add a comment in the code.

For the rest I applied your changes as you posted them. Although that 
probably wasn't your goal, they helped me understand the code better, so 
thanks for looking at this!

Martijn

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