Status

()

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

People

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

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 8 obsolete attachments)

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
Assignee

Description

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

8 years ago
Assignee

Updated

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

Comment 6

8 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

8 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

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

Comment 8

8 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

8 years ago
Assignee

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 14

8 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 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 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 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 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

Updated

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

Updated

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

Comment 21

8 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 23

8 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 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 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

8 years ago
Can you land these if you get a chance? If not I can do it sometime this weekend.
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

8 years ago
Didn't know if I was allowed to do that before it was reviewed. I'll do that in the future.
Assignee

Updated

8 years ago
Blocks: deCOM
You need to log in before you can comment on or make changes to this bug.