Closed
Bug 775049
Opened 12 years ago
Closed 12 years ago
DeCOMtaminate nsIRadioGroupContainer.
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: Ms2ger, Assigned: Luqman)
Details
(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])
Attachments
(1 file, 2 obsolete files)
19.38 KB,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Attachment #643895 -
Flags: review?(Ms2ger)
Reporter | ||
Comment 2•12 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•12 years ago
|
||
Thanks for the feedback.
Attachment #643895 -
Attachment is obsolete: true
Attachment #643915 -
Flags: review?(mounir)
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Confirmed to build against m-c.
Attachment #643915 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d4f603c78754
Comment 7•12 years ago
|
||
(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•12 years ago
|
Assignee: nobody → laden
Reporter | ||
Comment 8•12 years ago
|
||
I pushed again just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=c3c74331df99
Updated•12 years ago
|
Keywords: checkin-needed
Comment 9•12 years ago
|
||
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•12 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
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/03a0ad94d77a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•