Closed Bug 577974 Opened 10 years ago Closed 10 years ago

Remove nsICSSGroupRule and deCOM methods that came from it

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: craig.topper, Assigned: craig.topper)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 11 obsolete files)

7.13 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
42.51 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
16.69 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.84 KB, patch
Details | Diff | Splinter Review
nsICSSGroupRule is only implemented by nsCSSGroupRule. This should be collapsed and everyone should just use nsCSSGroupRule. Then we should deCOM and devirtualize the methods.
Attached patch Part 1: Remove nsICSSGroupRule (obsolete) — Splinter Review
Also converted a member in nsCSSGroupRule to nsRefPtr to remove manual reference counting.
Attachment #456786 - Flags: review?(dbaron)
Attachment #456787 - Flags: review?(dbaron)
Depends on: deCOM
Blocks: deCOM
No longer depends on: deCOM
Blocks: 577976
Attached patch Part 1: Remove nsICSSGroupRule (obsolete) — Splinter Review
Updated due to patch in another bug
Attachment #456786 - Attachment is obsolete: true
Attachment #456795 - Flags: review?(dbaron)
Attachment #456786 - Flags: review?(dbaron)
Depends on: 577002
Attachment #456787 - Flags: review?(dbaron)
Attachment #456795 - Flags: review?(dbaron)
Attachment #520141 - Flags: review?(bzbarsky)
Attached patch Part 2: Remove nsICSSGroupRule (obsolete) — Splinter Review
Attachment #456795 - Attachment is obsolete: true
Attachment #520142 - Flags: review?(bzbarsky)
Attachment #456787 - Attachment is obsolete: true
Attachment #520143 - Flags: review?(bzbarsky)
Attachment #520144 - Flags: review?(bzbarsky)
Patches 3 and 4 appear to have their order reversed.
Oops(In reply to comment #8)
> Patches 3 and 4 appear to have their order reversed.

I think you're right. Sorry.
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.
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?
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.
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’
Attachment #520141 - Attachment is obsolete: true
Attachment #520141 - Flags: review?(bzbarsky)
Attachment #520142 - Attachment is obsolete: true
Attachment #520142 - Flags: review?(bzbarsky)
Attachment #520143 - Attachment is obsolete: true
Attachment #520143 - Flags: review?(bzbarsky)
Attachment #520144 - Attachment is obsolete: true
Attachment #520144 - Flags: review?(bzbarsky)
Attachment #523944 - Flags: review?(bzbarsky)
Attachment #523945 - Flags: review?(bzbarsky)
Attachment #523946 - Flags: review?(bzbarsky)
Attachment #523947 - Flags: review?(bzbarsky)
Comment on attachment 523944 [details] [diff] [review]
Part 1: Rename CSSGroupRuleListImpl

r=me
Attachment #523944 - Flags: review?(bzbarsky) → review+
Comment on attachment 523945 [details] [diff] [review]
Part 2: Remove nsICSSGroupRule

r=me
Attachment #523945 - Flags: review?(bzbarsky) → review+
Comment on attachment 523946 [details] [diff] [review]
Part 3: Rename nsCSSDocumentRule and nsCSSMediaRule

r=me
Attachment #523946 - Flags: review?(bzbarsky) → review+
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!
Attachment #523947 - Flags: review?(bzbarsky) → review-
Attachment #523947 - Attachment is obsolete: true
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.
Attachment #524122 - Flags: review?(bzbarsky)
Attachment #524122 - Attachment is obsolete: true
Attachment #524122 - Flags: review?(bzbarsky)
Attachment #524127 - Attachment is obsolete: true
Attachment #524132 - Flags: review?(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.
Attachment #524132 - Flags: review?(bzbarsky) → review+
Attachment #524132 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.