956285 - Zoom controls percentage label doesn't update when it's in the toolbar and you navigate page

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/integration/fx-team/rev/b70ad79e2d7c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140102200527

Bug 934951 does not fix this problem.

Steps To Reproduce:
1. Move "zoom controls" to toolbar by customize.
2. Open a page(ex. https://www.mozilla.org/en-US/about/ ) and Zoom in
   --- Zoom controls percentage label is changed to 110% ---- as expected
3. Open New tab
   --- Zoom controls percentage label is changed to 100% ---- as expected
4. Load the same page of step2(i.e. https://www.mozilla.org/en-US/about/ )
   --- Zoom controls percentage label is keeping to stay 100% ---- bug

Actual Results:
Zoom controls percentage label is not changed.

Expected Results:
Zoom controls percentage label should be changed to the site specific zoom level.

Comment 1

[:Gijs (he/him)]

So AFAICT in order to get this right, we need to update the label *after* the browser full zoom code has done its work by checking the content preference service and so on. Which means using that notification which we used to use and then removed, because it was "just for tests" and now is actually called "FullZoom:TESTS:location-change". Drew, what should we be doing here? Because the update is async (AFAICT) we do need some sort of notification... :-\

Comment 2

[Drew Willcoxon :adw]

(In reply to :Gijs Kruitbosch from comment #1)
> Because the update is async (AFAICT) we do need some sort of
> notification... :-\

Yes, whenever a zoom setting for a site isn't cached in memory, we have to check the CPS's database, and in those cases the zoom change is async.

Looking at bug 897410 comment 15 and 17, I think the reason I didn't like this notification being used outside tests is that sometimes the browser that triggers it can be destroyed before it's broadcasted, which observers do not expect, resulting in the JS errors in that bug for example.  So if you rely on this notification, you'll need to make sure that the browser that changed locations is still alive before you use it, e.g., by checking that browser.docShell is still defined.  And actually, the browser isn't included in the notification, but it seems like you would need it in this case, so it probably should be.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: Update zoom controls percentage label upon navigation

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: Update zoom controls percentage label upon navigation

updated comment.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8361588 [details] [diff] [review]
Patch 1: Update zoom controls percentage label upon navigation

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

In principle this looks fine, but a try run would not go amiss for a change like this considering the errors we've seen in the past. (in fact, adding a test for this particular issue would also be a good idea...)

Note that when updating the tests you missed browser_privatebrowsing_zoomrestore.js:

http://mxr.mozilla.org/mozilla-central/search?string=TESTS%3Alocation-change

Regardless of whether you add a test or do a try run, please fix that before landing. :-)

Comment 6

[:Gijs (he/him)]

Comment on attachment 8361588 [details] [diff] [review]
Patch 1: Update zoom controls percentage label upon navigation

Ugh, bugzilla, you suck.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8361590 [details] [diff] [review]
Patch 1.1: Update zoom controls percentage label upon navigation

For the review comment, see my previous comment. Seems you picked up the private browsing one in this version already (forgot to qref?) so that's fine. A test specifically for this issue would be useful though.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: unit test.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8361602 [details] [diff] [review]
Patch 2: unit test.

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

::: browser/components/customizableui/test/browser_934951_zoom_in_toolbar.js
@@ +35,5 @@
> +  // Test zoom label updates while navigating pages in the same tab.
> +  FullZoom.enlarge();
> +  yield zoomChangePromise;
> +  is(parseInt(zoomResetButton.label, 10), 110, "Zoom is changed to 110% for about:mozilla");
> +  yield promiseTabLoadEvent(tab1, "about:home");

If this doesn't return the test will time out and we'll all be very sad (including subsequent tests). Can you reject the promise after some timeout (inside both promiseTabLoadEvent and promiseTabNavigate) and deal will fallout in some kind of cleanup to remove the added tabs?

@@ +87,5 @@
> + * @param aEvent
> + *        The load event type to wait for.  Defaults to "load".
> + * @return {Promise} resolved when the event is handled.
> + */
> +function promiseTabLoadEvent(aTab, aURL, aEventType="load") {

This should probably live in head.js. Did you copy it from somewhere?

@@ +104,5 @@
> +  aTab.linkedBrowser.loadURI(aURL);
> +  return deferred.promise;
> +}
> +
> +function promiseTabNavigate(back = true) {

Ditto.

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: unit test.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8361618 [details] [diff] [review]
Patch 2.1: unit test.

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

r+ but nits below.

::: browser/components/customizableui/test/head.js
@@ +16,5 @@
>  
>  let {synthesizeDragStart, synthesizeDrop} = ChromeUtils;
>  
>  const kNSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +const {Ci: interfaces} = Components;

This is broken; it's defined above already.

@@ +17,5 @@
>  let {synthesizeDragStart, synthesizeDrop} = ChromeUtils;
>  
>  const kNSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +const {Ci: interfaces} = Components;
> +const kTimeoutInMS = 20000;

This should have a more specific name.

@@ +356,5 @@
> +    gBrowser.removeEventListener("pageshow", listener, true);
> +    clearTimeout(timeoutId);
> +    // Make sure to resolve the promise until other async ops are finished (ex.
> +    // FullZoom ops).
> +    setTimeout(() => deferred.resolve(), 2000);

I'd prefer adding a condition callback function and:

if (aConditionCallbackFunction) {
  yield waitForCondition(aConditionCallbackFunction)
}

over the settimeout.

@@ +360,5 @@
> +    setTimeout(() => deferred.resolve(), 2000);
> +  }
> +  gBrowser.addEventListener("pageshow", listener, true);
> +
> +  if (aBack)

We don't seem to ever use the other version of this, and it seems counterintuitive to me that "navigate" goes *back* rather than... somewhere else, and that it has a boolean that indicates when you want to go forward, ie:

promiseTabNavigation(false); // This actually navigates forward

That seems weird.

Maybe name it promiseTabHistoryNavigation or something, and use an int for direction? IIRC there's a gBrowser or navigator method that'll take such an int directly, too. I dunno. As you like.

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: unit test.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8361632 [details] [diff] [review]
Patch 2.2: unit test.

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

r=me with the nits addressed.

::: browser/components/customizableui/test/browser_934951_zoom_in_toolbar.js
@@ +57,5 @@
>    };
>    let timeoutId = setTimeout(() => {
>      Services.obs.removeObserver(notificationCallback, aObserver, false);
>      deferred.reject("Notification '" + aObserver + "' did not happen within 20 seconds.");
> +  }, kTabEventFailureTimeoutInMs);

Assign the value from this to an appropriately named variable at the top of the file. Sorry, I didn't realize you used it here as well...

::: browser/components/customizableui/test/head.js
@@ +17,5 @@
>  let {synthesizeDragStart, synthesizeDrop} = ChromeUtils;
>  
>  const kNSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +const kTabEventFailureTimeoutInMs = 20000;
> +const kBack = -1;

Don't really need these, see below. If you want to, you could keep kBack for the default argument value, but otherwise...

@@ +309,5 @@
> + * Starts a load in an existing tab and waits for it to finish (via some event).
> + *
> + * @param aTab   The tab to load into.
> + * @param aUrl   The url to load.
> + * @param aEvent The load event type to wait for.  Defaults to "load".

You mean aEventType.

@@ +344,5 @@
> + * @param aDirection Number to indicate to move backward in history (kBack) or
> + *                   forward (kForward).
> + * @return {Promise} resolved when navigation has finished.
> + */
> +function promiseTabHistoryNavigation(aDirection = kBack, aConditionFn) {

There's no documentation for aConditionFn.

@@ +368,5 @@
> +
> +  if (aDirection == kBack)
> +    gBrowser.goBack();
> +  else if (aDirection == kForward)
> +    gBrowser.goForward();

content.history.go(aDirection);

Comment 14

[Mike de Boer [:mikedeboer]]

Thanks!

remote: https://hg.mozilla.org/integration/fx-team/rev/14e817fe25e1
remote: https://hg.mozilla.org/integration/fx-team/rev/7c74e99d80c1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc failures. That's two backouts in two days. Can you please run these through Try before pushing?
https://hg.mozilla.org/integration/fx-team/rev/00dca774ecd1

https://tbpl.mozilla.org/php/getParsedLog.php?id=33171646&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=33171720&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=33171783&tree=Fx-Team

Comment 16

[Mike de Boer [:mikedeboer]]

Try build running with observer cleanup: https://tbpl.mozilla.org/?tree=Try&rev=7f0ebe2bb2b8

Comment 17

[Mike de Boer [:mikedeboer]]

re-pushed with memleak fixes:

https://hg.mozilla.org/integration/fx-team/rev/ee6b70532489
https://hg.mozilla.org/integration/fx-team/rev/3843c855d7ec

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee6b70532489
https://hg.mozilla.org/mozilla-central/rev/3843c855d7ec

Comment 19

[Camelia Badau [:cbadau], Desktop QA]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0

Verified fixed on Firefox 29 beta 5 (build ID: 20140403132807).