Closed Bug 898764 Opened 6 years ago Closed 3 years ago

Introduce a CSSRule class to implement the WebIDL API


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: Ms2ger, Assigned: Ms2ger)




(6 files, 1 obsolete file)

No description provided.
Attachment #782158 - Flags: review?(cam)
Comment on attachment 782158 [details] [diff] [review]
Part a: Introduce a CSSRule class and implement nsIDOMCSSRule::GetCSSRule;

Making all rule classes inherit from this separately bloats them all by a word.

The existing mozilla::css::Rule class (which should be renamed back to mozilla::CSSRule) is for anything that has the DOM API of a rule.  Can you reuse that instead?
Attachment #782158 - Flags: review-
What's the disadvantage of having StyleRule inherit from it anyway, even though style rule has a separate (and more persistent) DOMCSSStyleRule object?
The way WebIDL bindings work, every DOM interface needs to have a corresponding C++ class.  Furthermore, all objects that implement that interface (which are the things that JS objects have pointers to) need to inherit from that class.  So we need a common base class for DOMCSSStyleRule and the various other rules, basically....
I still don't see why css::Rule can't be that base class.  Is there a requirement that every instance of the class actually be DOM-exposed?
Flags: needinfo?(bzbarsky)
There is not, but for css::Rule to be the base class we'd need to have DOMCSSStyleRule inheriting from css::Rule, no?
Flags: needinfo?(bzbarsky)
And that's a problem because...?
Flags: needinfo?(bzbarsky)
It's not a priori; it just doesn't right now.
Flags: needinfo?(bzbarsky)
Comment on attachment 782158 [details] [diff] [review]
Part a: Introduce a CSSRule class and implement nsIDOMCSSRule::GetCSSRule;

I'll await an updated patch with David's suggested class hierarchy before having a look.
Attachment #782158 - Flags: review?(cam)
Flags: needinfo?(dbaron)
Comment on attachment 782159 [details] [diff] [review]
Part b: Introduce CSSRule::Type();

Cancelling review on the rest of the patches for now.
Attachment #782159 - Flags: review?(cam)
Attachment #782160 - Flags: review?(cam)
Attachment #782161 - Flags: review?(cam)
Attachment #782162 - Flags: review?(cam)
Attachment #782163 - Flags: review?(cam)
(In reply to Boris Zbarsky [:bz] from comment #14)
> It's not a priori; it just doesn't right now.

Hmm, didn't we get to a conclusion here?
We got to the point where I need to go look at the current inheritance setup and then think about how this should all work.
Not currently working on this; happy to take another look when the design is clear.
Assignee: Ms2ger → nobody
Flags: needinfo?(bzbarsky)
OK.  So the main issue I see with using Rule as a base class for DOMCSSStyleRule is that it has two members (mSheet and mParentRule) which DOMCSSStyleRule doesn't have right now.  So we'd be increasing the size of DOMCSSStyleRule by two words and adding a bit of complexity (having to keep those members in sync between DOMCSSStyleRule and StyleRule).  The complexity worries me a lot more than the memory usage, honestly.

The actual methods our base class would need to have are Type() and GetCSSText() (both already virtual on Rule) and GetParentStyleSheet()/GetParentStyleRule(), which are non-virtual on Rule right now because of those members (albeit with different names/signatures).

The other css::Rule methods could just be forwarded to Rule(), of course.

It seems to me to be simplest (because it avoids the parent sheet/rule syncing bits) to introduce a new mozilla::dom::CSSRule abstract class with just those four virtual methods on it which mozilla::css::Rule and DOMCSSStyleRule both inherit from.  But we _could_ make the other work as well...
I just talked to dbaron about this.  Filed bug 1022442 for the long-term thing we want, but for now I think we want to do the following:

1)  Add a pure virtual dom::CSSRule class.
2)  Have the various non-StyleRule rules inherit from dom::CSSRule.  We don't want to do
    this for StyleRule (and in particular don't want Rule inheriting from dom::CSSRule)
    because it would add an extra vtable pointer to StyleRule.
3)  Have DOMCSSStyleRule inherit from dom::CSSRule.

Ms2ger, is that basically what your part (a) above does?  If so, do you have time to pick this up again?
Flags: needinfo?(dbaron)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(Ms2ger)
Attachment #782158 - Attachment is obsolete: true
Assignee: nobody → Ms2ger
Attachment #8462655 - Flags: review?(cam)
Flags: needinfo?(Ms2ger)
We might be able to avoid some of the complexity here post-bug 1221436.

(Not sure what happened with this review request, though.)
I proposed a plan in bug 851892 comment 1 that I think means we don't need this separate class anymore.
Comment on attachment 8462655 [details] [diff] [review]
Part a: Introduce a CSSRule class and implement nsIDOMCSSRule::GetDOMCSSRule

Cancelling review given comment 25.
Attachment #8462655 - Flags: review?(cam)
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.