Closed Bug 610391 Opened 9 years ago Closed 9 years ago

Don't eagerly create widgets for combobox dropdowns

Categories

(Core :: Layout: Form Controls, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: bzbarsky, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

And in fact, we should create them when showing the dropdown, and tear them down when hiding the dropdown.

Ehsan, please feel free to steal!
Priority: P2 → P1
OS: Mac OS X → All
Hardware: x86 → All
You asked for it!  ;-)

But I'm not sure if I can actually work on this before 2.0...
Assignee: bzbarsky → ehsan
Keywords: perf
I think this is a post-2.0 project.
Whiteboard: [post-2.0]
Target Milestone: --- → Future
Blocks: 631049
Is this about the widget created in nsCSSFrameConstructor::InitializeSelectFrame?
Attached patch Patch (v1) (obsolete) — Splinter Review
This was much easier than I thought, which makes me think that I might have missed something!  :-)
Attachment #520103 - Flags: review?(roc)
I think it would be slightly cleaner to put widget creation in ShowList alongside the destruction code.
Attached patch Patch (v2) (obsolete) — Splinter Review
Done.
Attachment #520103 - Attachment is obsolete: true
Attachment #520257 - Flags: review?(roc)
Attachment #520103 - Flags: review?(roc)
Ehsan, in bug 631049 we are trying to prevent a widget ever being created for combobox dropdowns, for Fennec and Camino (which don't need such widgets). bz pointed me to your patch here, which removes the code changed in my patch there. Reading it now, I think the simplest thing would be to just change a single line in your patch and fix both bugs at once. Specifically, if

+  if (aShowList) {

were

+  if (aShowList && !nsComboboxControlFrame::ToolkitHasNativePopup()) {

then the widget will not be created if not needed (ToolkitHasNativePopup is true only on Fennec and Camino). What do you think?
(In reply to comment #8)
> Ehsan, in bug 631049 we are trying to prevent a widget ever being created for
> combobox dropdowns, for Fennec and Camino (which don't need such widgets). bz
> pointed me to your patch here, which removes the code changed in my patch
> there. Reading it now, I think the simplest thing would be to just change a
> single line in your patch and fix both bugs at once. Specifically, if
> 
> +  if (aShowList) {
> 
> were
> 
> +  if (aShowList && !nsComboboxControlFrame::ToolkitHasNativePopup()) {
> 
> then the widget will not be created if not needed (ToolkitHasNativePopup is
> true only on Fennec and Camino). What do you think?

I'm usually a fan of smaller bugs, so I'd rather do that in bug 631049.  I'm happy to own that bug too if you want me to (since I was the one who stepped on your toes there).  :-)
Ok, as discussed on IRC I'll make the patch in the other bug fit on top of this one.

As I was doing that I noticed that the patch here doesn't compile - nsComboboxControlFrame::ShowList uses view and viewManager which are not defined.
Attached patch Patch (v2)Splinter Review
I guess code being compiled is a prerequisite for what we accept on m-c.  This patch meets that criteria at least!  ;-)
Attachment #520257 - Attachment is obsolete: true
Attachment #520332 - Flags: review?(roc)
Attachment #520257 - Flags: review?(roc)
http://hg.mozilla.org/projects/birch/rev/165279f6a853
Whiteboard: [post-2.0] → fixed-in-birch
http://hg.mozilla.org/mozilla-central/rev/165279f6a853
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-birch
Target Milestone: Future → mozilla5
Depends on: 665540
Depends on: 682041
Depends on: 798230
You need to log in before you can comment on or make changes to this bug.