Make nsHTMLInputElement::GetRadioGroupContainer saner

RESOLVED FIXED in mozilla6

Status

()

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

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla6
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
Attachment #519299 - Flags: review?(bzbarsky)
(Assignee)

Comment 1

6 years ago
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?
(Assignee)

Comment 3

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

Comment 4

6 years ago
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+
(Assignee)

Updated

6 years ago
Whiteboard: [needs review] → [update might be needed after bug 639378 resolution]
(Assignee)

Updated

6 years ago
Blocks: 639490
(Assignee)

Updated

6 years ago
Blocks: 639378
No longer depends on: 639378
(Assignee)

Comment 6

6 years ago
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.
Attachment #519299 - Attachment is obsolete: true
Attachment #525174 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Whiteboard: [update might be needed after bug 639378 resolution] → [needs review]
(Assignee)

Comment 7

6 years ago
Created attachment 525252 [details] [diff] [review]
Patch v1.2

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)
(Assignee)

Comment 8

6 years ago
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+
(Assignee)

Comment 10

6 years ago
Created attachment 529975 [details] [diff] [review]
Inter diff

Applying review comments.
(Assignee)

Updated

6 years ago
Attachment #525252 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review] → [fixed in cedar]
http://hg.mozilla.org/mozilla-central/rev/3c5e31fe829c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.