529750 - Move GetImportantRule to nsICSSStyleRule and make it nonvirtual

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 523148 comment 2.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Need to wait for bug 522595 to land before working on this.

Comment 2

[Boris Zbarsky [:bzbarsky]]

One thing to keep track of, from bug 5231348:

  There are some UA-level rules that are not in fact nsICSSStyleRule (e.g. the
  restriction rules for first-letter and first-line).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Don't walk non-CSS UA rules when checking for important UA rules.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Assume that in AssertNoImportantRules and AddImportantRules the rule is a StyleRule.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch nsRuleWalker to only calling GetImportantRule on CSS StyleRules.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Move GetImportantRule to css::StyleRule.

Comment 7

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

Comment on attachment 530778 [details] [diff] [review]
part 1.  Don't walk non-CSS UA rules when checking for important UA rules.

You should AssertNoCSSRules between lastRestrictionRN and mRuleTree.

r=dbaron with that

Comment 8

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

Comment on attachment 530779 [details] [diff] [review]
part 2.  Assume that in AssertNoImportantRules and AddImportantRules the rule is a StyleRule.

>+#ifdef DEBUG
>+    nsRefPtr<css::StyleRule> cssRule(do_QueryObject(node->GetRule()));
>+    NS_ASSERTION(cssRule, "Unexpected non-CSS rule");
>+#endif

If we haven't broken the ability to do it, I'd prefer (no ifdefs):

+    NS_ASSERTION(nsRefPtr<css::StyleRule>(do_QueryObject(node->GetRule())),
+                 "Unexpected non-CSS rule");

r=dbaron with that

Comment 9

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

Comment on attachment 530780 [details] [diff] [review]
part 3.  Switch nsRuleWalker to only calling GetImportantRule on CSS StyleRules.

Instead of having one overload of forward call the other, could you
have both call a DoForward() method, and then have
Forward(nsIStyleRule*) assert that its argument is not a
css::StyleRule (to prevent the same mistake you had to fix earlier
in the patch)?

r=dbaron with that

Comment 10

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

Comment on attachment 530781 [details] [diff] [review]
part 4.  Move GetImportantRule to css::StyleRule.

r=dbaron

Comment 11

[Boris Zbarsky [:bzbarsky]]

> You should AssertNoCSSRules between lastRestrictionRN and mRuleTree.

Done.

>+    NS_ASSERTION(nsRefPtr<css::StyleRule>(do_QueryObject(node->GetRule())),
>+                 "Unexpected non-CSS rule");

Done.

> could you have both call a DoForward() method, and then have
> Forward(nsIStyleRule*) assert

Done.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So adding that assert made tests fail because ResolveStyleByAddingRules() passes in a nsCOMArray<nsIStyleRule> &aRules.

And StyleWithDeclarationAdded calls ResolveStyleByAddingRules and passes in an array containing a single StyleRule.  Other callers pass non-style rules here.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I disabled that assert for now.  Please let me know if you'd prefer some other solution.

Comment 14

[Boris Zbarsky [:bzbarsky]]

David, see comment 12 and comment 13.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed to cedar:

http://hg.mozilla.org/projects/cedar/rev/957089411310
http://hg.mozilla.org/projects/cedar/rev/05e6b77b99c4
http://hg.mozilla.org/projects/cedar/rev/a5cfc3f56f62
http://hg.mozilla.org/projects/cedar/rev/99cff4d7f934

and then fixed test orange per comment 12 and comment 13: http://hg.mozilla.org/projects/cedar/rev/e0580e4f3825

Comment 16

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

The other solution would be to add a third Forward() method (ForwardOnPossiblyCSSRule) that doesn't have the assert.  I'd sort of prefer that...

Comment 17

[Boris Zbarsky [:bzbarsky]]

I can do that.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: followup; reenable the assertion.

Comment 19

[Matt Brubeck (:mbrubeck)]

Merged all parts from Cedar to mozilla-central:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=e0580e4f3825

Comment 20

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

Comment on attachment 534669 [details] [diff] [review]
followup; reenable the assertion.

>+  void ForwardOnPossiblyCSSRule(nsIStyleRule* aRule) {
>+    DoForward(aRule);
>+  }

Add a comment that callers should only use this when they have a list of rules that already has the important rules in it?

r=dbaron with that

Comment 21

[Boris Zbarsky [:bzbarsky]]

Done, and pushed http://hg.mozilla.org/projects/cedar/rev/db398bf9cffe with that.

Comment 22

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/db398bf9cffe