Closed
Bug 648925
Opened 13 years ago
Closed 13 years ago
Remove nsICSSRule
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: craig.topper, Assigned: craig.topper)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 8 obsolete files)
2.37 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
77.83 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
16.00 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
10.25 KB,
patch
|
Details | Diff | Splinter Review | |
2.83 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
Assignee | ||
Comment 3•13 years ago
|
||
Assignee | ||
Comment 4•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #525019 -
Attachment is obsolete: true
Assignee | ||
Comment 5•13 years ago
|
||
Assignee | ||
Comment 6•13 years ago
|
||
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.
Attachment #525015 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•13 years ago
|
||
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.
Attachment #525016 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #525023 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•13 years ago
|
||
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.
Attachment #525026 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #525015 -
Attachment is obsolete: true
Attachment #525015 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #525016 -
Attachment is obsolete: true
Attachment #525016 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #525026 -
Attachment is obsolete: true
Attachment #525026 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #525023 -
Attachment is obsolete: true
Attachment #525023 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 11•13 years ago
|
||
Assignee | ||
Comment 12•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #525623 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #525625 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #525634 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #525637 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #525623 -
Attachment is obsolete: true
Attachment #525623 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•13 years ago
|
||
Comment on attachment 529311 [details] [diff] [review] Part 1: Change uses of nsICSSRule to css::Rule Updated for minor bitrot
Attachment #529311 -
Flags: review?(bzbarsky)
![]() |
||
Comment 15•13 years ago
|
||
Comment on attachment 525625 [details] [diff] [review] Part 2: Make several css::Rule methods non-virtual and inline r=me
Attachment #525625 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 16•13 years ago
|
||
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?
Attachment #525634 -
Flags: review?(bzbarsky) → review-
![]() |
||
Comment 17•13 years ago
|
||
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.
Attachment #525637 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 18•13 years ago
|
||
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?
Attachment #529311 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 19•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #525634 -
Attachment is obsolete: true
Assignee | ||
Comment 20•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #525637 -
Attachment is obsolete: true
Assignee | ||
Comment 21•13 years ago
|
||
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.
Attachment #530559 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 22•13 years ago
|
||
Assignee | ||
Comment 23•13 years ago
|
||
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.
Attachment #530561 -
Flags: review?(bzbarsky)
![]() |
||
Comment 24•13 years ago
|
||
Comment on attachment 530559 [details] [diff] [review] Part 3: Factor some common method bodies into css::Rule r=me
Attachment #530559 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 25•13 years ago
|
||
Comment on attachment 530561 [details] [diff] [review] Part 5: Make nsCSSKeyframeRule use inherited AddRef/Release from Rule r=me
Attachment #530561 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 26•13 years ago
|
||
Can you land these if you get a chance? If not I can do it sometime this weekend.
![]() |
||
Comment 27•13 years ago
|
||
I can do it. For future reference, having the r=whoever at the end of the checkin comment would be nice too. ;)
Assignee | ||
Comment 28•13 years ago
|
||
Didn't know if I was allowed to do that before it was reviewed. I'll do that in the future.
![]() |
||
Comment 29•13 years ago
|
||
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
Flags: in-testsuite-
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla6
![]() |
||
Comment 30•13 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
You need to log in
before you can comment on or make changes to this bug.
Description
•