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.

Updated due to patches in other bugs

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.

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?

Comment on attachment 522594 [details] [diff] [review] Part 4: Move AddRef/Release back to nsCSSRule r=me

Comment on attachment 522595 [details] [diff] [review] Part 5: Remove AddRef from GetStyleSheet r=me

(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;

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

Comment on attachment 524124 [details] [diff] [review] Part 3: Rename nsCSSRule and put in css namespace Fixed to rename the file properly

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

Comment on attachment 524124 [details] [diff] [review] Part 3: Rename nsCSSRule and put in css namespace r=me

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

Turns out Windows needs part 6. Pushed: http://hg.mozilla.org/mozilla-central/rev/592aacaf042d http://hg.mozilla.org/mozilla-central/rev/f2685fc02b6b http://hg.mozilla.org/mozilla-central/rev/7e119e22b856 http://hg.mozilla.org/mozilla-central/rev/5d537abf7005 http://hg.mozilla.org/mozilla-central/rev/5235a5391635 http://hg.mozilla.org/mozilla-central/rev/b70744835d94

I failed to update nsICSSRule's IID in part 5. Is that something that needs to be fixed?

At this point, probably not... ;)