571946 - nsICSSRule::GetType should just return the type as a PRInt32 type, instead of taking an outparam for that

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[(no longer active)]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Craig, want to take a stab at this?

Comment 2

[Craig Topper]

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.

Comment 3

[Craig Topper]

Attachment: DeCOMtaminate GetType and the rest of the nsICSSRule interface

Went for bonus points and DeCOMtaminated the whole interface.

Comment 4

[Justin Wood (:Callek)]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[Justin Wood (:Callek)]

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.

Comment 7

[Justin Wood (:Callek)]

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

[Craig Topper]

Attachment: Address some of the comments made in the bug so far

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Craig Topper]

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

Comment 11

[Craig Topper]

> CSSStyleRuleImpl inherits from nsICSSStyleRule which inherits from nsICSSRule
> is declared. 

I bothced that comment. That should have said "where GetStyleSheet is declared".

Comment 12

[Craig Topper]

Attachment: Fixed assertion in ReplaceStyleRule

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 14

[Craig Topper]

Attachment: Fixed return type on one of the implementations of Clone

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

Comment 15

[Dão Gottwald [:dao]]

Your patch doesn't apply cleanly anymore.

Comment 16

[Craig Topper]

Attachment: Updated to tip

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f7a3958eef07