Cleanup multiple inheritance of nsICSSRule and nsCSSRule

RESOLVED FIXED in mozilla5

Status

()

Core
CSS Parsing and Computation
--
minor
RESOLVED FIXED
8 years ago
5 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

(6 attachments, 11 obsolete attachments)

8.64 KB, patch
Details | Diff | Splinter Review
5.19 KB, patch
Details | Diff | Splinter Review
16.28 KB, patch
Details | Diff | Splinter Review
9.53 KB, patch
Details | Diff | Splinter Review
5.05 KB, patch
Details | Diff | Splinter Review
979 bytes, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
nsICSSRule was the base class for all the other rule interfaces that are being removed by other bugs. nsCSSRule was then used to implement the common code for all the concrete rule classes that implemented those interfaces. This results in a bunch of virtual methods in the concrete classes to resolve the abiguity and forward to the implementation in nsCSSRule.

Now that the other interfaces are going away there is no need to preserve a pure interface inheritance path. At the very least we should make the rule classes inherit from nsCSSRule and then make nsCSSRule inherit from nsICSSRule. This will remove the multiple inheritance and allow the forwarding methods to be removed.

May go further and look into merging nsICSSRule and nsCSSRule to see if we can devirtualize some methods.
(Assignee)

Updated

8 years ago
Depends on: 576831, 576877, 577002, 577974
(Assignee)

Comment 1

8 years ago
Created attachment 456789 [details] [diff] [review]
Part 1: Make nsCSSRule inherit from nsICSSRule and remove inheritance of nsICSSRule from other classes
Attachment #456789 - Flags: review?(dbaron)
(Assignee)

Comment 2

8 years ago
Created attachment 456796 [details] [diff] [review]
Part 1: Make nsCSSRule inherit from nsICSSRule and remove inheritance of nsICSSRule from other classes

Updated due to patches in other bugs
Attachment #456789 - Attachment is obsolete: true
Attachment #456796 - Flags: review?(dbaron)
Attachment #456789 - Flags: review?(dbaron)
(Assignee)

Comment 3

8 years ago
Created attachment 456804 [details] [diff] [review]
Part 2: Remove the forwarding methods
Attachment #456804 - Flags: review?(dbaron)
(Assignee)

Updated

8 years ago
Attachment #456796 - Flags: review?(dbaron)
(Assignee)

Updated

8 years ago
Attachment #456804 - Flags: review?(dbaron)
(Assignee)

Updated

7 years ago
Attachment #456796 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #456804 - Attachment is obsolete: true
(Assignee)

Comment 4

7 years ago
Created attachment 522563 [details] [diff] [review]
Part 1: Make nsCSSRule inherit from nsICSSRule and remove inheritance of nsICSSRule from other classes
(Assignee)

Comment 5

7 years ago
Created attachment 522567 [details] [diff] [review]
Part 2: Remove forwarding methods to nsCSSRule from Rule classes
(Assignee)

Comment 6

7 years ago
Created attachment 522569 [details] [diff] [review]
Part 3: Rename nsCSSRule and put in css namespace
(Assignee)

Comment 7

7 years ago
Created attachment 522594 [details] [diff] [review]
Part 4: Move AddRef/Release back to nsCSSRule
(Assignee)

Comment 8

7 years ago
Created attachment 522595 [details] [diff] [review]
Part 5: Remove AddRef from GetStyleSheet
(Assignee)

Updated

7 years ago
Attachment #522563 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Attachment #522567 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Attachment #522569 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Attachment #522594 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Attachment #522595 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

7 years ago
That last one isn't exactly related but it's context heavily depends on the other patches.
Comment on attachment 522563 [details] [diff] [review]
Part 1: Make nsCSSRule inherit from nsICSSRule and remove inheritance of nsICSSRule from other classes

>+++ b/layout/style/nsCSSRule.h
>+  virtual already_AddRefed<nsIStyleSheet> GetStyleSheet() const;
>+  virtual void SetStyleSheet(nsCSSStyleSheet* aSheet);
>+  virtual void SetParentRule(mozilla::css::GroupRule* aRule);

Why do those need to be virtual?

r=me on the rest, but I'd like to understand why those are virtual before marking.
I guess I see why SetStyleSheet needs to be virtual.  But why do the other two need it?
Comment on attachment 522567 [details] [diff] [review]
Part 2: Remove forwarding methods to nsCSSRule from Rule classes

>-    nsCOMPtr<nsIStyleSheet> sheet = mOwnerRule->GetStyleSheet();
>+    nsCOMPtr<nsIStyleSheet> sheet = static_cast<nsCSSRule*>(mOwnerRule)->GetStyleSheet();

Is this needed due to shadowing?

If so, r=me.
Attachment #522567 - Flags: review?(bzbarsky) → review+
Comment on attachment 522569 [details] [diff] [review]
Part 3: Rename nsCSSRule and put in css namespace

This looks like it did an hg rm of nsCSSRule.h and an hg add of Rule.h instead of doing an hg move.  Please fix that?
Attachment #522569 - Flags: review?(bzbarsky) → review-
Comment on attachment 522594 [details] [diff] [review]
Part 4: Move AddRef/Release back to nsCSSRule

r=me
Attachment #522594 - Flags: review?(bzbarsky) → review+
Comment on attachment 522595 [details] [diff] [review]
Part 5: Remove AddRef from GetStyleSheet

r=me
Attachment #522595 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

7 years ago
(In reply to comment #11)
> I guess I see why SetStyleSheet needs to be virtual.  But why do the other two
> need it?

Aren't they all virtual by virtue of the fact that they are pure virtual in nsICSSRule which is now Rule's base?

virtual already_AddRefed<nsIStyleSheet> GetStyleSheet() const = 0;
virtual void SetStyleSheet(nsCSSStyleSheet* aSheet) = 0;
virtual void SetParentRule(nsICSSGroupRule* aRule) = 0;
(Assignee)

Comment 17

7 years ago
(In reply to comment #12)
> Comment on attachment 522567 [details] [diff] [review]
> Part 2: Remove forwarding methods to nsCSSRule from Rule classes
> 
> >-    nsCOMPtr<nsIStyleSheet> sheet = mOwnerRule->GetStyleSheet();
> >+    nsCOMPtr<nsIStyleSheet> sheet = static_cast<nsCSSRule*>(mOwnerRule)->GetStyleSheet();
> 
> Is this needed due to shadowing?
> 
> If so, r=me.

Yeah I got an error because there is a DOM version of GetStyleSheet on ImportRule and since the forwarding method was removed from ImportRule, the DOM version hides GetStyleSheet(). It also causes a bunch of warnings about it being hidden to fire too, but I don't know how to fix that without putting back forwarding.
(Assignee)

Comment 18

7 years ago
Created attachment 524124 [details] [diff] [review]
Part 3: Rename nsCSSRule and put in css namespace
(Assignee)

Updated

7 years ago
Attachment #522569 - Attachment is obsolete: true
(Assignee)

Comment 19

7 years ago
Created attachment 524125 [details] [diff] [review]
Part 4: Move AddRef/Release back to nsCSSRule
(Assignee)

Updated

7 years ago
Attachment #522594 - Attachment is obsolete: true
(Assignee)

Comment 20

7 years ago
Created attachment 524126 [details] [diff] [review]
Part 5: Remove AddRef from GetStyleSheet
(Assignee)

Updated

7 years ago
Attachment #522595 - Attachment is obsolete: true
(Assignee)

Comment 21

7 years ago
Comment on attachment 524124 [details] [diff] [review]
Part 3: Rename nsCSSRule and put in css namespace

Fixed to rename the file properly
Attachment #524124 - Flags: review?(bzbarsky)
(Assignee)

Comment 22

7 years ago
Had to update patch 4 and 5 to deal with changes from bug 577974.
> It also causes a bunch of warnings about it being hidden to fire too

I think you should be able to fix that with a |using nsCSSRule::GetStyleSheet;| or something in the header....  Might help the code that needed the cast too.
> Aren't they all virtual by virtue of the fact that they are pure virtual in
> nsICSSRule which is now Rule's base?

Ah, I see.  OK.
Comment on attachment 522563 [details] [diff] [review]
Part 1: Make nsCSSRule inherit from nsICSSRule and remove inheritance of nsICSSRule from other classes

r=me
Attachment #522563 - Flags: review?(bzbarsky) → review+
Comment on attachment 524124 [details] [diff] [review]
Part 3: Rename nsCSSRule and put in css namespace

r=me
Attachment #524124 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 27

7 years ago
Created attachment 524139 [details] [diff] [review]
Part 2: Remove forwarding methods to nsCSSRule from Rule classes
(Assignee)

Updated

7 years ago
Attachment #522567 - Attachment is obsolete: true
(Assignee)

Comment 28

7 years ago
Created attachment 524140 [details] [diff] [review]
Part 2: Remove forwarding methods to nsCSSRule from Rule classes
(Assignee)

Updated

7 years ago
Attachment #524139 - Attachment is obsolete: true
(Assignee)

Comment 29

7 years ago
Created attachment 524141 [details] [diff] [review]
Part 3: Rename nsCSSRule and put in css namespace
(Assignee)

Updated

7 years ago
Attachment #524124 - Attachment is obsolete: true
(Assignee)

Comment 30

7 years ago
Created attachment 524142 [details] [diff] [review]
Part 4: Move AddRef/Release back to nsCSSRule
(Assignee)

Updated

7 years ago
Attachment #524125 - Attachment is obsolete: true
(Assignee)

Comment 31

7 years ago
Created attachment 524143 [details] [diff] [review]
Part 5: Remove AddRef from GetStyleSheet
(Assignee)

Updated

7 years ago
Attachment #524126 - Attachment is obsolete: true
(Assignee)

Comment 32

7 years ago
I've added the using declaration which does fix the warning and removes the
need for the cast. I qualified it with a define from autoconf since comments in
configure suggest that this isn't supported by all compilers and I found other
places in the code that check the same define.

I suppose this means that the cast would still be needed by any compiler that
doesn't support using to resolve ambiguity so I'll upload another patch to
restore the cast that I removed before I thought of that.
(Assignee)

Comment 33

7 years ago
Created attachment 524144 [details] [diff] [review]
Part 6: Add cast to resolve ambiguity in case 'using' isn't supported by compiler
I believe we have at least one use of ambiguity-resolving 'using' in our tree not conditioned on that define already, so I'm not terribly worried about compilers that don't support that right now.  I'll land patches 1-5, and we can do 6 later if it becomes an issue...
(Assignee)

Comment 36

7 years ago
I failed to update nsICSSRule's IID in part 5. Is that something that needs to be fixed?
At this point, probably not...  ;)
You need to log in before you can comment on or make changes to this bug.