Closed
Bug 577974
Opened 13 years ago
Closed 13 years ago
Remove nsICSSGroupRule and deCOM methods that came from it
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
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.
Assignee | ||
Comment 1•13 years ago
|
||
Also converted a member in nsCSSGroupRule to nsRefPtr to remove manual reference counting.
Attachment #456786 -
Flags: review?(dbaron)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #456787 -
Flags: review?(dbaron)
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 3•13 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•13 years ago
|
Attachment #456787 -
Flags: review?(dbaron)
Assignee | ||
Updated•13 years ago
|
Attachment #456795 -
Flags: review?(dbaron)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #520141 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #456795 -
Attachment is obsolete: true
Attachment #520142 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #456787 -
Attachment is obsolete: true
Attachment #520143 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #520144 -
Flags: review?(bzbarsky)
Comment 8•13 years ago
|
||
Patches 3 and 4 appear to have their order reversed.
Assignee | ||
Comment 9•13 years ago
|
||
Oops(In reply to comment #8) > Patches 3 and 4 appear to have their order reversed. I think you're right. Sorry.
Comment 10•13 years ago
|
||
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•13 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?
Comment 12•13 years ago
|
||
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•13 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•13 years ago
|
Attachment #520141 -
Attachment is obsolete: true
Attachment #520141 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #520142 -
Attachment is obsolete: true
Attachment #520142 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 15•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #520143 -
Attachment is obsolete: true
Attachment #520143 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #520144 -
Attachment is obsolete: true
Attachment #520144 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 16•13 years ago
|
||
Assignee | ||
Comment 17•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #523944 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #523945 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #523946 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #523947 -
Flags: review?(bzbarsky)
![]() |
||
Comment 18•13 years ago
|
||
Comment on attachment 523944 [details] [diff] [review] Part 1: Rename CSSGroupRuleListImpl r=me
Attachment #523944 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 19•13 years ago
|
||
Comment on attachment 523945 [details] [diff] [review] Part 2: Remove nsICSSGroupRule r=me
Attachment #523945 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 20•13 years ago
|
||
Comment on attachment 523946 [details] [diff] [review] Part 3: Rename nsCSSDocumentRule and nsCSSMediaRule r=me
Attachment #523946 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 21•13 years ago
|
||
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 | ||
Comment 22•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #523947 -
Attachment is obsolete: true
Assignee | ||
Comment 23•13 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 | ||
Comment 24•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #524122 -
Attachment is obsolete: true
Attachment #524122 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 25•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #524127 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #524132 -
Flags: review?(bzbarsky)
![]() |
||
Comment 26•13 years ago
|
||
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 | ||
Comment 27•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #524132 -
Attachment is obsolete: true
![]() |
||
Comment 28•13 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
You need to log in
before you can comment on or make changes to this bug.
Description
•