OOP PDF: Do not call setZoom in response to an onZoomChange event.

OOP PDF: Do not call setZoom in response to an onZoomChange event.

Previously, the PDF extension called chrome.tabs.setZoom in response to
chrome.tabs.onZoomChange events received while a setZoom call was not
in progress. This setZoom call unnecessarily requested the zoom be set
to the current zoom value. Prior to another fix, this sometimes caused
the zoom indicator bubble to flicker.

This CL refactors the logic for when to call chrome.tabs.setZoom into a
separate, tested class that doesn't call setZoom in response to
onZoomChange events.

Committed: https://crrev.com/3e4104f7c8ccbb8987fcbde71c1402e123d2cb13
Cr-Commit-Position: refs/heads/master@{#324570}

[Sam McNally, 2015-03-24 03:41:39]

sammc@chromium.org changed reviewers:
+ raymes@chromium.org

[Sam McNally, 2015-03-25 07:13:18]

Patchset #2 (id:20001) has been deleted

[Sam McNally, 2015-03-25 07:40:47]

Patchset #2 (id:40001) has been deleted

[Sam McNally, 2015-03-25 07:44:24]

Refactored browser zoom management out into a separate class as discussed.

[Sam McNally, 2015-04-07 06:59:22]

ping

[raymes, 2015-04-09 03:40:01]

Looks great, thank you!!

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:187: return;
Do you think we should pass in the zoom id? Then we could bind a function of
zoomManager directly?

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:587:
chrome.tabs.setZoom(this.streamDetails_.tabId, zoom, resolve);
Why don't you pass in the function that sets the zoom into the zoom manager and
create the promise inside? It might be a bit clearer.

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/pdf/zoom_manager.js (right):

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/zoom_manager.js:11: constructor(viewport,
changeBrowserZoom) {
nit: setBrowserZoomFunction. Can we have documentation for the parameters?

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/zoom_manager.js:25: // the zoom. onZoomChange() is
called before setZoomComplete() callback
Should this be "onBrowserZoomChange"?

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/zoom_manager.js:43: // when we initiate the zoom.
I'm not sure if this comment makes sense here? Won't this just stop the
extension from setting the zoom if there is a zoom change already in progress?
In that case we will always call back into onPdfZoomChange below to make sure
the zoom is up to date.

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/zoom_manager.js:54: this.onPdfZoomChange();
This looks funny to a reader of the code who doesn't know what's going on. I
think we can make a note:
// The extension's zoom level may have changed while the browser zoom change was
in progress. We call back into onPdfZoomChange to ensure the browser zoom is up
to date.

https://codereview.chromium.org/1026223002/diff/80001/chrome/test/data/pdf/zo...
File chrome/test/data/pdf/zoom_manager_test.js (right):

https://codereview.chromium.org/1026223002/diff/80001/chrome/test/data/pdf/zo...
chrome/test/data/pdf/zoom_manager_test.js:20: class MockBrowserZoomChanger {
Please add a comment for this class. And comments for changeZoom and complete

[Sam McNally, 2015-04-09 05:28:05]

Patchset #4 (id:100001) has been deleted

[Sam McNally, 2015-04-09 05:29:23]

Patchset #4 (id:120001) has been deleted

[Sam McNally, 2015-04-09 08:05:47]

Patchset #5 (id:160001) has been deleted

[Sam McNally, 2015-04-09 08:24:41]

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:187: return;
On 2015/04/09 03:40:01, raymes wrote:
> Do you think we should pass in the zoom id? Then we could bind a function of
> zoomManager directly?

I'd prefer to avoid ZoomManager needing to know about tab IDs.

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:587:
chrome.tabs.setZoom(this.streamDetails_.tabId, zoom, resolve);
On 2015/04/09 03:40:01, raymes wrote:
> Why don't you pass in the function that sets the zoom into the zoom manager
and
> create the promise inside? It might be a bit clearer.

I'd prefer ZoomManager to use a nice interface instead of dealing with
callbacks.

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/pdf/zoom_manager.js (right):

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/zoom_manager.js:11: constructor(viewport,
changeBrowserZoom) {
On 2015/04/09 03:40:01, raymes wrote:
> nit: setBrowserZoomFunction. Can we have documentation for the parameters?

Done.

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/zoom_manager.js:25: // the zoom. onZoomChange() is
called before setZoomComplete() callback
On 2015/04/09 03:40:01, raymes wrote:
> Should this be "onBrowserZoomChange"?

Rewrote this comment.

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/zoom_manager.js:43: // when we initiate the zoom.
On 2015/04/09 03:40:01, raymes wrote:
> I'm not sure if this comment makes sense here? Won't this just stop the
> extension from setting the zoom if there is a zoom change already in progress?
> In that case we will always call back into onPdfZoomChange below to make sure
> the zoom is up to date.

Rewrote this comment too.

https://codereview.chromium.org/1026223002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/zoom_manager.js:54: this.onPdfZoomChange();
On 2015/04/09 03:40:01, raymes wrote:
> This looks funny to a reader of the code who doesn't know what's going on. I
> think we can make a note:
> // The extension's zoom level may have changed while the browser zoom change
was
> in progress. We call back into onPdfZoomChange to ensure the browser zoom is
up
> to date.

Done.

https://codereview.chromium.org/1026223002/diff/80001/chrome/test/data/pdf/zo...
File chrome/test/data/pdf/zoom_manager_test.js (right):

https://codereview.chromium.org/1026223002/diff/80001/chrome/test/data/pdf/zo...
chrome/test/data/pdf/zoom_manager_test.js:20: class MockBrowserZoomChanger {
On 2015/04/09 03:40:01, raymes wrote:
> Please add a comment for this class. And comments for changeZoom and complete

Done.

[raymes, 2015-04-09 23:56:42]

lgtm!

https://codereview.chromium.org/1026223002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/pdf/zoom_manager.js (right):

https://codereview.chromium.org/1026223002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/pdf/zoom_manager.js:51: // zoom level matches the
most-recently-sent zoom level.
nit: most recently sent

[Sam McNally, 2015-04-10 00:02:01]

sammc@chromium.org changed reviewers:
+ thestig@chromium.org

[Sam McNally, 2015-04-10 00:02:02]

+thestig for chrome/browser/resources/component_extension_resources.grd

https://codereview.chromium.org/1026223002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/pdf/zoom_manager.js (right):

https://codereview.chromium.org/1026223002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/pdf/zoom_manager.js:51: // zoom level matches the
most-recently-sent zoom level.
On 2015/04/09 23:56:42, raymes wrote:
> nit: most recently sent

Done.

[Lei Zhang, 2015-04-10 00:11:42]

lgtm

[Sam McNally, 2015-04-10 00:12:11]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-04-10 00:12:12]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1026223002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-04-10 00:13:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1026223002/200001

[commit-bot: I haz the power, 2015-04-10 01:48:43]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2015-04-10 01:49:35]

Patchset 6 (id:??) landed as
https://crrev.com/3e4104f7c8ccbb8987fcbde71c1402e123d2cb13
Cr-Commit-Position: refs/heads/master@{#324570}