Remove nsICSSGroupRule and deCOM methods that came from it

RESOLVED FIXED in mozilla5

Status

()

defect
--
minor
RESOLVED FIXED
9 years ago
8 years ago

People

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

Tracking

(Blocks 1 bug)

Trunk
mozilla5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 11 obsolete attachments)

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
Assignee

Description

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

Comment 1

9 years ago
Also converted a member in nsCSSGroupRule to nsRefPtr to remove manual reference counting.
Attachment #456786 - Flags: review?(dbaron)
Assignee

Comment 2

9 years ago
Attachment #456787 - Flags: review?(dbaron)
Assignee

Updated

9 years ago
Depends on: deCOM
Assignee

Updated

9 years ago
Blocks: deCOM
No longer depends on: deCOM
Assignee

Updated

9 years ago
Blocks: 577976
Assignee

Comment 3

9 years ago
Updated due to patch in another bug
Attachment #456786 - Attachment is obsolete: true
Attachment #456795 - Flags: review?(dbaron)
Attachment #456786 - Flags: review?(dbaron)
Assignee

Updated

9 years ago
Depends on: 577002
Assignee

Updated

9 years ago
Attachment #456787 - Flags: review?(dbaron)
Assignee

Updated

9 years ago
Attachment #456795 - Flags: review?(dbaron)
Assignee

Comment 4

8 years ago
Attachment #520141 - Flags: review?(bzbarsky)
Assignee

Comment 5

8 years ago
Attachment #456795 - Attachment is obsolete: true
Attachment #520142 - Flags: review?(bzbarsky)
Assignee

Comment 6

8 years ago
Attachment #456787 - Attachment is obsolete: true
Attachment #520143 - Flags: review?(bzbarsky)
Assignee

Comment 7

8 years ago
Attachment #520144 - Flags: review?(bzbarsky)
Patches 3 and 4 appear to have their order reversed.
Assignee

Comment 9

8 years ago
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.
Assignee

Comment 11

8 years ago
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.
Assignee

Comment 13

8 years ago
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’
Assignee

Updated

8 years ago
Attachment #520141 - Attachment is obsolete: true
Attachment #520141 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #520142 - Attachment is obsolete: true
Attachment #520142 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #520143 - Attachment is obsolete: true
Attachment #520143 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #520144 - Attachment is obsolete: true
Attachment #520144 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #523944 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #523945 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #523946 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
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-
Assignee

Updated

8 years ago
Attachment #523947 - Attachment is obsolete: true
Assignee

Comment 23

8 years ago
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)
Assignee

Updated

8 years ago
Attachment #524122 - Attachment is obsolete: true
Attachment #524122 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #524127 - Attachment is obsolete: true
Assignee

Updated

8 years ago
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+
Assignee

Updated

8 years ago
Attachment #524132 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.