Make nsCSSStyleSheet::GetStyleRuleAt return a css::Rule*

RESOLVED FIXED in mozilla25

Status

()

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

People

(Reporter: Ehsan, Assigned: poiru)

Tracking

Trunk
mozilla25
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=ehsan])

Attachments

(1 attachment, 2 obsolete attachments)

This really hurt my eyes, and my brains.

nsCSSStyleSheet::GetStyleRuleAt takes an nsICSSRule*&, which leads to code such as:

nsCOMPtr<nsICSSRule> bar;
stylesheet->GetStyleRuleAt(foo, *getter_AddRefs(bar));

I think pretty much every body would write this instead, assuming that this method is returning the pointer without addrefing,

nsICSSRule *bar;
stylesheet->GetStyleRuleAt(foo, bar);

... and tracking the leak is not the most pleasant thing to do.
Whiteboard: [good first bug]
Seems to me like the right signature is:

  nsICSSRule* GetStyleRuleAt(PRInt32 aIndex);

without any of the random refcounting.
Yes, that's probably even better!

Comment 3

7 years ago
Created attachment 451139 [details] [diff] [review]
Patch
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #451139 - Flags: review?(bzbarsky)
Comment on attachment 451139 [details] [diff] [review]
Patch

I landed a patch a few hours ago which bitrots this one (sorry about that!).
Attachment #451139 - Attachment is obsolete: true
Attachment #451139 - Flags: review?(bzbarsky)

Comment 5

7 years ago
noprob, although why doesn't the change show up in my updated mc working copy? i still have the old code.
This is the code in question: <http://hg.mozilla.org/mozilla-central/rev/208d7f999037#l3.256>

Comment 7

7 years ago
(In reply to comment #4)
> I landed a patch a few hours ago which bitrots this one (sorry about that!).

Does this mean that the bug should be resolved as invalid/wontfix or that a new patch is needed?
(In reply to comment #7)
> (In reply to comment #4)
> > I landed a patch a few hours ago which bitrots this one (sorry about that!).
> 
> Does this mean that the bug should be resolved as invalid/wontfix or that a new
> patch is needed?

A new patch is needed.  Once you have a new patch, please attach it to the bug and request the review flag from :bz again.

Thanks!
Saint Wesonga, are you still working on this bug? 
I'm looking at old "good first bugs" not touched for a while. 
Ehsan, do you think this bug could become a mentored bug? Would you like to apply as mentor?
Flags: needinfo?(wesongathedeveloper)
Sure!
Whiteboard: [good first bug] → [good first bug][mentor=ehsan]

Updated

4 years ago
Assignee: wesongathedeveloper → nobody
Flags: needinfo?(wesongathedeveloper)
Status: ASSIGNED → NEW
(Assignee)

Comment 11

4 years ago
Created attachment 769391 [details] [diff] [review]
Patch v1

Hi. I've attached a patch for this.
Attachment #769391 - Flags: review?(ehsan)
Comment on attachment 769391 [details] [diff] [review]
Patch v1

Review of attachment 769391 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but I think David should take a look at this too.
Attachment #769391 - Flags: review?(ehsan) → review?(dbaron)
Comment on attachment 769391 [details] [diff] [review]
Patch v1

>diff --git a/content/base/src/nsTreeSanitizer.cpp b/content/base/src/nsTreeSanitizer.cpp
>--- a/content/base/src/nsTreeSanitizer.cpp
>+++ b/content/base/src/nsTreeSanitizer.cpp
>@@ -1106,19 +1106,18 @@ nsTreeSanitizer::SanitizeStyleSheet(cons
>   NS_ENSURE_SUCCESS(rv, true);
>   // Mark the sheet as complete.
>   NS_ABORT_IF_FALSE(!sheet->IsModified(),
>       "should not get marked modified during parsing");
>   sheet->SetComplete();
>   // Loop through all the rules found in the CSS text
>   int32_t ruleCount = sheet->StyleRuleCount();
>   for (int32_t i = 0; i < ruleCount; ++i) {
>-    nsRefPtr<mozilla::css::Rule> rule;
>-    rv = sheet->GetStyleRuleAt(i, *getter_AddRefs(rule));
>-    if (NS_FAILED(rv))
>+    nsRefPtr<mozilla::css::Rule> rule = sheet->GetStyleRuleAt(i);
>+    if (!rule)
>       continue; NS_ASSERTION(rule, "We should have a rule by now");


This should just be mozilla::css::Rule* rather than using nsRefPtr.

And while you're there, could you also fix that NS_ASSERTION being on the same line?



r=dbaron with that fixed
Attachment #769391 - Flags: review?(dbaron) → review+
Removing the assertion is fine too.
(Assignee)

Comment 15

4 years ago
Created attachment 771528 [details] [diff] [review]
Patch v2

(In reply to David Baron [:dbaron])
> This should just be mozilla::css::Rule* rather than using nsRefPtr.
> Removing the assertion is fine too.

Done.
Assignee: nobody → birunthan
Attachment #769391 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #771528 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f030ce15fdb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1f030ce15fdb
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
And thanks for fixing this!

Updated

4 years ago
Flags: in-testsuite-
OS: Mac OS X → All
Hardware: x86 → All
Summary: Make nsCSSStyleSheet::GetStyleRuleAt take an nsICSSRule** parameter → Make nsCSSStyleSheet::GetStyleRuleAt return a css::Rule*
You need to log in before you can comment on or make changes to this bug.