Remove the dependency on FullZoom_onLocationChange from the zoom controls

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

unspecified
Firefox 28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M8])

Attachments

(1 attachment)

In my ts_paint profile, http://tests.themasta.com/cleopatra/?report=501ee08ac2a023cc2c03f0bc8436cc298d9c126f, I noticed that FullZoom_onLocationChange takes up 322ms out of 25 windows, or about 12.88ms per window.

This event listener gets hit every time a tab is changed, so this would also affect tpaint too.

I want to look in to removing this usage, and see if we can get the same results just querying for the zoom amount when the panel opens. We may need to keep this listener if the controls are in the navigation toolbar, but that is not the default setup.
(In reply to Jared Wein [:jaws] from comment #0)
> We may need
> to keep this listener if the controls are in the navigation toolbar, but
> that is not the default setup.

The controls don't display the current zoom value when placed on the toolbar.
Yeah, I missed this part, so it is even easier.

However onBuild isn't called until the panel is opened, so this patch shouldn't get us anything on tpaint or ts_paint.
No longer blocks: 880611, 889758
Created attachment 787604 [details] [diff] [review]
Patch

 0:43.54 INFO TEST-START | Shutdown
 0:43.54 Browser Chrome Test Summary
 0:43.54        Passed: 354
 0:43.54        Failed: 0
 0:43.54        Todo: 1
Attachment #787604 - Flags: review?(mdeboer)
Blocks: 902024
Whiteboard: [Australis:M?]
Seems quite related to the discussion in bug 897410!
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> Seems quite related to the discussion in bug 897410!

I know! :) I couldn't figure out where to upload this patch and so I went with filing a new bug.
This likely won't affect TART either since the panel wouldn't have been opened and thus onBuild wouldn't have been called in that case either.
No longer blocks: 902024
Comment on attachment 787604 [details] [diff] [review]
Patch

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

This is way better than the my naive implementation ;) Thanks!

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +275,5 @@
>      removable: true,
>      defaultArea: CustomizableUI.AREA_PANEL,
>      allowedAreas: [CustomizableUI.AREA_PANEL, CustomizableUI.AREA_NAVBAR],
>      onBuild: function(aDocument) {
> +      const panelId = "PanelUI-popup";

nit: please name this as a constant, like 'kPanelId'

@@ +392,5 @@
>  
> +          if (aPrevArea == CustomizableUI.AREA_PANEL) {
> +            let panel = aDocument.getElementById(panelId);
> +            panel.removeEventListener("popupshowing", updateZoomResetButton);
> +          }

So this should be an additional improvement, right? I didn't know that onBuild() is called again when a widget is moved from the palette.
Attachment #787604 - Flags: review?(mdeboer) → review+
Landed with your nits addressed and a couple minor changes:
https://hg.mozilla.org/projects/ux/rev/8350a4f51a48
Whiteboard: [Australis:M?] → [Australis:M8][fixed-in-ux]
Summary: Investigate removing the dependency on FullZoom_onLocationChange from the zoom controls → Remove the dependency on FullZoom_onLocationChange from the zoom controls

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/8350a4f51a48
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.