[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-25 15:37:30
[Download RAW message or body]

On Tuesday 25 September 2001 16:23, Martijn Klingens wrote:
> Could someone please carefully check this patch?
>
> 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.

> Also, another concern is the addition of two methods to MediaListImpl that
> I need internally. What's the consensus regarding this? Should I make them
> private and make CSSMediaRuleImpl a friend class? Or leave them public and
> mark them as internal as I do now? Or even something completely different?

This is no problem at all. *Impl classes are internal. The only classes that 
get exported are the classes in the dom/ subdirectory and some of the classes 
in the main khtml directory. One of the main motivations for the Impl classes 
were that you have the freedom to modify them as you please without breaking 
binary compatibilty.

> The code does compile for me, but I haven't actually tested yet, since the
> MediaList & co. classes are not called anywhere yet. That would be the next
> step if this patch is ok.

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.

> Not related to this patch, but to the CSSMediaRule class: the DOM API sais
> the cssRules and media attributes of CSSMediaRule are readonly. They are
> properly declared const in the definition as well, but the Impl class has
> them as read/write members because I didn't add the const keyword when
> updating the API. Should I add 'const' here, or would that introduce side
> effects?

You can do whatever you want in the Impl classes. Especially, please add 
const qualifiers to all methods that do not modify the class. It helps the 
compiler optimizing.

> Oh, and a final question: I still don't have proper DOM exceptions in place
> and I wonder whether I should change that. As David pointed out they _are_
> used in khtml/dom, but nowhere in khtml/css which makes me wonder if they
> are wanted in the first place...

See my other mail on the subject.

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