Share code between the customizable zoom control and the location bar's zoom indicator

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

7 months ago
The code currently managing the customizable zoom control is a bit more messy than the URLBarZoom.jsm code and maybe even suffers from subtle bugs still. Now, rather than copying the code from one place to the other, we should just share it.
(Assignee)

Updated

7 months ago
Depends on: 869933, 565718
Comment hidden (mozreview-request)
(sorry, I'm out today and won't be able to review it today)
Comment on attachment 8848264 [details]
Bug 1348122 - Share code between the customizable zoom control and the location bar's zoom indicator.

https://reviewboard.mozilla.org/r/121202/#review123814

::: browser/modules/FullZoomUI.jsm:12
(Diff revision 1)
>  
> -this.EXPORTED_SYMBOLS = [ "URLBarZoom" ];
> +this.EXPORTED_SYMBOLS = [ "FullZoomUI" ];
>  
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  
> -var URLBarZoom = {
> +let windows = [];

Use a const Set instead? Then you can use the more idiomatic .add and .delete to add/remove items.

::: browser/modules/FullZoomUI.jsm:108
(Diff revision 1)
> +customizationListener.onWidgetAdded =
> +customizationListener.onWidgetRemoved =
> +customizationListener.onWidgetMoved =
> +customizationListener.onWidgetInstanceRemoved = function(aWidgetId) {
> +  if (aWidgetId == "zoom-controls") {
> +    for (let window of windows) {

In fact, rather than keeping our own set of windows, you could just iterate over CustomizableUI.windows directly.
Attachment #8848264 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)

Comment 5

7 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8f8b6cb5b7b
Share code between the customizable zoom control and the location bar's zoom indicator. r=Gijs

Comment 6

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c8f8b6cb5b7b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Please request Aurora approval on this when you get a chance (jaws mentioned that you wanted to uplift this when we were discussing bug 1318830 and bug 1345375).
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 8

7 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> Please request Aurora approval on this when you get a chance (jaws mentioned
> that you wanted to uplift this when we were discussing bug 1318830 and bug
> 1345375).

That's a bit of a mischaracterization. Jared wanted to uplift bug 1345375 to ease potential future uplifts. This is only going to pan out if we uplift this too.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 9

7 months ago
Created attachment 8851945 [details] [diff] [review]
patch for uplift

Approval Request Comment
[Feature/Bug causing the regression]: bug 565718 & bug 869933
[User impact if declined]: /
[Is this code covered by automated tests?]: yes, to some extent
[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?]: average risk level
[Why is the change risky/not risky?]: there's nothing particularly risky about this patch
[String changes made/needed]: the original patch has string changes. I've removed them in this patch.
Attachment #8851945 - Flags: approval-mozilla-aurora?
status-firefox54: --- → affected
Hi ::dao,
From your uplift request, there is no user impact. Why do we need this in Aurora54? Can we let this ride the train on 55 and won't fix in 54?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 11

7 months ago
(In reply to Gerry Chang [:gchang] from comment #10)
> Hi ::dao,
> From your uplift request, there is no user impact. Why do we need this in
> Aurora54? Can we let this ride the train on 55 and won't fix in 54?

Please see comment 8.
Flags: needinfo?(dao+bmo)
Comment on attachment 8851945 [details] [diff] [review]
patch for uplift

Take this to ease potential future uplifts. Aurora54+.
Attachment #8851945 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
 https://hg.mozilla.org/releases/mozilla-aurora/rev/0032f065b456fef31bc6deb4ead372dd8faa9611
status-firefox54: affected → fixed
Depends on: 1356911
You need to log in before you can comment on or make changes to this bug.