The default bug view has changed. See this FAQ.

Status

()

Core
CSS Parsing and Computation
--
minor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Craig Topper, Assigned: Craig Topper)

Tracking

(Blocks: 1 bug)

Trunk
mozilla6
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 8 obsolete attachments)

2.37 KB, patch
Details | Diff | Splinter Review
77.83 KB, patch
Details | Diff | Splinter Review
16.00 KB, patch
Details | Diff | Splinter Review
10.25 KB, patch
Details | Diff | Splinter Review
2.83 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 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

6 years ago
Created attachment 525015 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule
(Assignee)

Comment 2

6 years ago
Created attachment 525016 [details] [diff] [review]
Part 2: Make several css::Rule methods non-virtual and inline
(Assignee)

Comment 3

6 years ago
Created attachment 525019 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule
(Assignee)

Comment 4

6 years ago
Created attachment 525023 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule
(Assignee)

Updated

6 years ago
Attachment #525019 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Created attachment 525026 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak
(Assignee)

Comment 6

6 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

6 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

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

Comment 8

6 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

6 years ago
Created attachment 525623 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule
(Assignee)

Updated

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

Comment 10

6 years ago
Created attachment 525625 [details] [diff] [review]
Part 2: Make several css::Rule methods non-virtual and inline
(Assignee)

Updated

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

Updated

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

Updated

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

Comment 11

6 years ago
Created attachment 525634 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule
(Assignee)

Comment 12

6 years ago
Created attachment 525637 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Comment 13

6 years ago
Created attachment 529311 [details] [diff] [review]
Part 1: Change uses of nsICSSRule to css::Rule
(Assignee)

Updated

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

Comment 14

6 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)

Comment 19

6 years ago
Created attachment 530559 [details] [diff] [review]
Part 3: Factor some common method bodies into css::Rule
(Assignee)

Updated

6 years ago
Attachment #525634 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Created attachment 530560 [details] [diff] [review]
Part 4: DeCOMtaminate GetDOMRule and GetDOMRuleWeak
(Assignee)

Updated

6 years ago
Attachment #525637 - Attachment is obsolete: true
(Assignee)

Comment 21

6 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

6 years ago
Created attachment 530561 [details] [diff] [review]
Part 5: Make nsCSSKeyframeRule use inherited AddRef/Release from Rule
(Assignee)

Comment 23

6 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

6 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

6 years ago
Didn't know if I was allowed to do that before it was reviewed. I'll do that in the future.
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
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
(Assignee)

Updated

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