391385 - Split PopupAutoComplete in three: UrlbarAutoComplete, SearchAutoComplete and ContentAutoComplete

Status: RESOLVED WORKSFORME

(Firefox :: General, defect)

Description

[Simon Bünzli]

Comment 1

[Simon Bünzli]

Attachment: aus eins mach drei

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 275801 [details] [diff] [review]
aus eins mach drei

I was concerned about losing the RTL rules for the other panels, but Mano tells me that the patch for bug 350611 was only meant to apply to the location bar.

Comment 3

[Phil Ringnalda (:philor)]

Reconciling the bit-rot from bug 385275 seems like a little too much for a random checkin-needed passerby.

Comment 4

[Simon Bünzli]

Attachment: for check-in (unbitrotted)

Comment 5

[Phil Ringnalda (:philor)]

Thanks.

browser/base/content/browser.css 1.35
browser/base/content/browser.xul 1.360
browser/base/content/urlbarBindings.xml 1.18
browser/components/search/content/search.xml 1.96
browser/themes/winstripe/browser/browser.css 1.80

Comment 6

[[e-mail]]

Why not give autocomplete textboxes their own popup as part of the XBL?

... like the way XPFE autocomplete textboxes do ;-)

Comment 7

[Phil Ringnalda (:philor)]

Backed out for perf regressions:

           Ts-pre   Ts-post  Txul-pre Txul-post
Linux        989     1000     285       296
Windows    ~1995    ~2015    ~570      ~600

Mac's a little hard to tell, but it probably wasn't much affected, maybe Txul.

Sorry :(

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> Why not give autocomplete textboxes their own popup as part of the XBL?
> 
> ... like the way XPFE autocomplete textboxes do ;-)

Yes, that would make sense. I can't think of any reasons why users of the autocomplete binding would need to specify their own popup. File a new bug?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

The fact that this caused a performance regression really sucks. Changing 1 popup into 3 shouldn't cause a ~1% Ts hit :( I guess we'll probably have to go with the patch in bug 361735, unless we can figure out why this patch would cause this kind of performance regression. Maybe Neil Deakin has some ideas?

I wonder if perhaps we could have the panel be display:none until it's needed? That would just be moving the problem, sure, but it'd get it out of the "startup" critical path.

(Note that giving autocomplete textboxes their own popup as the other Neil suggests is likely to cause the same perf regression, so we'll have to watch out for that, I guess.)

Comment 10

[Simon Bünzli]

No low-hanging fruit, then.

Comment 11

[Neil Deakin]

If just having the panels in the document causes the regression, then it is likely because of the native widgets that need to be created.

For bug 279703, I create the widgets lazily for menus, perhaps we should just do this for certain other types of panels as well. This would involve adjusting the call to CreateWidgetForView in nsMenuPopupFrame::Init.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> For bug 279703, I create the widgets lazily for menus, perhaps we should just
> do this for certain other types of panels as well.

I filed bug 394887 for looking into this potential improvement.

Comment 13

[Mark Banner (:standard8)]

I believe this happened somewhere over the years. Urlbar & Search are now separate, and content popup is handled differently.