Fix rotation shortcut keys in PDF

Fix rotation shortcut keys in PDF

Currently, if the control key is held down on non-mac platforms then we will never check whether the rotation keys are pressed (we will enter the first branch of the if-statement and so never enter the else-branch). This change ensures both branches are tested sequentially.

BUG=429483
Committed: https://crrev.com/eb635da17345d5b82965da47ffd1d22b61a2b873
Cr-Commit-Position: refs/heads/master@{#302418}

[Deepak, 2014-11-02 10:50:34]

deepak.m1@samsung.com changed reviewers:
+ raymes@chromium.org

[Deepak, 2014-11-02 10:50:34]

PTAL

[raymes, 2014-11-03 05:23:56]

Thanks!

https://codereview.chromium.org/689393002/diff/1/pdf/instance.cc
File pdf/instance.cc (left):

https://codereview.chromium.org/689393002/diff/1/pdf/instance.cc#oldcode601
pdf/instance.cc:601: switch (keyboard_event.GetKeyCode()) {
This doesn't look right to me. kDefaultKeyModifier is different on mac and
windows so I disagree that the if-statement is the same on both.

I can see why this might be a problem on non-mac platforms though - if the ctrl
key is held down we will enter the if-block but never the else- block. I think
the right solution here is to get rid of the "else" and just have two sequential
if-statements.

[Deepak, 2014-11-03 06:34:02]

On 2014/11/03 05:23:56, raymes wrote:
> Thanks!
> 
> https://codereview.chromium.org/689393002/diff/1/pdf/instance.cc
> File pdf/instance.cc (left):
> 
> https://codereview.chromium.org/689393002/diff/1/pdf/instance.cc#oldcode601
> pdf/instance.cc:601: switch (keyboard_event.GetKeyCode()) {
> This doesn't look right to me. kDefaultKeyModifier is different on mac and
> windows so I disagree that the if-statement is the same on both.
> 
> I can see why this might be a problem on non-mac platforms though - if the
ctrl
> key is held down we will enter the if-block but never the else- block. I think
> the right solution here is to get rid of the "else" and just have two
sequential
> if-statements.

Thanks @Raymes for review.
I agree with you, Changes done & tested.
PTAL.

[raymes, 2014-11-03 06:49:17]

lgtm

[Deepak, 2014-11-03 06:52:25]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2014-11-03 06:52:58]

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

[raymes, 2014-11-03 06:54:16]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2014-11-03 06:58:50]

I just updated the CL description which wasn't accurate. In general please take
more care with the CL descriptions and make sure they are updated in response to
suggested changes. 

Also please try to put some more work into making the descriptions clear and as
grammatically correct as you are able (I know it might be hard). It may
sometimes be clearer to describe things in point form (a list of changes made
and why they were made).

Thanks!

[raymes, 2014-11-03 06:59:38]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-11-03 07:00:08]

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

[commit-bot: I haz the power, 2014-11-03 07:24:15]

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

[commit-bot: I haz the power, 2014-11-03 07:24:16]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Deepak, 2014-11-03 08:05:14]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2014-11-03 08:05:39]

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

[commit-bot: I haz the power, 2014-11-03 08:23:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-03 08:24:19]

Patchset 2 (id:??) landed as
https://crrev.com/eb635da17345d5b82965da47ffd1d22b61a2b873
Cr-Commit-Position: refs/heads/master@{#302418}