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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[(no longer active)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Seems to me like the right signature is:

  nsICSSRule* GetStyleRuleAt(PRInt32 aIndex);

without any of the random refcounting.

Comment 2

[(no longer active)]

Yes, that's probably even better!

Comment 3

[Saint Wesonga]

Attachment: Patch

Comment 4

[(no longer active)]

Comment on attachment 451139 [details] [diff] [review]
Patch

I landed a patch a few hours ago which bitrots this one (sorry about that!).

Comment 5

[Saint Wesonga]

noprob, although why doesn't the change show up in my updated mc working copy? i still have the old code.

Comment 6

[(no longer active)]

This is the code in question: <http://hg.mozilla.org/mozilla-central/rev/208d7f999037#l3.256>

Comment 7

[Saint Wesonga]

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

Comment 8

[(no longer active)]

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

Comment 9

[Tiziana Sellitto [:tiziana]]

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?

Comment 10

[(no longer active)]

Sure!

Comment 11

[Biru [:poiru]]

Attachment: Patch v1

Hi. I've attached a patch for this.

Comment 12

[(no longer active)]

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.

Comment 13

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

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

Comment 14

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

Removing the assertion is fine too.

Comment 15

[Biru [:poiru]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f030ce15fdb

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f030ce15fdb

Comment 18

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

And thanks for fixing this!