Closed Bug 641720 Opened 10 years ago Closed 10 years ago

Make nsHTMLInputElement::GetRadioGroupContainer saner

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #519299 - Flags: review?(bzbarsky)
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 [1]. This would require changing this patch and unfortunately, calling GetAttr() inside GetRadioGroupContainer.

Marking this bug depending on bug 639378 to reflect that.

[1] That's the behavior we have since 2.0 and we didn't have in 1.9.2.
Depends on: 639378
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
Attachment #519299 - Flags: review?(bzbarsky) → review+
Whiteboard: [needs review] → [update might be needed after bug 639378 resolution]
Blocks: 639490
Blocks: 639378
No longer depends on: 639378
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #519299 - Attachment is obsolete: true
Attachment #525174 - Flags: review?(bzbarsky)
Whiteboard: [update might be needed after bug 639378 resolution] → [needs review]
Attached patch Patch v1.2Splinter Review
Looks like I can't use HasName with all elements :(
Attachment #525174 - Attachment is obsolete: true
Attachment #525174 - Flags: review?(bzbarsky)
Attachment #525252 - Flags: review?(bzbarsky)
Comment on attachment 525252 [details] [diff] [review]
Patch v1.2

Jonas told me he could review that so, maybe... :)
Attachment #525252 - Flags: review?(jonas)
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.
Attachment #525252 - Flags: review?(jonas) → review+
Attached patch Inter diffSplinter Review
Applying review comments.
Attachment #525252 - Flags: review?(bzbarsky)
Whiteboard: [needs review] → [fixed in cedar]
http://hg.mozilla.org/mozilla-central/rev/3c5e31fe829c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.