Last Comment Bug 648925 - Remove nsICSSRule
: Remove nsICSSRule
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla6
Assigned To: Craig Topper
:
Mentors:
Depends on:
Blocks: deCOM
  Show dependency treegraph
 
Reported: 2011-04-10 18:30 PDT by Craig Topper
Modified: 2011-05-07 00:35 PDT (History)
4 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Change uses of nsICSSRule to css::Rule (69.51 KB, patch)
2011-04-10 18:42 PDT, Craig Topper
no flags Details | Diff | Review
Part 2: Make several css::Rule methods non-virtual and inline (2.38 KB, patch)
2011-04-10 18:45 PDT, Craig Topper
no flags Details | Diff | Review
Part 3: Factor some common method bodies into css::Rule (6.96 KB, patch)
2011-04-10 19:33 PDT, Craig Topper
no flags Details | Diff | Review
Part 3: Factor some common method bodies into css::Rule (9.13 KB, patch)
2011-04-10 20:08 PDT, Craig Topper
no flags Details | Diff | Review
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak (9.29 KB, patch)
2011-04-10 20:22 PDT, Craig Topper
no flags Details | Diff | Review
Part 1: Change uses of nsICSSRule to css::Rule (78.47 KB, patch)
2011-04-12 22:36 PDT, Craig Topper
no flags Details | Diff | Review
Part 2: Make several css::Rule methods non-virtual and inline (2.37 KB, patch)
2011-04-12 22:41 PDT, Craig Topper
bzbarsky: review+
Details | Diff | Review
Part 3: Factor some common method bodies into css::Rule (9.99 KB, patch)
2011-04-12 23:33 PDT, Craig Topper
bzbarsky: review-
Details | Diff | Review
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak (10.26 KB, patch)
2011-04-12 23:58 PDT, Craig Topper
bzbarsky: review+
Details | Diff | Review
Part 1: Change uses of nsICSSRule to css::Rule (77.83 KB, patch)
2011-04-30 14:27 PDT, Craig Topper
bzbarsky: review+
Details | Diff | Review
Part 3: Factor some common method bodies into css::Rule (16.00 KB, patch)
2011-05-06 00:20 PDT, Craig Topper
bzbarsky: review+
Details | Diff | Review
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak (10.25 KB, patch)
2011-05-06 00:20 PDT, Craig Topper
no flags Details | Diff | Review
Part 5: Make nsCSSKeyframeRule use inherited AddRef/Release from Rule (2.83 KB, patch)
2011-05-06 00:34 PDT, Craig Topper
bzbarsky: review+
Details | Diff | Review

Description Craig Topper 2011-04-10 18:30:44 PDT
This class isn't needed anymore as we can move everyone to use the concrete css::Rule class. This allows us to make some of the methods in css::Rule non-virtual and inlined.
Comment 1 Craig Topper 2011-04-10 18:42:01 PDT
Created attachment 525015 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule
Comment 2 Craig Topper 2011-04-10 18:45:52 PDT
Created attachment 525016 [details] [diff] [review]
Part 2: Make several css::Rule methods non-virtual and inline
Comment 3 Craig Topper 2011-04-10 19:33:30 PDT
Created attachment 525019 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule
Comment 4 Craig Topper 2011-04-10 20:08:45 PDT
Created attachment 525023 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule
Comment 5 Craig Topper 2011-04-10 20:22:54 PDT
Created attachment 525026 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak
Comment 6 Craig Topper 2011-04-10 20:29:22 PDT
Comment on attachment 525015 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule

One note on this patch is that I'm still using nsCOMArray<css::Rule> because nsTArray doesn't provide the enumeration facilities that nsCOMArray does. I'll work on a follow up patch to fix that.
Comment 7 Craig Topper 2011-04-10 20:31:47 PDT
Comment on attachment 525016 [details] [diff] [review]
Part 2: Make several css::Rule methods non-virtual and inline

This also changes the return type on GetStyleSheet() to match the type of mSheet.
Comment 8 Craig Topper 2011-04-10 20:33:45 PDT
Comment on attachment 525026 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak

This leaves only the "weak" version behind, but I renamed it to remove Weak from its name.
Comment 9 Craig Topper 2011-04-12 22:36:42 PDT
Created attachment 525623 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule
Comment 10 Craig Topper 2011-04-12 22:41:33 PDT
Created attachment 525625 [details] [diff] [review]
Part 2: Make several css::Rule methods non-virtual and inline
Comment 11 Craig Topper 2011-04-12 23:33:30 PDT
Created attachment 525634 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule
Comment 12 Craig Topper 2011-04-12 23:58:01 PDT
Created attachment 525637 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak
Comment 13 Craig Topper 2011-04-30 14:27:12 PDT
Created attachment 529311 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule
Comment 14 Craig Topper 2011-04-30 14:28:23 PDT
Comment on attachment 529311 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule

Updated for minor bitrot
Comment 15 Boris Zbarsky [:bz] 2011-05-05 10:40:57 PDT
Comment on attachment 525625 [details] [diff] [review]
Part 2: Make several css::Rule methods non-virtual and inline

r=me
Comment 16 Boris Zbarsky [:bz] 2011-05-05 10:45:55 PDT
Comment on attachment 525634 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule

> MediaRule::GetParentRule(nsIDOMCSSRule** aParentRule)
>-  return GroupRule::GetParentRule(aParentRule);
>+  return Rule::GetParentRule(aParentRule);

This could keep calling GroupRule::GetParentRule, right?  Please leave that as it was.

Same for DocumentRule.  Also, both should call GroupRule::GetParentStyleSheet.

> nsCSSKeyframeRule::GetParentStyleSheet(nsIDOMCSSStyleSheet** aSheet)
>+  return css::Rule::GetParentStyleSheet(aSheet);

Why is the "css::" needed there?

Same for nsCSSKeyframesRule::GetParentStyleSheet and nsCSSKeyframesRule::GetParentRule.  Also, keyframes should forward to GroupRule, no?
Comment 17 Boris Zbarsky [:bz] 2011-05-05 13:45:46 PDT
Comment on attachment 525637 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak

> GroupRule::AppendRulesToCssText(nsAString& aCssText)
>+    nsCOMPtr<nsIDOMCSSRule> domRule = rule->GetDOMRule();

That could just be nsIDOMCSSRule*.

> nsCSSFontFaceStyleDecl::GetParentRule(nsIDOMCSSRule** aParentRule)
> {
>-  return ContainingRule()->GetDOMRule(aParentRule);
>+   NS_IF_ADDREF(*aParentRule = ContainingRule()->GetDOMRule());
>+   return NS_OK;

Fix the indent.

r=me with those.
Comment 18 Boris Zbarsky [:bz] 2011-05-05 13:55:13 PDT
Comment on attachment 529311 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule

r=me

If you decide you want me to push these, just attach updated patches and let me know, ok?
Comment 19 Craig Topper 2011-05-06 00:20:27 PDT
Created attachment 530559 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule
Comment 20 Craig Topper 2011-05-06 00:20:47 PDT
Created attachment 530560 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak
Comment 21 Craig Topper 2011-05-06 00:33:04 PDT
Comment on attachment 530559 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule

Fixed review comments. Better factored GetCssRules. Also removed unnecessary mozilla:: in front of css in several places since the namespace is aliased at the top of the file.
Comment 22 Craig Topper 2011-05-06 00:34:06 PDT
Created attachment 530561 [details] [diff] [review]
Part 5: Make nsCSSKeyframeRule use inherited AddRef/Release from Rule
Comment 23 Craig Topper 2011-05-06 00:37:29 PDT
Comment on attachment 530561 [details] [diff] [review]
Part 5: Make nsCSSKeyframeRule use inherited AddRef/Release from Rule

Also made MediaRule/DocumentRule specify inheriting AddRef/Release from GroupRule instead of Rule to be consistent with review comments from part 3 for other forwarding cases.
Comment 24 Boris Zbarsky [:bz] 2011-05-06 09:37:01 PDT
Comment on attachment 530559 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule

r=me
Comment 25 Boris Zbarsky [:bz] 2011-05-06 09:38:39 PDT
Comment on attachment 530561 [details] [diff] [review]
Part 5: Make nsCSSKeyframeRule use inherited AddRef/Release from Rule

r=me
Comment 26 Craig Topper 2011-05-06 09:59:30 PDT
Can you land these if you get a chance? If not I can do it sometime this weekend.
Comment 27 Boris Zbarsky [:bz] 2011-05-06 10:04:22 PDT
I can do it.

For future reference, having the r=whoever at the end of the checkin comment would be nice too.  ;)
Comment 28 Craig Topper 2011-05-06 10:41:24 PDT
Didn't know if I was allowed to do that before it was reviewed. I'll do that in the future.

Note You need to log in before you can comment on or make changes to this bug.