902924 - Remove the dependency on FullZoom_onLocationChange from the zoom controls

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 1

[Dão Gottwald [:dao]]

(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.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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

Comment 4

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

Seems quite related to the discussion in bug 897410!

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 7

[Mike de Boer [:mikedeboer]]

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.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Landed with your nits addressed and a couple minor changes:
https://hg.mozilla.org/projects/ux/rev/8350a4f51a48

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/8350a4f51a48