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
:
: Jet Villegas (:jet)
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 | Splinter 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 | Splinter 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 | Splinter 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 | Splinter Review
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak (9.29 KB, patch)
2011-04-10 20:22 PDT, Craig Topper
no flags Details | Diff | Splinter 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 | Splinter 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 | Splinter 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 | Splinter Review
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak (10.26 KB, patch)
2011-04-12 23:58 PDT, Craig Topper
bzbarsky: review+
Details | Diff | Splinter 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 | Splinter 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 | Splinter Review
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak (10.25 KB, patch)
2011-05-06 00:20 PDT, Craig Topper
no flags Details | Diff | Splinter 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 | Splinter Review

Description User image 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 User image 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 User image 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 User image 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 User image 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 User image Craig Topper 2011-04-10 20:22:54 PDT
Created attachment 525026 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak
Comment 6 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Craig Topper 2011-04-12 23:58:01 PDT
Created attachment 525637 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak
Comment 13 User image 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 User image 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 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image 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 User image Craig Topper 2011-05-06 00:20:47 PDT
Created attachment 530560 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak
Comment 21 User image 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 User image 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 User image 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 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image 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 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image 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.