Created attachment 519299 [details] [diff] [review] Patch v1 For unknown reasons GetRadioGroupContainer is virtual and annoyingly returning an already_addRefed object. In addition, it is not doing the name attribute check which forces all consumers to do that. Cleaning this method helps simplifying some code.
Just a note about a discussion I just had with Jonas asking to keep our behavior making <input type='radio' name=''> not part of a group . This would require changing this patch and unfortunately, calling GetAttr() inside GetRadioGroupContainer. Marking this bug depending on bug 639378 to reflect that.  That's the behavior we have since 2.0 and we didn't have in 1.9.2.
Should I still be reviewing the attached patch?
(In reply to comment #2) > Should I still be reviewing the attached patch? If the change requested by Jonas is done, it will just change a few lines. Basically, changing the HasAttr() check in GetRadioGroupContainer to GetAttr() && !name.IsEmpty(). Maybe we could have the name returned as an out param. Whatever, I will write a patch on top of this one so I guess you can review this one and I will ask a review for the second one. You can also wait for the second part to be ready. As you wish.
And if the patch in bug 639378 is accepted, it sounds better to have this patch reviewed... I have the right to dream :)
Comment on attachment 519299 [details] [diff] [review] Patch v1 r=me
Created attachment 525174 [details] [diff] [review] Patch v1.1 I had to rebase this on my queue and I've changed HasAttr() check in GetRadioGroupContainer to HasName(). Unfortunately, an interdiff would be useless given that the rebase has affect nearly the entire patch :( The good point is I will be able to land this as soon as it's re-reviewed.
Created attachment 525252 [details] [diff] [review] Patch v1.2 Looks like I can't use HasName with all elements :(
Comment on attachment 525252 [details] [diff] [review] Patch v1.2 Jonas told me he could review that so, maybe... :)
Comment on attachment 525252 [details] [diff] [review] Patch v1.2 >+nsHTMLInputElement::GetRadioGroupContainer() const ... >+ nsIDocument* doc = GetCurrentDoc(); >+ nsCOMPtr<nsIRadioGroupContainer> group = do_QueryInterface(doc); >+ return group.get(); Eep! This is major evilness. This breaks XPCOM rules and will crash exploitably if nsDocument implements nsIRadioGroupContainer using a tearoff. You should never Release an interface and then continue using it. One possible solution would be to cast from nsIDocument to nsDocument and then to nsIRadioGroupContainer. >@@ -2274,39 +2280,38 @@ nsHTMLInputElement::PostHandleEvent(nsEv I take it the changes here are just indentation changes, apart from the removal of the GetNameIfExists check? It's always nice if you can attach a -w patch as well when there are large chunks that are just reindented. If you do change GetRadioGroupContainer to not use do_QI, then you could also change nsIRadioGroupContainer to nsRadioGroupContainer. But that might be better for a separate patch. r=me with those things fixed.