Last Comment Bug 571635 - Make nsCSSStyleSheet::GetStyleRuleAt return a css::Rule*
: Make nsCSSStyleSheet::GetStyleRuleAt return a css::Rule*
Status: RESOLVED FIXED
[good first bug][mentor=ehsan]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Birunthan Mohanathas [:poiru]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-11 17:46 PDT by :Ehsan Akhgari
Modified: 2013-07-12 05:56 PDT (History)
4 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.90 KB, patch)
2010-06-14 15:11 PDT, Saint Wesonga
no flags Details | Diff | Splinter Review
Patch v1 (5.54 KB, patch)
2013-06-29 09:26 PDT, Birunthan Mohanathas [:poiru]
dbaron: review+
Details | Diff | Splinter Review
Patch v2 (5.62 KB, patch)
2013-07-04 19:38 PDT, Birunthan Mohanathas [:poiru]
birunthan: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2010-06-11 17:46:52 PDT
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 [:bz] 2010-06-11 19:20:43 PDT
Seems to me like the right signature is:

  nsICSSRule* GetStyleRuleAt(PRInt32 aIndex);

without any of the random refcounting.
Comment 2 :Ehsan Akhgari 2010-06-11 19:29:34 PDT
Yes, that's probably even better!
Comment 3 Saint Wesonga 2010-06-14 15:11:39 PDT
Created attachment 451139 [details] [diff] [review]
Patch
Comment 4 :Ehsan Akhgari 2010-06-14 15:19:53 PDT
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 2010-06-14 15:31:46 PDT
noprob, although why doesn't the change show up in my updated mc working copy? i still have the old code.
Comment 6 :Ehsan Akhgari 2010-06-14 16:25:30 PDT
This is the code in question: <http://hg.mozilla.org/mozilla-central/rev/208d7f999037#l3.256>
Comment 7 Saint Wesonga 2010-07-12 12:52:16 PDT
(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 :Ehsan Akhgari 2010-07-12 14:52:12 PDT
(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] 2013-06-24 07:46:57 PDT
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 :Ehsan Akhgari 2013-06-24 07:56:38 PDT
Sure!
Comment 11 Birunthan Mohanathas [:poiru] 2013-06-29 09:26:02 PDT
Created attachment 769391 [details] [diff] [review]
Patch v1

Hi. I've attached a patch for this.
Comment 12 :Ehsan Akhgari 2013-07-02 11:45:56 PDT
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-7 (review requests must explain patch) 2013-07-02 14:05:20 PDT
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-7 (review requests must explain patch) 2013-07-04 13:03:38 PDT
Removing the assertion is fine too.
Comment 15 Birunthan Mohanathas [:poiru] 2013-07-04 19:38:32 PDT
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.
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-07-07 13:28:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f030ce15fdb
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-07-07 18:31:42 PDT
https://hg.mozilla.org/mozilla-central/rev/1f030ce15fdb
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-10 14:19:55 PDT
And thanks for fixing this!

Note You need to log in before you can comment on or make changes to this bug.