898764 - Introduce a CSSRule class to implement the WebIDL API

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Introduce a CSSRule class and implement nsIDOMCSSRule::GetCSSRule;

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Introduce CSSRule::Type();

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Introduce CSSRule::GetDOMCssText();

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part d: Introduce CSSRule::SetDOMCssText();

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part e: Introduce CSSRule::GetParentStyleSheet();

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part f: Introduce CSSRule::GetParentRule();

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

No; see http://logbot.glob.com.au/?c=mozilla%23content&s=15+Jul+2013&e=15+Jul+2013&#c107832

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's the disadvantage of having StyleRule inherit from it anyway, even though style rule has a separate (and more persistent) DOMCSSStyleRule object?

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

There is not, but for css::Rule to be the base class we'd need to have DOMCSSStyleRule inheriting from css::Rule, no?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And that's a problem because...?

Comment 14

[Boris Zbarsky [:bzbarsky]]

It's not a priori; it just doesn't right now.

Comment 15

[Cameron McCormack (:heycam)]

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.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 782159 [details] [diff] [review]
Part b: Introduce CSSRule::Type();

Cancelling review on the rest of the patches for now.

Comment 17

[(no longer active)]

(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?

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Not currently working on this; happy to take another look when the design is clear.

Comment 20

[Boris Zbarsky [: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...

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Introduce a CSSRule class and implement nsIDOMCSSRule::GetDOMCSSRule

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/users/Ms2ger_gmail.com/patches/rev/7d27dbe71a27
https://hg.mozilla.org/users/Ms2ger_gmail.com/patches/rev/8c4b4b3edf8a

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We might be able to avoid some of the complexity here post-bug 1221436.

(Not sure what happened with this review request, though.)

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I proposed a plan in bug 851892 comment 1 that I think means we don't need this separate class anymore.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8462655 [details] [diff] [review]
Part a: Introduce a CSSRule class and implement nsIDOMCSSRule::GetDOMCSSRule

Cancelling review given comment 25.