Last Comment Bug 902924 - Remove the dependency on FullZoom_onLocationChange from the zoom controls
: Remove the dependency on FullZoom_onLocationChange from the zoom controls
Status: RESOLVED FIXED
[Australis:M8]
:
Product: Firefox
Classification: Client Software
Component: Toolbars and Customization (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 28
Assigned To: (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me)
:
: :Gijs (he/him)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-08 07:43 PDT by (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me)
Modified: 2013-11-18 12:51 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.47 KB, patch)
2013-08-08 09:01 PDT, (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me)
mikedeboer: review+
Details | Diff | Splinter Review

Description User image (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) 2013-08-08 07:43:44 PDT
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 User image Dão Gottwald [::dao] 2013-08-08 08:33:58 PDT
(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 User image (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) 2013-08-08 08:51:13 PDT
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 User image (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) 2013-08-08 09:01:35 PDT
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
Comment 4 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2013-08-08 09:15:37 PDT
Seems quite related to the discussion in bug 897410!
Comment 5 User image (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) 2013-08-08 09:44:08 PDT
(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.
Comment 6 User image (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) 2013-08-08 10:07:22 PDT
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 User image Mike de Boer [:mikedeboer] 2013-08-08 10:58:04 PDT
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 User image (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) 2013-08-08 11:19:56 PDT
Landed with your nits addressed and a couple minor changes:
https://hg.mozilla.org/projects/ux/rev/8350a4f51a48

Note You need to log in before you can comment on or make changes to this bug.