610391 - Don't eagerly create widgets for combobox dropdowns

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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!

Comment 1

[(no longer active)]

You asked for it!  ;-)

But I'm not sure if I can actually work on this before 2.0...

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think this is a post-2.0 project.

Comment 3

[(no longer active)]

Is this about the widget created in nsCSSFrameConstructor::InitializeSelectFrame?

Comment 4

[Timothy Nikkel (:tnikkel)]

Yes.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

This was much easier than I thought, which makes me think that I might have missed something!  :-)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it would be slightly cleaner to put widget creation in ShowList alongside the destruction code.

Comment 7

[(no longer active)]

Attachment: Patch (v2)

Done.

Comment 8

[Alon Zakai (:azakai)]

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?

Comment 9

[(no longer active)]

(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).  :-)

Comment 10

[Alon Zakai (:azakai)]

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.

Comment 11

[(no longer active)]

Attachment: Patch (v2)

I guess code being compiled is a prerequisite for what we accept on m-c.  This patch meets that criteria at least!  ;-)

Comment 12

[(no longer active)]

http://hg.mozilla.org/projects/birch/rev/165279f6a853

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/165279f6a853