PDF: Change the rotate shortcut keys to ctrl+[] on all platforms.

PDF: Change the rotate shortcut keys to ctrl+[] on all platforms.

The default modifier key on Mac is Command, but that conflicts with
the forward and backward keys.

BUG=111232, 417902

[Lei Zhang, 2014-09-26 01:16:41]

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

[raymes, 2014-09-26 02:49:27]

lgtm

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

https://codereview.chromium.org/606933002/diff/1/pdf/instance.cc#newcode599
pdf/instance.cc:599: else if (event.GetModifiers() &
PP_INPUTEVENT_MODIFIER_CONTROLKEY) {
nit: shift onto the previous line

[Lei Zhang, 2014-09-26 04:31:41]

Will wait a bit longer in case there's additional feedback.

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

https://codereview.chromium.org/606933002/diff/1/pdf/instance.cc#newcode599
pdf/instance.cc:599: else if (event.GetModifiers() &
PP_INPUTEVENT_MODIFIER_CONTROLKEY) {
On 2014/09/26 02:49:26, raymes wrote:
> nit: shift onto the previous line

Done.

[Nikhil, 2014-09-26 05:13:30]

Thanks thestig@! The other suggestion is to use "/" and "\". Please see -
https://code.google.com/p/chromium/issues/detail?id=111232#c9. But having said
that, "Cntrl + []" sounds better as "[]" are more commonly used for rotations.

[Lei Zhang, 2014-09-26 21:07:33]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-09-26 21:09:02]

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

[commit-bot: I haz the power, 2014-09-26 21:16:04]

Committed patchset #2 (id:20001) as 9e63365a43ca0a613902b49ebf705d4c0b0791ba

[commit-bot: I haz the power, 2014-09-26 21:16:59]

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

[Nikhil, 2014-10-01 14:44:23]

raymes@, thestig@ - Out of curiosity, was it intentional _not_ to support this
in in-process version? If not, please let me know and I'll be happy to take a
look :)

[raymes, 2014-10-01 16:07:37]

I'm not sure what you mean? I think thestig@'s patch was meant to change
in-process too.

On Wed Oct 01 2014 at 12:44:24 AM <n.bansal@samsung.com> wrote:

> raymes@, thestig@ - Out of curiosity, was it intentional _not_ to support
> this
> in in-process version? If not, please let me know and I'll be happy to take
> a
> look :)
>
> https://codereview.chromium.org/606933002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nikhil, 2014-10-01 16:55:30]

On 2014/10/01 16:07:37, raymes wrote:
> I'm not sure what you mean? I think thestig@'s patch was meant to change
> in-process too.
> 

Sorry, I should have been more clearer :)

I tested ToT on Linux and rotation shortcut is not working. The reason I suspect
is that we are entering 'if' case because kDefaultKeyModifier is Ctrl key and []
keys are not handled in switch case. On Mac, we must be entering 'else if' case.

Please see :
[*] 'if' case -
https://code.google.com/p/chromium/codesearch#chromium/src/pdf/instance.cc&l=594
[*] 'kDefaultKeyModifier' -
https://code.google.com/p/chromium/codesearch#chromium/src/pdf/pdf_engine.h&l=41

I wasn't sure whether it was intentional _not_ to support rotation in in-process
considering OOP release and code-changes required to support this in in-process.
If you think this should be fixed in in-process, then please let me know. I'll
try to prepare a CL for this.

(hoping I'm not way off here, apologies in advance if that's the case)

> On Wed Oct 01 2014 at 12:44:24 AM <mailto:n.bansal@samsung.com> wrote:
> 
> > raymes@, thestig@ - Out of curiosity, was it intentional _not_ to support
> > this
> > in in-process version? If not, please let me know and I'll be happy to take
> > a
> > look :)
> >
> > https://codereview.chromium.org/606933002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[raymes, 2014-10-02 20:01:50]

Ah I see the problem. Yeah it would be good to fix it. I don't think it's
very urgent though.

Thanks!

On Wed Oct 01 2014 at 10:55:31 AM <n.bansal@samsung.com> wrote:

> On 2014/10/01 16:07:37, raymes wrote:
> > I'm not sure what you mean? I think thestig@'s patch was meant to change
> > in-process too.
>
>
> Sorry, I should have been more clearer :)
>
> I tested ToT on Linux and rotation shortcut is not working. The reason I
> suspect
> is that we are entering 'if' case because kDefaultKeyModifier is Ctrl key
> and []
> keys are not handled in switch case. On Mac, we must be entering 'else if'
> case.
>
> Please see :
> [*] 'if' case -
> https://code.google.com/p/chromium/codesearch#chromium/src/
> pdf/instance.cc&l=594
> [*] 'kDefaultKeyModifier' -
> https://code.google.com/p/chromium/codesearch#chromium/src/
> pdf/pdf_engine.h&l=41
>
> I wasn't sure whether it was intentional _not_ to support rotation in
> in-process
> considering OOP release and code-changes required to support this in
> in-process.
> If you think this should be fixed in in-process, then please let me know.
> I'll
> try to prepare a CL for this.
>
> (hoping I'm not way off here, apologies in advance if that's the case)
>
> > On Wed Oct 01 2014 at 12:44:24 AM <mailto:n.bansal@samsung.com> wrote:
>
> > > raymes@, thestig@ - Out of curiosity, was it intentional _not_ to
> > support
> > > this
> > > in in-process version? If not, please let me know and I'll be happy to
> > take
> > > a
> > > look :)
> > >
> > > https://codereview.chromium.org/606933002/
> > >
>
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> https://codereview.chromium.org/606933002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.