577974 - Remove nsICSSGroupRule and deCOM methods that came from it

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Craig Topper]

nsICSSGroupRule is only implemented by nsCSSGroupRule. This should be collapsed and everyone should just use nsCSSGroupRule. Then we should deCOM and devirtualize the methods.

Comment 1

[Craig Topper]

Attachment: Part 1: Remove nsICSSGroupRule

Also converted a member in nsCSSGroupRule to nsRefPtr to remove manual reference counting.

Comment 2

[Craig Topper]

Attachment: Part 2: DeCOM and devirtualize nsCSSGroupRule methods

Comment 3

[Craig Topper]

Attachment: Part 1: Remove nsICSSGroupRule

Updated due to patch in another bug

Comment 4

[Craig Topper]

Attachment: Part 1: Rename CSSGroupRuleListImp

Comment 5

[Craig Topper]

Attachment: Part 2: Remove nsICSSGroupRule

Comment 6

[Craig Topper]

Attachment: Part 3: DeCOM and de-virtualize methods in GroupRule

Comment 7

[Craig Topper]

Attachment: Part 4: Rename nsCSSDocumentRule and nsCSSMediaRule

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Patches 3 and 4 appear to have their order reversed.

Comment 9

[Craig Topper]

Oops(In reply to comment #8)
> Patches 3 and 4 appear to have their order reversed.

I think you're right. Sorry.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, I get:

/home/dbaron/builds/ssd/mozilla-central/mozilla/layout/style/nsCSSRules.cpp:91: error: incomplete type ‘mozilla::css::GroupRuleRuleList’ not allowed

(repeated about 10 times)

I think the DOMCI_DATA needs to move down below the class declaration.

Comment 11

[Craig Topper]

Interesting. I'll have to check when I get home, but I'm pretty sure I was able to build with it where it is. Though I can see why it wouldn't work. What OS are you compiling on?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

64-bit Ubuntu 10.10, gcc 4.4.5

One other idea:  you might be able to avoid the namespace hacks around the uses of DOMCI_DATA if you put a :: inside the contents of the DOMCI_DATA macro.

Comment 13

[Craig Topper]

Didn't work. Compiler does like declaring a variable with :: in front of it.

Got errors like this:

dom/base/nsDOMException.cpp:156: error: explicit qualification in declaration of ‘kDOMClassInfo_DOMException_interfaces’

Comment 14

[Craig Topper]

Attachment: Part 1: Rename CSSGroupRuleListImpl

Comment 15

[Craig Topper]

Attachment: Part 2: Remove nsICSSGroupRule

Comment 16

[Craig Topper]

Attachment: Part 3: Rename nsCSSDocumentRule and nsCSSMediaRule

Comment 17

[Craig Topper]

Attachment: Part 4: DeCOM and de-virtualize methods in GroupRule

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 523944 [details] [diff] [review]
Part 1: Rename CSSGroupRuleListImpl

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 523945 [details] [diff] [review]
Part 2: Remove nsICSSGroupRule

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 523946 [details] [diff] [review]
Part 3: Rename nsCSSDocumentRule and nsCSSMediaRule

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 523947 [details] [diff] [review]
Part 4: DeCOM and de-virtualize methods in GroupRule

>+++ b/layout/style/GroupRule.h
>+  PRInt32 StyleRuleCount() const { return mRules.Count(); }
>+  nsresult GetStyleRuleAt(PRInt32 aIndex, nsICSSRule*& aRule) const;

Why not just:

  nsICSSRule* GetStyleRuleAt(PRInt32 aIndex) const;

returning null on out-of-bounds?  Seems like that would work better.

>+  already_AddRefed<nsIDOMCSSStyleSheet> GetParentStyleSheet();

Why can't this return nsIDOMCSSStyleSheet* instead?

>   nsresult GetParentRule(nsIDOMCSSRule** aParentRule);

This will presumably get deCOMed when we deCOM mParentRule, right?

>+  already_AddRefed<nsIDOMCSSRuleList> GetCssRules();

Why can't this return nsIDOMCSSRuleList* instead?

r- for now; I'd really prefer we return weak pointers instead of manually refcounting smart pointers.  But if there's a good reason to do things the way this patch does, I'm open to being convinced!

Comment 22

[Craig Topper]

Attachment: Part 4: DeCOM and de-virtualize methods in GroupRule

Comment 23

[Craig Topper]

Comment on attachment 524122 [details] [diff] [review]
Part 4: DeCOM and de-virtualize methods in GroupRule

This addresses everything but GetParentRule. That needs to be cleaned up on all rule classes which I will deal with in another bug.

Comment 24

[Craig Topper]

Attachment: Part 4: DeCOM and de-virtualize methods in GroupRule

Comment 25

[Craig Topper]

Attachment: Part 4: DeCOM and de-virtualize methods in GroupRule

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 524132 [details] [diff] [review]
Part 4: DeCOM and de-virtualize methods in GroupRule

> +GroupRule::GetStyleRuleAt(PRInt32 aIndex) const

This should just do:

  return mRules.SafeObjectAt(aIndex);

instead of reimplementing it.

r=me with that nit.

Comment 27

[Craig Topper]

Attachment: Part 4: DeCOM and de-virtualize methods in GroupRule

Comment 28

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/mozilla-central/rev/45d658bb4be1
  http://hg.mozilla.org/mozilla-central/rev/f100d70f483a
  http://hg.mozilla.org/mozilla-central/rev/8bd0c6330318
  http://hg.mozilla.org/mozilla-central/rev/7e05a50ed66b