The default bug view has changed. See this FAQ.

Move GetImportantRule to nsICSSStyleRule and make it nonvirtual

RESOLVED FIXED in mozilla7

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla7
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

See bug 523148 comment 2.
Depends on: 523148
Need to wait for bug 522595 to land before working on this.
Depends on: 522595
Depends on: 529749
Priority: -- → P2

Updated

7 years ago
Depends on: 576831
Priority: P2 → P3
Priority: P3 → P1
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).
Created attachment 530778 [details] [diff] [review]
part 1.  Don't walk non-CSS UA rules when checking for important UA rules.
Attachment #530778 - Flags: review?(dbaron)
Created attachment 530779 [details] [diff] [review]
part 2.  Assume that in AssertNoImportantRules and AddImportantRules the rule is a StyleRule.
Attachment #530779 - Flags: review?(dbaron)
Created attachment 530780 [details] [diff] [review]
part 3.  Switch nsRuleWalker to only calling GetImportantRule on CSS StyleRules.
Attachment #530780 - Flags: review?(dbaron)
Created attachment 530781 [details] [diff] [review]
part 4.  Move GetImportantRule to css::StyleRule.
Attachment #530781 - Flags: review?(dbaron)
Whiteboard: [need review]
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
Attachment #530778 - Flags: review?(dbaron) → review+
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
Attachment #530779 - Flags: review?(dbaron) → review+
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
Attachment #530780 - Flags: review?(dbaron) → review+
Comment on attachment 530781 [details] [diff] [review]
part 4.  Move GetImportantRule to css::StyleRule.

r=dbaron
Attachment #530781 - Flags: review?(dbaron) → review+
> 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.
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.
I disabled that assert for now.  Please let me know if you'd prefer some other solution.
David, see comment 12 and comment 13.
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
Flags: in-testsuite-
Whiteboard: [need review] → fixed-in-cedar
Target Milestone: --- → mozilla6
Target Milestone: mozilla6 → mozilla7
The other solution would be to add a third Forward() method (ForwardOnPossiblyCSSRule) that doesn't have the assert.  I'd sort of prefer that...
I can do that.
Created attachment 534669 [details] [diff] [review]
followup; reenable the assertion.
Attachment #534669 - Flags: review?(dbaron)
Merged all parts from Cedar to mozilla-central:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=e0580e4f3825
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
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
Attachment #534669 - Flags: review?(dbaron) → review+
Done, and pushed http://hg.mozilla.org/projects/cedar/rev/db398bf9cffe with that.
http://hg.mozilla.org/mozilla-central/rev/db398bf9cffe
You need to log in before you can comment on or make changes to this bug.