DeCOMtaminate nsIRadioGroupContainer.

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Luqman)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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>.
(Assignee)

Comment 1

5 years ago
Created attachment 643895 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.
Attachment #643895 - Flags: review?(Ms2ger)
(Reporter)

Comment 2

5 years ago
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.
Attachment #643895 - Flags: review?(Ms2ger) → feedback+
(Assignee)

Comment 3

5 years ago
Created attachment 643915 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.

Thanks for the feedback.
Attachment #643895 - Attachment is obsolete: true
Attachment #643915 - Flags: review?(mounir)
Comment on attachment 643915 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.

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

r=me
Attachment #643915 - Flags: review?(mounir) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 644732 [details] [diff] [review]
DeCOMtaminate nsIRadioGroupContainer.

Confirmed to build against m-c.
Attachment #643915 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d4f603c78754
(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?
(Reporter)

Updated

5 years ago
Assignee: nobody → laden
(Reporter)

Comment 8

5 years ago
I pushed again just to be sure:

https://tbpl.mozilla.org/?tree=Try&rev=c3c74331df99
Keywords: checkin-needed
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
Keywords: checkin-needed
(Reporter)

Comment 10

5 years ago
Thanks for the patch, and sorry we lost track of it for a while!

https://hg.mozilla.org/integration/mozilla-inbound/rev/03a0ad94d77a
Status: NEW → ASSIGNED
Flags: in-testsuite-
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/03a0ad94d77a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.