Last Comment Bug 775049 - DeCOMtaminate nsIRadioGroupContainer.
: DeCOMtaminate nsIRadioGroupContainer.
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger][lang=...
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Luqman Aden [:Luqman]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-18 04:43 PDT by :Ms2ger
Modified: 2012-08-06 07:42 PDT (History)
5 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
DeCOMtaminate nsIRadioGroupContainer. (15.07 KB, patch)
2012-07-19 09:44 PDT, Luqman Aden [:Luqman]
Ms2ger: feedback+
Details | Diff | Review
DeCOMtaminate nsIRadioGroupContainer. (16.84 KB, patch)
2012-07-19 10:36 PDT, Luqman Aden [:Luqman]
mounir: review+
Details | Diff | Review
DeCOMtaminate nsIRadioGroupContainer. (19.38 KB, patch)
2012-07-22 00:42 PDT, Luqman Aden [:Luqman]
no flags Details | Diff | Review

Description :Ms2ger 2012-07-18 04:43:39 PDT
At least {Get,Set}CurrentRadioButton and {AddTo,RemoveFrom}RadioGroup only ever return NS_OK. Those, and any others that are in the same situation, should return their result directly (or return void if there is no result).

See <http://mxr.mozilla.org/mozilla-central/source/content/html/content/public/nsIRadioGroupContainer.h>.
Comment 1 Luqman Aden [:Luqman] 2012-07-19 09:44:35 PDT
Created attachment 643895 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.
Comment 2 :Ms2ger 2012-07-19 10:04:48 PDT
Comment on attachment 643895 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.

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

Two substantial issues from our convoluted ways of refcounting... Please fix those and ask ':mounir' to review.

::: content/base/src/nsDocument.cpp
@@ +6425,3 @@
>  {
>    nsRadioGroupStruct* radioGroup = GetRadioGroup(aName);
>    NS_ENSURE_TRUE(radioGroup, NS_OK);

Remove this line or change the NS_OK to nsnull.

@@ +6428,5 @@
>  
> +  nsIDOMHTMLInputElement* aRadio = radioGroup->mSelectedRadioButton;
> +  NS_IF_ADDREF(aRadio);
> +
> +  return aRadio;

Just 'return radioGroup->mSelectedRadioButton;'. The addref here would cause a leak if you return the pointer directly rather than through an out-parameter.

::: content/html/content/public/nsIRadioGroupContainer.h
@@ -46,5 @@
>  
>    /**
>     * Get the current radio button in a group
>     * @param aName the group name
> -   * @param aRadio the currently selected radio button [OUT]

Make this @return instead of removing the line?

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +1871,2 @@
>  
> +  return aRadio;

This will leak too. Instead, you can use 'return mSelectedRadioButtons.GetWeak(aName);'.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1716,4 @@
>    }
>  
>    // Let the group know that we are now the One True Radio Button
>    nsresult rv = NS_OK;

You can remove this

@@ +1728,5 @@
>    // validity state. We have to be sure the radio group container knows
>    // the currently selected radio.
>    if (NS_SUCCEEDED(rv)) {
>      SetCheckedInternal(true, aNotify);
>    }

And make the SetCheckedInternal unconditional, and either just return NS_OK from this function, or make it return void as well.
Comment 3 Luqman Aden [:Luqman] 2012-07-19 10:36:10 PDT
Created attachment 643915 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.

Thanks for the feedback.
Comment 4 Mounir Lamouri (:mounir) 2012-07-20 23:39:41 PDT
Comment on attachment 643915 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.

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

r=me
Comment 5 Luqman Aden [:Luqman] 2012-07-22 00:42:17 PDT
Created attachment 644732 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.

Confirmed to build against m-c.
Comment 6 Luqman Aden [:Luqman] 2012-07-22 01:46:16 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d4f603c78754
Comment 7 Till Schneidereit [:till] 2012-08-03 03:32:55 PDT
(In reply to Luqman A. from comment #6)
> https://tbpl.mozilla.org/?tree=Try&rev=d4f603c78754

Was the try run green? If so, the patch should be ready to land. Can you ask on IRC or set the checkin-needed keyword?
Comment 8 :Ms2ger 2012-08-03 03:40:08 PDT
I pushed again just to be sure:

https://tbpl.mozilla.org/?tree=Try&rev=c3c74331df99
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-05 17:11:39 PDT
Sorry for the hassle, but can you please attach a patch with all the necessary commit information? Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Comment 10 :Ms2ger 2012-08-06 00:26:27 PDT
Thanks for the patch, and sorry we lost track of it for a while!

https://hg.mozilla.org/integration/mozilla-inbound/rev/03a0ad94d77a
Comment 11 Ed Morley [:emorley] 2012-08-06 07:42:13 PDT
https://hg.mozilla.org/mozilla-central/rev/03a0ad94d77a

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