Last Comment Bug 641720 - Make nsHTMLInputElement::GetRadioGroupContainer saner
: Make nsHTMLInputElement::GetRadioGroupContainer saner
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 639378 639490
  Show dependency treegraph
 
Reported: 2011-03-14 18:29 PDT by Mounir Lamouri (:mounir)
Modified: 2011-05-04 13:50 PDT (History)
2 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (17.45 KB, patch)
2011-03-14 18:29 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
Details | Diff | Splinter Review
Patch v1.1 (16.91 KB, patch)
2011-04-11 14:37 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.2 (16.99 KB, patch)
2011-04-11 18:13 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Inter diff (1.25 KB, patch)
2011-05-04 05:41 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-03-14 18:29:45 PDT
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.
Comment 1 Mounir Lamouri (:mounir) 2011-03-14 18:33:22 PDT
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.
Comment 2 Boris Zbarsky [:bz] 2011-03-15 08:03:04 PDT
Should I still be reviewing the attached patch?
Comment 3 Mounir Lamouri (:mounir) 2011-03-15 08:12:35 PDT
(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.
Comment 4 Mounir Lamouri (:mounir) 2011-03-15 08:14:41 PDT
And if the patch in bug 639378 is accepted, it sounds better to have this patch reviewed... I have the right to dream :)
Comment 5 Boris Zbarsky [:bz] 2011-03-15 12:14:47 PDT
Comment on attachment 519299 [details] [diff] [review]
Patch v1

r=me
Comment 6 Mounir Lamouri (:mounir) 2011-04-11 14:37:05 PDT
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.
Comment 7 Mounir Lamouri (:mounir) 2011-04-11 18:13:17 PDT
Created attachment 525252 [details] [diff] [review]
Patch v1.2

Looks like I can't use HasName with all elements :(
Comment 8 Mounir Lamouri (:mounir) 2011-04-11 18:15:11 PDT
Comment on attachment 525252 [details] [diff] [review]
Patch v1.2

Jonas told me he could review that so, maybe... :)
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2011-04-22 17:41:03 PDT
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.
Comment 10 Mounir Lamouri (:mounir) 2011-05-04 05:41:06 PDT
Created attachment 529975 [details] [diff] [review]
Inter diff

Applying review comments.
Comment 11 Boris Zbarsky [:bz] 2011-05-04 13:50:30 PDT
http://hg.mozilla.org/mozilla-central/rev/3c5e31fe829c

Note You need to log in before you can comment on or make changes to this bug.