648925 - Remove nsICSSRule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Craig Topper]

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]

Attachment: Part 1: Change uses of nsICSSRule to css::Rule

Comment 2

[Craig Topper]

Attachment: Part 2: Make several css::Rule methods non-virtual and inline

Comment 3

[Craig Topper]

Attachment: Part 3: Factor some common method bodies into css::Rule

Comment 4

[Craig Topper]

Attachment: Part 3: Factor some common method bodies into css::Rule

Comment 5

[Craig Topper]

Attachment: Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak

Comment 6

[Craig Topper]

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]

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]

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]

Attachment: Part 1: Change uses of nsICSSRule to css::Rule

Comment 10

[Craig Topper]

Attachment: Part 2: Make several css::Rule methods non-virtual and inline

Comment 11

[Craig Topper]

Attachment: Part 3: Factor some common method bodies into css::Rule

Comment 12

[Craig Topper]

Attachment: Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak

Comment 13

[Craig Topper]

Attachment: Part 1: Change uses of nsICSSRule to css::Rule

Comment 14

[Craig Topper]

Comment on attachment 529311 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule

Updated for minor bitrot

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 525625 [details] [diff] [review]
Part 2: Make several css::Rule methods non-virtual and inline

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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]

Attachment: Part 3: Factor some common method bodies into css::Rule

Comment 20

[Craig Topper]

Attachment: Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak

Comment 21

[Craig Topper]

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]

Attachment: Part 5: Make nsCSSKeyframeRule use inherited AddRef/Release from Rule

Comment 23

[Craig Topper]

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 [:bzbarsky]]

Comment on attachment 530559 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule

r=me

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 530561 [details] [diff] [review]
Part 5: Make nsCSSKeyframeRule use inherited AddRef/Release from Rule

r=me

Comment 26

[Craig Topper]

Can you land these if you get a chance? If not I can do it sometime this weekend.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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]

Didn't know if I was allowed to do that before it was reviewed. I'll do that in the future.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Pushed to cedar:
  http://hg.mozilla.org/projects/cedar/rev/6140c5320525
  http://hg.mozilla.org/projects/cedar/rev/750d39ddb81f
  http://hg.mozilla.org/projects/cedar/rev/f19cd0781e64
  http://hg.mozilla.org/projects/cedar/rev/74f3c3baa67a
  http://hg.mozilla.org/projects/cedar/rev/dbccf95e0a81

Comment 30

[Boris Zbarsky [:bzbarsky]]

Fixed:
  http://hg.mozilla.org/mozilla-central/rev/6140c5320525
  http://hg.mozilla.org/mozilla-central/rev/750d39ddb81f
  http://hg.mozilla.org/mozilla-central/rev/f19cd0781e64
  http://hg.mozilla.org/mozilla-central/rev/74f3c3baa67a
  http://hg.mozilla.org/mozilla-central/rev/dbccf95e0a81