Cleanup multiple inheritance of nsICSSRule and nsCSSRule

RESOLVED FIXED in mozilla5

Status

()

defect
--
minor
RESOLVED FIXED
9 years ago
6 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
bzbarsky
: review+
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

9 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

9 years ago
Depends on: 576831, 576877, 577002, 577974
Assignee

Comment 2

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

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 7

8 years ago
Assignee

Comment 8

8 years ago
Assignee

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 9

8 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

8 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

8 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

Updated

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

Comment 19

8 years ago
Assignee

Updated

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

Comment 20

8 years ago
Assignee

Updated

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

Comment 21

8 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

8 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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 32

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

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