775049 - DeCOMtaminate nsIRadioGroupContainer.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]]

Attachment: DeCOMtaminate nsIRadioGroupContainer.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]]

Attachment: DeCOMtaminate nsIRadioGroupContainer.

Thanks for the feedback.

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 643915 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.

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

r=me

Comment 5

[Luqman Aden [:Luqman]]

Attachment: DeCOMtaminate nsIRadioGroupContainer.

Confirmed to build against m-c.

Comment 6

[Luqman Aden [:Luqman]]

https://tbpl.mozilla.org/?tree=Try&rev=d4f603c78754

Comment 7

[Till Schneidereit [:till]]

(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 (he/him; ⌚ UTC+1/+2)]

I pushed again just to be sure:

https://tbpl.mozilla.org/?tree=Try&rev=c3c74331df99

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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 (he/him; ⌚ UTC+1/+2)]

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]]

https://hg.mozilla.org/mozilla-central/rev/03a0ad94d77a