If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla2.0b2

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Craig Topper)

Tracking

Trunk
mozilla2.0b2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 4 obsolete attachments)

Because it's just ugly, and there's no real case where the method would fail.
Craig, want to take a stab at this?
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → craig.topper
(Assignee)

Comment 3

7 years ago
Created attachment 451229 [details] [diff] [review]
DeCOMtaminate GetType and the rest of the nsICSSRule interface

Went for bonus points and DeCOMtaminated the whole interface.
Attachment #451229 - Flags: review?(bzbarsky)
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|)
(Assignee)

Comment 8

7 years ago
Created attachment 451872 [details] [diff] [review]
Address some of the comments made in the bug so far
Attachment #451229 - Attachment is obsolete: true
Attachment #451872 - Flags: review?(bzbarsky)
Attachment #451229 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Attachment #451872 - Flags: review?(bzbarsky) → review?(dbaron)
(Assignee)

Updated

7 years ago
Blocks: 575900
(Assignee)

Updated

7 years ago
Blocks: 576831
(Assignee)

Updated

7 years ago
Attachment #451872 - Flags: review?(dbaron) → review?(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.
(Assignee)

Comment 10

7 years ago
> 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.
(Assignee)

Comment 11

7 years ago
> CSSStyleRuleImpl inherits from nsICSSStyleRule which inherits from nsICSSRule
> is declared. 

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

Comment 12

7 years ago
Created attachment 457482 [details] [diff] [review]
Fixed assertion in ReplaceStyleRule

Fixed the assertion. Will do 1 and 2 as follow on bugs/patches.
Attachment #451872 - Attachment is obsolete: true
Attachment #457482 - Flags: review?(bzbarsky)
Attachment #451872 - Flags: review?(bzbarsky)
Comment on attachment 457482 [details] [diff] [review]
Fixed assertion in ReplaceStyleRule

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

Comment 14

7 years ago
Created attachment 457504 [details] [diff] [review]
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.
Attachment #457482 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Your patch doesn't apply cleanly anymore.
Keywords: checkin-needed
(Assignee)

Comment 16

7 years ago
Created attachment 458152 [details] [diff] [review]
Updated to tip
Attachment #457504 - Attachment is obsolete: true

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/f7a3958eef07
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b2
Blocks: 488993
You need to log in before you can comment on or make changes to this bug.