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

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Dão Gottwald [:dao]]

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.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1348122 - Share code between the customizable zoom control and the location bar's zoom indicator.

Review commit: https://reviewboard.mozilla.org/r/121202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121202/

Comment 2

[:Gijs (he/him)]

(sorry, I'm out today and won't be able to review it today)

Comment 3

[:Gijs (he/him)]

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.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8848264 [details]
Bug 1348122 - Share code between the customizable zoom control and the location bar's zoom indicator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121202/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c8f8b6cb5b7b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Dão Gottwald [:dao]]

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

Comment 9

[Dão Gottwald [:dao]]

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

Comment 10

[Gerry Chang [:gchang]]

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?

Comment 11

[Dão Gottwald [:dao]]

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

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8851945 [details] [diff] [review]
patch for uplift

Take this to ease potential future uplifts. Aurora54+.

Comment 13

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-aurora/rev/0032f065b456fef31bc6deb4ead372dd8faa9611