Move nsRadioVisitor and its children to nsRadioVisitor.{h,cpp} and remove radio visitor factories

RESOLVED FIXED in mozilla5

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

For the moment, only nsIRadioVisitor is declared in public/. Implementations (nsRadioVisitor and all other radio visitors) are declared and defined in nsHTMLInputElement. However, there are some factories declared in public/ used in nsHTMLInputElement.

So, if these factories are only needed for nsHTMLInputElement (it looks like they are) we should not declare them in public/ and define them in nsHTMLInputElement. We should not have factories considering the nsIRadioVisitor implementations are not visible outside nsHTMLInputElement.cpp.

Even better IMO, we can create nsRadioVisitor.{h,cpp} in content/html/content/src.
Depends on: 640978
Summary: Move or remove radio visitors factories → Move nsRadioVisitor and its children to nsRadioVisitor.{h,cpp} and remove radio visitor factories
Posted patch Patch v1Splinter Review
Man, it feels so good to clean that up...
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #518744 - Flags: review?(jonas)
Whiteboard: [needs review]
Comment on attachment 518744 [details] [diff] [review]
Patch v1

Is there a reason these are refcounted? Couldn't we just change nsIRadioVisitor to RadioVisitor and new/delete them, or even allocate them on the stack?

r=me either way. But if you do do the above please do it as a second patch on top of this one.
Attachment #518744 - Flags: review?(jonas) → review+
(In reply to comment #2)
> Comment on attachment 518744 [details] [diff] [review]
> Patch v1
> 
> Is there a reason these are refcounted? Couldn't we just change nsIRadioVisitor
> to RadioVisitor and new/delete them, or even allocate them on the stack?
> 
> r=me either way. But if you do do the above please do it as a second patch on
> top of this one.

FWIW, I'm thinking of removing ns{I,}RadioVisitor given two visitors might be removed by bug 641001 and the last one by bug 636774 (or one of its follow-up).

Anyway, to keep that in my radar, I've open bug 641349.
Whiteboard: [needs review] → [ready to land][waits for dependencies][passed try]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/55a070932e02
Flags: in-testsuite-
Whiteboard: [ready to land][waits for dependencies][passed try]
Target Milestone: --- → mozilla2.2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.