Don't eagerly create widgets for combobox dropdowns

RESOLVED FIXED in mozilla5

Status

()

Core
Layout: Form Controls
P1
normal
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: bz, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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!
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

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

Updated

8 years ago
Whiteboard: [post-2.0]

Updated

8 years ago
Target Milestone: --- → Future

Updated

7 years ago
Blocks: 631049
(Assignee)

Comment 3

7 years ago
Is this about the widget created in nsCSSFrameConstructor::InitializeSelectFrame?
Yes.
(Assignee)

Comment 5

7 years ago
Created attachment 520103 [details] [diff] [review]
Patch (v1)

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

Comment 7

7 years ago
Created attachment 520257 [details] [diff] [review]
Patch (v2)

Done.
Attachment #520103 - Attachment is obsolete: true
Attachment #520257 - Flags: review?(roc)
Attachment #520103 - Flags: review?(roc)

Comment 8

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

Comment 9

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

Comment 11

7 years ago
Created attachment 520332 [details] [diff] [review]
Patch (v2)

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

Comment 12

7 years ago
http://hg.mozilla.org/projects/birch/rev/165279f6a853
Whiteboard: [post-2.0] → fixed-in-birch
(Assignee)

Comment 13

7 years ago
http://hg.mozilla.org/mozilla-central/rev/165279f6a853
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-birch
(Assignee)

Updated

7 years ago
Target Milestone: Future → mozilla5

Updated

7 years ago
Depends on: 665540

Updated

7 years ago
Depends on: 682041

Updated

6 years ago
Depends on: 798230
You need to log in before you can comment on or make changes to this bug.