One-off buttons should be cached the first time

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks 1 bug, {regression})

Trunk
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 disabled, firefox56 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(1 attachment)

Assignee

Description

2 years ago
In bug 1312999 I added code to cache the one-off buttons and rebuild them only if the textbox size changed.

Unfortunately, the current caller code sets the popup before the textbox. Setting popup causes a _rebuild call, which can't cache the textbox size as the textbox setter hasn't been called yet. This causes the reflows in bug 1357054 to happen both the first and second time the awesomebar or searchbar panel is opened, instead of only the first time.
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-performance]
Assignee

Comment 1

2 years ago
Posted patch PatchSplinter Review
This wasn't a problem before bug 1369705 as both the popup and textbox setters were called before we had any panel open.
Attachment #8889459 - Flags: review?(mak77)
Assignee

Updated

2 years ago
Keywords: regression
Comment on attachment 8889459 [details] [diff] [review]
Patch

Review of attachment 8889459 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/urlbarBindings.xml
@@ +1684,5 @@
>            this._oneOffSearchesEnabled = enable;
>            if (enable) {
>              this.oneOffSearchButtons.telemetryOrigin = "urlbar";
>              this.oneOffSearchButtons.style.display = "-moz-box";
> +            // The popup setter will cause a _rebuild call, which uses .textbox

Maybe I'd rephrase both comments as
// Set .textbox first, since the popup setter will cause
// a _rebuild call that uses it.
otherwise it's a bit unclear what the comment tries to underline.
Attachment #8889459 - Flags: review?(mak77) → review+

Comment 3

2 years ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a2c24475fa
One-off buttons should be cached the first time, r=mak.
Iteration: --- → 56.4 - Aug 1

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91a2c24475fa
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.