See bug 523148 comment 2.

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

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 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 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 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 on attachment 530781 [details] [diff] [review] part 4. Move GetImportantRule to css::StyleRule. r=dbaron

> 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

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.

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

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

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