Closed Bug 391385 Opened 13 years ago Closed 11 months ago

Split PopupAutoComplete in three: UrlbarAutoComplete, SearchAutoComplete and ContentAutoComplete

Categories

(Firefox :: General, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: zeniko, Unassigned)

References

()

Details

Attachments

(1 file, 1 obsolete file)

 
Attached patch aus eins mach drei (obsolete) — Splinter Review
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #275801 - Flags: review?(gavin.sharp)
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.
Attachment #275801 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Target Milestone: --- → Firefox 3 M8
Reconciling the bit-rot from bug 385275 seems like a little too much for a random checkin-needed passerby.
Keywords: checkin-needed
Attachment #275801 - Attachment is obsolete: true
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Why not give autocomplete textboxes their own popup as part of the XBL?

... like the way XPFE autocomplete textboxes do ;-)
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 :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
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.)
No low-hanging fruit, then.
Assignee: zeniko → nobody
Status: REOPENED → NEW
Target Milestone: Firefox 3 M8 → ---
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.
(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.

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

Status: NEW → RESOLVED
Closed: 13 years ago11 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.