Because it's just ugly, and there's no real case where the method would fail.

Craig, want to take a stab at this?

Sure. I actually have this buried inside another patch I think. I've been playing around with removing some pointless interfaces in the style rule area, but I was told to not make major changes to the style rule classes at this time due to some pending bugs. I'll isolate the changes relevant to this and submit a patch.

Went for bonus points and DeCOMtaminated the whole interface.

Comment on attachment 451229 [details] [diff] [review] DeCOMtaminate GetType and the rest of the nsICSSRule interface Obviously I'm not a reviewer here, but I wonder why your changing the IID of the other nsI* rather than just nsICSSRule

Because binary compatibility has changed for all interfaces derived from nsICSSRule due to the change in nsICSSRule.

Comment on attachment 451229 [details] [diff] [review] DeCOMtaminate GetType and the rest of the nsICSSRule interface >+++ b/layout/style/nsCSSParser.cpp > if (lastRule) { >- PRInt32 type; >- lastRule->GetType(type); >+ PRInt32 type = lastRule->GetType(); > switch (type) { Drive-By Nit: switch (lastRule->GetType()) { (Similar dropping of PRInt32 var in other hunks where type is only referenced once right after the assignment) >+++ b/layout/style/nsCSSRuleProcessor.cpp > CascadeEnumData* data = (CascadeEnumData*)aData; >- PRInt32 type = nsICSSRule::UNKNOWN_RULE; >- aRule->GetType(type); >+ PRInt32 type = aRule->GetType(); > > if (nsICSSRule::STYLE_RULE == type) { Pending bz's thoughts, this hunk might be even better to turn that if into a switch statement.

(In reply to comment #5) > Because binary compatibility has changed for all interfaces derived from > nsICSSRule due to the change in nsICSSRule. err yea, (how'd I miss: | : public nsICSSRule|)

Comment on attachment 451872 [details] [diff] [review] Address some of the comments made in the bug so far Just a few comments: 1) GetStyleSheet should just return nsIStyleSheet* and skip the addref. And maybe make whatever callers we can hold weak refs. Followup bug ok here. 2) It'd really be nice to make Clone() take an nsICSSRule** as a followup (but not in this bug; this patch is big enough). Or even just return nsICSSRule* if we can prove that all the possible errors it might hit are OOM that can't happen due to infallible new. 3) It's not quite clear to me why some of these methods that just directly call the superclass (e.g. CSSStyleRuleImpl::GetStyleSheet) exist at all. Do we have another GetStyleSheet defined on that class or something? 4) The change in nsCSSStyleSheet::ReplaceStyleRule is wrong. The second assert should be asserting about aOld, not aNew.

> 3) It's not quite clear to me why some of these methods that just directly > call > the superclass (e.g. CSSStyleRuleImpl::GetStyleSheet) exist at all. Do we > have another GetStyleSheet defined on that class or something? CSSStyleRuleImpl inherits from nsICSSStyleRule which inherits from nsICSSRule is declared. CSSStyleRuleImpl also inherits from nsCSSRule which as an implementation of GetStyleSheet. This means that at CSSStyleRuleImpl GetStyleSheet would be ambiguous so the method exists to forward to nsCSSRule. I have other bugs to clean up this mess.

> CSSStyleRuleImpl inherits from nsICSSStyleRule which inherits from nsICSSRule > is declared. I bothced that comment. That should have said "where GetStyleSheet is declared".

Fixed the assertion. Will do 1 and 2 as follow on bugs/patches.

Comment on attachment 457482 [details] [diff] [review] Fixed assertion in ReplaceStyleRule r=bzbarsky

Accidentally left an NS_IMETHODIMP instead of just nsresult. This would fail build on Windows.

Your patch doesn't apply cleanly anymore.