PDF Viewer - PageUp/Down don't move pdf by page size in FitToPage mode

PDF Viewer - PageUp/Down don't move pdf by page size in FitToPage mode

Currently when pdf is scaled to fit to page mode, both page down and
page up keys do not move pdf by page size. This is because both
the keys are not handled in HandleInputEvent().

This patch updates HandleInputEvent() to factor in key codes for
page up and page down to move pdf by page size in fit to page mode.
Similar to left and right arrow keys.

BUG=390599
Committed: https://crrev.com/9dad99260f7d8f991ef4ab502472381a55a12f96
Cr-Commit-Position: refs/heads/master@{#300435}

[Nikhil, 2014-09-13 12:26:46]

n.bansal@samsung.com changed reviewers:
+ raymes@chromium.org, thestig@chromium.org, vitalybuka@chromium.org

[Nikhil, 2014-09-13 12:26:47]

PTAL at my solution to fix the issue where page up and page down do not move pdf
by page size in fit to page mode.

If you've any review comments, then please let me know. I'll incorporate them.

Thanks

[Nikhil, 2014-09-13 12:32:57]

n.bansal@samsung.com changed reviewers:
+ alekseys@chromium.org

[Nikhil, 2014-09-13 12:32:58]

Also adding alekseys@ as reviewer since bug is currently assigned to him. 

Please take a look.

[raymes, 2014-09-15 00:12:14]

https://codereview.chromium.org/573523002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/573523002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:172: var viewport = this.viewport_;
nit: remove this line and just add ".bind(this) below

https://codereview.chromium.org/573523002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:186: };
}.bind(this);

https://codereview.chromium.org/573523002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:197: };
}.bind(this);

https://codereview.chromium.org/573523002/diff/1/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/573523002/diff/1/pdf/instance.cc#newcode522
pdf/instance.cc:522: page_down |= key_is_space || key_code == ui::VKEY_NEXT;
But it should get set here when in ZOOM_FIT_TO_PAGE, right?

[Lei Zhang, 2014-09-15 16:54:51]

With pdf_instance.cc, the problem is that sometimes |zoom_mode_| is ZOOM_SCALE
even though it should be ZOOM_FIT_TO_PAGE. If you change pages a few times and
then press the fit-to-page button again, |zoom_mode_| eventually becomes
ZOOM_FIT_TO_PAGE, and the key handling code works as intended. Try to fix this
rather than changing the key handling code.

[Nikhil, 2014-09-16 10:23:29]

Thanks raymes@, thestig@ for taking a look at this!

The OOP version is fixed and is working fine.

But with instance.cc, what is happening is that when we switch to
ZOOM_FIT_TO_PAGE mode using the pdf ui available on bottom right - 

[1] We first use |SetZoom()| to zoom. This zooms the page to fit to page.

[2] Then we also update browser to update zoom level.

The second operation results in |SetZoom()| getting called again - with same
scale but with different zoom mode. This changes zoom mode to ZOOM_SCALE from
ZOOM_FIT_TO_PAGE. And so, the page down/up do not work properly.

I've dropped the second call to |SetZoom()| by comparing current scale level to
new scale level. If they are same, then just return. I tested a few basic zoom
use cases, and this seems to be working fine.

One change that I could see is that now after the pdf loads, zoom mode is
ZOOM_AUTO and _not_ ZOOM_SCALE. If we don't want this, then maybe plugin should
not receive the call for |SetZoom()| since plugin is the initiator?

Please take a look at my solution. If you've any review comments, then please
let me know. 

Thanks

https://codereview.chromium.org/573523002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/573523002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:172: var viewport = this.viewport_;
On 2014/09/15 00:12:14, raymes wrote:
> nit: remove this line and just add ".bind(this) below

Done.

https://codereview.chromium.org/573523002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:186: };
On 2014/09/15 00:12:14, raymes wrote:
> }.bind(this);

Done.

https://codereview.chromium.org/573523002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:197: };
On 2014/09/15 00:12:14, raymes wrote:
> }.bind(this);

Done.

https://codereview.chromium.org/573523002/diff/1/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/573523002/diff/1/pdf/instance.cc#newcode522
pdf/instance.cc:522: page_down |= key_is_space || key_code == ui::VKEY_NEXT;
On 2014/09/15 00:12:14, raymes wrote:
> But it should get set here when in ZOOM_FIT_TO_PAGE, right?

Acknowledged.

[Lei Zhang, 2014-09-18 22:41:36]

I commented on the bug:
https://code.google.com/p/chromium/issues/detail?id=390599#c12

Let's see what wjmaclean@ says about this.

[Nikhil, 2014-09-19 14:53:07]

On 2014/09/18 22:41:36, Lei Zhang wrote:
> I commented on the bug:
> https://code.google.com/p/chromium/issues/detail?id=390599#c12
> 
> Let's see what wjmaclean@ says about this.

Thanks, I think OOP version is now fixed by Raymes' CL. I'll update this CL for
in-process version based on @wjmaclean's response :)

[wjmaclean, 2014-10-08 14:14:33]

wjmaclean@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[wjmaclean, 2014-10-08 14:14:33]

LGTM (with minor suggestion) for the instance.cc change. I don't understand the
impact of the pdf.js changes well enough to comment on them.

https://codereview.chromium.org/573523002/diff/20001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/573523002/diff/20001/pdf/instance.cc#newcode2429
pdf/instance.cc:2429: // If same zoom level, don't do anything.
You might modify this comment to indicate that, in particular, we avoid changing
the zoom_mode value if the scale is the same.

[raymes, 2014-10-08 17:23:48]

On 2014/10/08 14:14:33, wjmaclean wrote:
> LGTM (with minor suggestion) for the instance.cc change. I don't understand
the
> impact of the pdf.js changes well enough to comment on them.
> 
> https://codereview.chromium.org/573523002/diff/20001/pdf/instance.cc
> File pdf/instance.cc (right):
> 
>
https://codereview.chromium.org/573523002/diff/20001/pdf/instance.cc#newcode2429
> pdf/instance.cc:2429: // If same zoom level, don't do anything.
> You might modify this comment to indicate that, in particular, we avoid
changing
> the zoom_mode value if the scale is the same.

lgtm but please make sure thestig@ is ok with this.

[raymes, 2014-10-08 17:23:55]

https://codereview.chromium.org/573523002/diff/20001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/573523002/diff/20001/pdf/instance.cc#newcode2429
pdf/instance.cc:2429: // If same zoom level, don't do anything.
// If the zoom level doesn't change it means that this zoom change might have
been initiated by
// the plugin. In that case we don't want to change the zoom mode to ZOOM_SCALE
as it may have
// been intentionally set to ZOOM_FIT_TO_PAGE or some other value when the zoom
was last changed.

[Vitaly Buka (NO REVIEWS), 2014-10-08 17:36:09]

vitalybuka@chromium.org changed reviewers:
- alekseys@chromium.org, vitalybuka@chromium.org

[Lei Zhang, 2014-10-08 18:48:09]

lgtm++

Thanks for investigating everyone.

[Nikhil, 2014-10-09 06:36:20]

raymes@, thestig@, wjmaclean@ - Thanks for reviewing! I've updated the comment
based on your suggestion. I'll land this once you take a look at this :)

Thanks!

https://codereview.chromium.org/573523002/diff/20001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/573523002/diff/20001/pdf/instance.cc#newcode2429
pdf/instance.cc:2429: // If same zoom level, don't do anything.
On 2014/10/08 17:23:55, raymes wrote:
> // If the zoom level doesn't change it means that this zoom change might have
> been initiated by
> // the plugin. In that case we don't want to change the zoom mode to
ZOOM_SCALE
> as it may have
> // been intentionally set to ZOOM_FIT_TO_PAGE or some other value when the
zoom
> was last changed.

Done.

[raymes, 2014-10-09 17:15:53]

I just had a look at the code again and I think your change might result in a
slight unwanted change in behavior. In particular, if something calls
SetZoom(ZOOM_FIT_TO_PAGE, x) and the current zoom mode is ZOOM_SCALE then we
really do want to change the zoom mode to ZOOM_FIT_TO_PAGE even if the zoom
level hasn't changed. I think we want to move the check up to the place I
indicated. Will this still fix the issue?

https://codereview.chromium.org/573523002/diff/40001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/573523002/diff/40001/pdf/instance.cc#newcode782
pdf/instance.cc:782: SetZoom(ZOOM_SCALE, scale);
Could we move the check up to before this line?

[wjmaclean, 2014-10-09 17:19:01]

https://codereview.chromium.org/573523002/diff/40001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/573523002/diff/40001/pdf/instance.cc#newcode782
pdf/instance.cc:782: SetZoom(ZOOM_SCALE, scale);
On 2014/10/09 17:15:53, raymes wrote:
> Could we move the check up to before this line?

This was my original thought, as then it only catches duplicates coming from the
renderer; no one else seems to call Zoom() internally.

[Nikhil, 2014-10-20 09:25:29]

raymes@, wjmaclean@ - Sorry for the delay! I've moved the check to Zoom() method
instead of SetZoom() method. PTAL. Thanks!

https://codereview.chromium.org/573523002/diff/40001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/573523002/diff/40001/pdf/instance.cc#newcode782
pdf/instance.cc:782: SetZoom(ZOOM_SCALE, scale);
On 2014/10/09 17:15:53, raymes wrote:
> Could we move the check up to before this line?

Done.

[wjmaclean, 2014-10-20 13:25:13]

https://codereview.chromium.org/573523002/diff/90001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/573523002/diff/90001/pdf/instance.cc#newcode790
pdf/instance.cc:790: SetZoom(ZOOM_SCALE, scale);
This looks better ... lgtm.

[raymes, 2014-10-20 23:07:40]

lgtm

[Nikhil, 2014-10-21 00:23:05]

raymes@, wjmaclean@, thestig@ - Thanks for reviewing! CQing this CL now :)

[Nikhil, 2014-10-21 00:23:14]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-10-21 00:24:53]

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

[commit-bot: I haz the power, 2014-10-21 02:28:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-21 02:28:15]

Exceeded time limit waiting for builds to trigger.

[Nikhil, 2014-10-21 03:13:38]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-10-21 03:15:28]

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

[commit-bot: I haz the power, 2014-10-21 04:19:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-21 04:19:27]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Nikhil, 2014-10-21 05:46:05]

Rebase!

[Nikhil, 2014-10-21 05:46:19]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-10-21 05:47:12]

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

[commit-bot: I haz the power, 2014-10-21 08:10:23]

Committed patchset #5 (id:110001)

[commit-bot: I haz the power, 2014-10-21 08:11:13]

Patchset 5 (id:??) landed as
https://crrev.com/9dad99260f7d8f991ef4ab502472381a55a12f96
Cr-Commit-Position: refs/heads/master@{#300435}