980560 - nsDocument::StyleRuleChanged etc. should not QI to css::Rule

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Cameron McCormack (:heycam)]

NeilAway pointed out that in nsDocument::StyleRuleChanged etc., we're attempting to QI from nsIStyleRule to css::Rule, and css::Rule doesn't have an IID, so we're in fact unsafely casting to css::Rule.

We only ever pass in null or a css::StyleRule into these methods.  Should I static_cast to it (or just css::Rule) or change the prototypes to take css::StyleRules instead?

Comment 1

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

Hmmm.  In https://hg.mozilla.org/mozilla-central/rev/1a34a6a07d71 I changed it so we do pass a non-style-rule (a keyframe rule), though we could probably change to passing null without hurting anything.

I think it would be best for the methods to take css::Rule, actually.  (I'd like to make nsIStyleRule be the "source of style data" concept, which might mean making StyleRule not be the nsIStyleRule implementation; see bug 978833, and also my comments in bug 898764.)

And given the change I made, we should probably get this in the same release as bug 978648, or fix that patch accordingly.

Comment 2

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #1)
> Hmmm.  In https://hg.mozilla.org/mozilla-central/rev/1a34a6a07d71 I changed
> it so we do pass a non-style-rule (a keyframe rule), though we could
> probably change to passing null without hurting anything.

Oh right.  (Was looking through a few days old checkout.)

> I think it would be best for the methods to take css::Rule, actually.

What is the correct way to check-and-cast from css::Rule to css::StyleRule then?

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> What is the correct way to check-and-cast from css::Rule to css::StyleRule
> then?

Checking |GetType() == nsIDOMCSSRule::STYLE_RULE| might be sufficient?

Comment 4

[Cameron McCormack (:heycam)]

Sorry my mistake; we don't even need to check whether the rule is a style rule.  GetDOMRule() can be called on all rule types.

Comment 5

[Cameron McCormack (:heycam)]

I'll make nsDocument::StyleRuleChanged take css::Rule pointers, and call GetDOMRule() regardless of the type of rule that gets passed in.  IRC conversation about this:

 <heycam> dbaron, why is it that keyframes rules (and others) implement nsIDOMStyleRule themselves, and return themselves from GetDOMRule(), while StyleRule returns mDOMRule, which I assume is a different object?
 <heycam> can you explain DOM rules as separate from css::Rules?
 <dbaron> heycam, style rules need to have the separate object at the very least for rule immutability
 <dbaron> heycam, I'm not sure if that's when we added the separate DOM wrapper object, though
 <heycam> dbaron, so the nsIDOMStyleRule stays immutable but the separate DOM rule object can change?
 <dbaron> heycam, since when there are dynamic changes to a declaration, we need a new object implementing nsIStyleRule, but we have to have the same object exposed to the dom
 <heycam> oh I see
 <heycam> the other way around
 <dbaron> heycam, it's become a bit of a mess
 <heycam> :)
 <dbaron> heycam, I think the best way out is to change it so that the implementation of nsIStyleRule lives on nsCSSCompressedDataBlock instead of on StyleRule
 <dbaron> heycam, then we'd probably rename nsIStyleRule to something better
 <heycam> StyleData or something
 <dbaron> heycam, StyleSource sounds nice, except I also want to rename nsIStyleRuleProcessor, and having StyleSource and StyleSourceSource doesn't quite have a ring to it :-)
 <heycam> heh
 <dbaron> yeah, maybe StyleData and StyleDataSource
 <heycam> dbaron, I'm wondering whether the style sheet rule-change notification methods should be fired for all rules, not just for style rules
 <dbaron> heycam, probably not
 <heycam> well not just fired, but have the old/new rule pointers passed through
 <heycam> no?
 <dbaron> heycam, well, maybe
 <dbaron> heycam, we should probably clarify what it means
 <dbaron> heycam, since really different types of rules lead to different types of changes
 <dbaron> heycam, it's also not clear that it still needs to be on the document at all
 <dbaron> heycam, since I think the only thing that cares is the pres shell / frame constructor, and there's now only one presentation per document
 <dbaron> heycam, although maybe it still makes some logical sense
 <heycam> dbaron, devtools uses it for style rule changes at least
 <heycam> and perhaps they would want to know other things
 <dbaron> heycam, it probably does make sense to pass the rule instead of null, where we have the rule, at least
 <dbaron> heycam, and where we're currently calling it
 <dbaron> heycam, there might be rule changes where we're nott currently calling StyleRuleChanged and where we don't need the handling that's currently there, so we probably want to be a little more careful there
 <heycam> dbaron, ok. so how about I just leave the current set of callers, make StyleRuleChanged take css::Rule pointers, and pass along GetDOMRule() whether it's a style rule or not
 <dbaron> heycam, do we always have a dom rule, or would that lazily create something in some cases?
 <heycam> dbaron, I think all of the non-style rules cases just return themselves
 <heycam> *rule
 <heycam> so the lazy creation only happens for StyleRule
 <dbaron> heycam, right
 <dbaron> heycam, do we always have the object in the cases where we call StyleRuleChanged for StyleRule?
 <heycam> dbaron, already? I don't know, but we currently do call GetDOMRule on it so it will be created if it doesn't
 <dbaron> heycam, where do we do that?
 <heycam> but only if StyleSheetChangeEventsEnabled()
 <heycam> in nsDocument::StyleRuleChanged
 <dbaron> heycam, is that for devtools?
 <heycam> yes
 <dbaron> heycam, I wouldn't want to trigger more lazy creation than we already have
 <heycam> they turn that on and off when you open/close the console I think
 <heycam> right, so I don't think this would cause any new creation
 <dbaron> heycam, it might be that in any case where we're sending StyleRuleChanged, we've already created the DOM rule
 <dbaron> heycam, but you should check that first :-)
 <heycam> dbaron, I am not sure about that. but in any case, my change wouldn't change the set of times the lazy creation happens.
 <dbaron> heycam, ok

Comment 6

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

Cameron, were you going to work on this?

Comment 7

[Cameron McCormack (:heycam)]

Yes but I won't get to it for at least 6 weeks.  Feel free to take it if you wish.

Comment 8

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

The patch here should remove the workaround added in
https://bug514280.bugzilla.mozilla.org/attachment.cgi?id=8419249

Comment 9

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

In bug 978833 patch 14 I fixed these methods to take a css::Rule parameter instead of nsIStyleRule.  It also removes the nsDocument.cpp change referenced in comment 8.

Comment 10

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

Fixed by bug 978833.