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

List:       kfm-devel
Subject:    Re: [PATCH] CSSMediaRule/CSSRuleList implementation
From:       Lars Knoll <knoll () kde ! org>
Date:       2001-09-26 8:53:51
[Download RAW message or body]

On Wednesday 26 September 2001 10:34, Martijn Klingens wrote:
> 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?

No. Adding and removing references have to happen at the points where you 
store a pointer to the object. Everything else would make the code completely 
non understandable and very hard to debug at some point.

> 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.

Looks ok.

Lars

>
> Martijn

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