Closed Bug 1387783 Opened 3 years ago Closed 3 years ago

onZoomChange delazifies all browsers at once

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: perf)

Attachments

(2 files)

from mozillazine:

> [bug 1345098] Lazy browser prematurely inserted via 'fullZoom' property access:
> getter@chrome://browser/content/tabbrowser.xml:2293:45
> ZoomManager_getZoomForBrowser@chrome://global/content/viewZoomOverlay.js:45:1
> getZoomLevel@chrome://browser/content/ext-tabs.js:687:20
> getAPI/self.tabs.onZoomChange<@chrome://browser/content/ext-tabs.js:698:39
> addListener@resource://gre/modules/ExtensionCommon.jsm:1441:42
> addListener@resource://gre/modules/ExtensionCommon.jsm:1479:50
> addListener@resource://gre/modules/ExtensionParent.jsm:760:35
> async*receiveMessage@resource://gre/modules/ExtensionParent.jsm:606:11
> tabbrowser.xml:2293:23
> 
> Several thousands of errors like that

http://searchfox.org/mozilla-central/rev/39ccebaf1890f8731d534b5eb3818b66d2b25221/browser/components/extensions/ext-tabs.js#694-700
When accessing fullZoom on a lazy browser, the browser gets inserted and then fullZoom always returns 1 regardless because the browser doesn't have anything loaded yet. So we might as well add a shim returning 1 without inserting the browser.
Component: WebExtensions: General → Tabbed Browser
Product: Toolkit → Firefox
Attached patch patchSplinter Review
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #8895037 - Flags: review?(kevinhowjones)
Comment on attachment 8895037 [details] [diff] [review]
patch

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

LGTM.  Interesting syntax, thanks for cleaning everything up.
Attachment #8895037 - Flags: review?(kevinhowjones) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbc4ded4bae
Add lazy browser shims for fullZoom and textZoom. r=kevinjones
https://hg.mozilla.org/mozilla-central/rev/9bbc4ded4bae
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Attached patch patch for upliftSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: bug 906076
[User impact if declined]: bad performance with the onZoomChange webextension API
[Is this code covered by automated tests?]: not sure if there are webextension tests for this
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: hardly
[Why is the change risky/not risky?]: small, isolated and straightforward fix
[String changes made/needed]: /
Attachment #8895711 - Flags: approval-mozilla-beta?
Comment on attachment 8895711 [details] [diff] [review]
patch for uplift

Fix for a perf issue with web extensions, let's uplift for beta 3.
Attachment #8895711 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Someone please add "perf" key word. Thanks.
You need to log in before you can comment on or make changes to this bug.