Handle the keyboard event for ctrl+a inside pdf.js instead of out_of_process_pdf.cc

Handle the keyboard event for ctrl+a  inside pdf.js instead of out_of_process_pdf.cc

This moves the logic to handle the ctrl+a keyboard event into the pdf.js container page, sending a message to the plugin to select all the text. This ensures that the event is handled, even if the plugin element doesn't have focus in the document.

BUG=419986
Committed: https://crrev.com/2faaced431869fd80ba281d230d0bc4bcfec32fb
Cr-Commit-Position: refs/heads/master@{#297998}

[Deepak, 2014-10-01 10:36:57]

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

[Deepak, 2014-10-01 10:36:57]

PTAL

[raymes, 2014-10-01 12:28:01]

I don't think this is needed because the functionality is implemented in
the JS.

On Wed Oct 01 2014 at 8:36:58 PM <deepak.m1@samsung.com> wrote:

> Reviewers: raymes,
>
> Message:
> PTAL
>
> Description:
> Logic of rotation of pdf by ctrl+[] is not present in
> Out_of_process_instance.cc
>
> The Logic for rotating the pdf that open in chrome is not
> present in Out_of_Process_instance.cc file's
> OutOfProcessInstance::HandleInputEvent().Logic added as
> HandleInputEvent() have in Instance.cc file.
>
> BUG=419306
>
> Please review this at https://codereview.chromium.org/615853003/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+19, -6 lines):
>    M pdf/out_of_process_instance.cc
>
>
> Index: pdf/out_of_process_instance.cc
> diff --git a/pdf/out_of_process_instance.cc b/pdf/out_of_process_instance.
> cc
> index
> a385bb3aa93e8fcb8e1ff851786043c6d4bea994..4b2f98ed41ffbd70562c68de759361
> c903a15361
> 100644
> --- a/pdf/out_of_process_instance.cc
> +++ b/pdf/out_of_process_instance.cc
> @@ -488,13 +488,26 @@ bool OutOfProcessInstance::HandleInputEvent(
>     // TODO(raymes): Implement this scroll behavior in JS:
>     // When click+dragging, scroll the document correctly.
>
> -  if (event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN &&
> -      event.GetModifiers() & kDefaultKeyModifier) {
> +  if (event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN) {
>       pp::KeyboardInputEvent keyboard_event(event);
> -    switch (keyboard_event.GetKeyCode()) {
> -      case 'A':
> -        engine_->SelectAll();
> -        return true;
> +    const uint32 modifier = event.GetModifiers();
> +    if (modifier & kDefaultKeyModifier) {
> +      switch (keyboard_event.GetKeyCode()) {
> +        case 'A':
> +          engine_->SelectAll();
> +          return true;
> +      }
> +    } else if (modifier & PP_INPUTEVENT_MODIFIER_CONTROLKEY) {
> +      switch (keyboard_event.GetKeyCode()) {
> +        case ui::VKEY_OEM_4:
> +          // Left bracket.
> +          engine_->RotateCounterclockwise();
> +          return true;
> +        case ui::VKEY_OEM_6:
> +          // Right bracket.
> +          engine_->RotateClockwise();
> +          return true;
> +      }
>       }
>     }
>
>
>
>

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

[Deepak, 2014-10-01 12:33:23]

I saw the keyboard_event.GetKeyCode() for 'A' with SelectAll call is present
here,so thought that rotation handling should also be here.

On 2014/10/01 12:28:01, raymes wrote:
> I don't think this is needed because the functionality is implemented in
> the JS.
> 
> On Wed Oct 01 2014 at 8:36:58 PM <mailto:deepak.m1@samsung.com> wrote:
> 
> > Reviewers: raymes,
> >
> > Message:
> > PTAL
> >
> > Description:
> > Logic of rotation of pdf by ctrl+[] is not present in
> > Out_of_process_instance.cc
> >
> > The Logic for rotating the pdf that open in chrome is not
> > present in Out_of_Process_instance.cc file's
> > OutOfProcessInstance::HandleInputEvent().Logic added as
> > HandleInputEvent() have in Instance.cc file.
> >
> > BUG=419306
> >
> > Please review this at https://codereview.chromium.org/615853003/
> >
> > SVN Base: https://chromium.googlesource.com/chromium/src.git@master
> >
> > Affected files (+19, -6 lines):
> >    M pdf/out_of_process_instance.cc
> >
> >
> > Index: pdf/out_of_process_instance.cc
> > diff --git a/pdf/out_of_process_instance.cc b/pdf/out_of_process_instance.
> > cc
> > index
> > a385bb3aa93e8fcb8e1ff851786043c6d4bea994..4b2f98ed41ffbd70562c68de759361
> > c903a15361
> > 100644
> > --- a/pdf/out_of_process_instance.cc
> > +++ b/pdf/out_of_process_instance.cc
> > @@ -488,13 +488,26 @@ bool OutOfProcessInstance::HandleInputEvent(
> >     // TODO(raymes): Implement this scroll behavior in JS:
> >     // When click+dragging, scroll the document correctly.
> >
> > -  if (event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN &&
> > -      event.GetModifiers() & kDefaultKeyModifier) {
> > +  if (event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN) {
> >       pp::KeyboardInputEvent keyboard_event(event);
> > -    switch (keyboard_event.GetKeyCode()) {
> > -      case 'A':
> > -        engine_->SelectAll();
> > -        return true;
> > +    const uint32 modifier = event.GetModifiers();
> > +    if (modifier & kDefaultKeyModifier) {
> > +      switch (keyboard_event.GetKeyCode()) {
> > +        case 'A':
> > +          engine_->SelectAll();
> > +          return true;
> > +      }
> > +    } else if (modifier & PP_INPUTEVENT_MODIFIER_CONTROLKEY) {
> > +      switch (keyboard_event.GetKeyCode()) {
> > +        case ui::VKEY_OEM_4:
> > +          // Left bracket.
> > +          engine_->RotateCounterclockwise();
> > +          return true;
> > +        case ui::VKEY_OEM_6:
> > +          // Right bracket.
> > +          engine_->RotateClockwise();
> > +          return true;
> > +      }
> >       }
> >     }
> >
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[raymes, 2014-10-01 14:10:53]

I think we should change ctrl+a as well. The problem is that if the plugin
isn't focussed (say the UI is focussed instead) then the wrong thing will
happen. We should have a list of keys events which get forwarded onto the
plugin inside pdf.js.

On Wed Oct 01 2014 at 10:33:32 PM <deepak.m1@samsung.com> wrote:

>
> I saw the keyboard_event.GetKeyCode() for 'A' with SelectAll call is
> present
> here,so thought that rotation handling should also be here.
>
> On 2014/10/01 12:28:01, raymes wrote:
> > I don't think this is needed because the functionality is implemented in
> > the JS.
>
> > On Wed Oct 01 2014 at 8:36:58 PM <mailto:deepak.m1@samsung.com> wrote:
>
> > > Reviewers: raymes,
> > >
> > > Message:
> > > PTAL
> > >
> > > Description:
> > > Logic of rotation of pdf by ctrl+[] is not present in
> > > Out_of_process_instance.cc
> > >
> > > The Logic for rotating the pdf that open in chrome is not
> > > present in Out_of_Process_instance.cc file's
> > > OutOfProcessInstance::HandleInputEvent().Logic added as
> > > HandleInputEvent() have in Instance.cc file.
> > >
> > > BUG=419306
> > >
> > > Please review this at https://codereview.chromium.org/615853003/
> > >
> > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master
> > >
> > > Affected files (+19, -6 lines):
> > >    M pdf/out_of_process_instance.cc
> > >
> > >
> > > Index: pdf/out_of_process_instance.cc
> > > diff --git a/pdf/out_of_process_instance.cc
> > b/pdf/out_of_process_instance.
> > > cc
> > > index
> > > a385bb3aa93e8fcb8e1ff851786043c6d4bea994..
> 4b2f98ed41ffbd70562c68de759361
> > > c903a15361
> > > 100644
> > > --- a/pdf/out_of_process_instance.cc
> > > +++ b/pdf/out_of_process_instance.cc
> > > @@ -488,13 +488,26 @@ bool OutOfProcessInstance::HandleInputEvent(
> > >     // TODO(raymes): Implement this scroll behavior in JS:
> > >     // When click+dragging, scroll the document correctly.
> > >
> > > -  if (event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN &&
> > > -      event.GetModifiers() & kDefaultKeyModifier) {
> > > +  if (event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN) {
> > >       pp::KeyboardInputEvent keyboard_event(event);
> > > -    switch (keyboard_event.GetKeyCode()) {
> > > -      case 'A':
> > > -        engine_->SelectAll();
> > > -        return true;
> > > +    const uint32 modifier = event.GetModifiers();
> > > +    if (modifier & kDefaultKeyModifier) {
> > > +      switch (keyboard_event.GetKeyCode()) {
> > > +        case 'A':
> > > +          engine_->SelectAll();
> > > +          return true;
> > > +      }
> > > +    } else if (modifier & PP_INPUTEVENT_MODIFIER_CONTROLKEY) {
> > > +      switch (keyboard_event.GetKeyCode()) {
> > > +        case ui::VKEY_OEM_4:
> > > +          // Left bracket.
> > > +          engine_->RotateCounterclockwise();
> > > +          return true;
> > > +        case ui::VKEY_OEM_6:
> > > +          // Right bracket.
> > > +          engine_->RotateClockwise();
> > > +          return true;
> > > +      }
> > >       }
> > >     }
> > >
> > >
> > >
> > >
>
> > 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/615853003/
>

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

[Deepak, 2014-10-01 14:29:34]

On 2014/10/01 14:10:53, raymes wrote:
> I think we should change ctrl+a as well. The problem is that if the plugin
> isn't focussed (say the UI is focussed instead) then the wrong thing will
> happen. We should have a list of keys events which get forwarded onto the
> plugin inside pdf.js.

Thanks raymes, I will come up with more info about this.

> On Wed Oct 01 2014 at 10:33:32 PM <mailto:deepak.m1@samsung.com> wrote:
> 
> >
> > I saw the keyboard_event.GetKeyCode() for 'A' with SelectAll call is
> > present
> > here,so thought that rotation handling should also be here.
> >
> > On 2014/10/01 12:28:01, raymes wrote:
> > > I don't think this is needed because the functionality is implemented in
> > > the JS.
> >
> > > On Wed Oct 01 2014 at 8:36:58 PM <mailto:deepak.m1@samsung.com> wrote:
> >
> > > > Reviewers: raymes,
> > > >
> > > > Message:
> > > > PTAL
> > > >
> > > > Description:
> > > > Logic of rotation of pdf by ctrl+[] is not present in
> > > > Out_of_process_instance.cc
> > > >
> > > > The Logic for rotating the pdf that open in chrome is not
> > > > present in Out_of_Process_instance.cc file's
> > > > OutOfProcessInstance::HandleInputEvent().Logic added as
> > > > HandleInputEvent() have in Instance.cc file.
> > > >
> > > > BUG=419306
> > > >
> > > > Please review this at https://codereview.chromium.org/615853003/
> > > >
> > > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master
> > > >
> > > > Affected files (+19, -6 lines):
> > > >    M pdf/out_of_process_instance.cc
> > > >
> > > >
> > > > Index: pdf/out_of_process_instance.cc
> > > > diff --git a/pdf/out_of_process_instance.cc
> > > b/pdf/out_of_process_instance.
> > > > cc
> > > > index
> > > > a385bb3aa93e8fcb8e1ff851786043c6d4bea994..
> > 4b2f98ed41ffbd70562c68de759361
> > > > c903a15361
> > > > 100644
> > > > --- a/pdf/out_of_process_instance.cc
> > > > +++ b/pdf/out_of_process_instance.cc
> > > > @@ -488,13 +488,26 @@ bool OutOfProcessInstance::HandleInputEvent(
> > > >     // TODO(raymes): Implement this scroll behavior in JS:
> > > >     // When click+dragging, scroll the document correctly.
> > > >
> > > > -  if (event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN &&
> > > > -      event.GetModifiers() & kDefaultKeyModifier) {
> > > > +  if (event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN) {
> > > >       pp::KeyboardInputEvent keyboard_event(event);
> > > > -    switch (keyboard_event.GetKeyCode()) {
> > > > -      case 'A':
> > > > -        engine_->SelectAll();
> > > > -        return true;
> > > > +    const uint32 modifier = event.GetModifiers();
> > > > +    if (modifier & kDefaultKeyModifier) {
> > > > +      switch (keyboard_event.GetKeyCode()) {
> > > > +        case 'A':
> > > > +          engine_->SelectAll();
> > > > +          return true;
> > > > +      }
> > > > +    } else if (modifier & PP_INPUTEVENT_MODIFIER_CONTROLKEY) {
> > > > +      switch (keyboard_event.GetKeyCode()) {
> > > > +        case ui::VKEY_OEM_4:
> > > > +          // Left bracket.
> > > > +          engine_->RotateCounterclockwise();
> > > > +          return true;
> > > > +        case ui::VKEY_OEM_6:
> > > > +          // Right bracket.
> > > > +          engine_->RotateClockwise();
> > > > +          return true;
> > > > +      }
> > > >       }
> > > >     }
> > > >
> > > >
> > > >
> > > >
> >
> > > 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/615853003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nikhil, 2014-10-01 14:48:30]

Thanks deepak.m1@ for working on this! I verified this functionality, and it
seems to be working fine in out-of-process (OOP) version. 

To test OOP version, please launch Chrome with "--out-of-process-pdf
--enable-mime-handler-view". You should be able to rotate pdf using Ctrl + []
shortcut.

Thanks!

[Deepak, 2014-10-01 15:30:54]

On 2014/10/01 14:48:30, Nikhil wrote:
> Thanks deepak.m1@ for working on this! I verified this functionality, and it
> seems to be working fine in out-of-process (OOP) version. 
> 
> To test OOP version, please launch Chrome with "--out-of-process-pdf
> --enable-mime-handler-view". You should be able to rotate pdf using Ctrl + []
> shortcut.
> 
> Thanks!

I agree, rotation works without the handling in OOP HandleInputEvent().
Here we need to handle ctrl+A, as this is not getting handled from javascript as
raymes suggested.

How about handling ctrl+A also in the javascript, so that in OOP
HandleInputEvent() only mouse events will get handles.?
Please let me know your opinion on this, I will close this review after knowing
your opinion.
Thanks.

[raymes, 2014-10-01 16:02:19]

>
>
> I agree, rotation works without the handling in OOP HandleInputEvent().
> Here we need to handle ctrl+A, as this is not getting handled from
> javascript as
> raymes suggested.
>
> How about handling ctrl+A also in the javascript, so that in OOP
> HandleInputEvent() only mouse events will get handles.?
> Please let me know your opinion on this, I will close this review after
> knowing

your opinion.
> Thanks.
>

Yep I agree.



>
> https://codereview.chromium.org/615853003/
>

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

[Nikhil, 2014-10-01 17:08:39]

deepak.m1@ - To help you get started and assuming we just want to pull out logic
for 'SelectAll' from out_of_process_instance.cc and move it to pdf.js, I believe
following CL would be a good starting point :
https://codereview.chromium.org/581913003/

Basically :
[*] Handle case in pdf.js
[*] Post a message from page to plugin
[*] Handle the message in plugin (out_of_process_instance.cc) 
[*] Call engine API for 'SelectAll'

Hope it helps :)

[Deepak, 2014-10-02 08:02:23]

On 2014/10/01 17:08:39, Nikhil wrote:
> deepak.m1@ - To help you get started and assuming we just want to pull out
logic
> for 'SelectAll' from out_of_process_instance.cc and move it to pdf.js, I
believe
> following CL would be a good starting point :
> https://codereview.chromium.org/581913003/
> 
> Basically :
> [*] Handle case in pdf.js
> [*] Post a message from page to plugin
> [*] Handle the message in plugin (out_of_process_instance.cc) 
> [*] Call engine API for 'SelectAll'
> 
> Hope it helps :)

Thanks Nikhil for suggestion and providing reference.
I have done the changes.
PTAL.

[Nikhil, 2014-10-02 12:27:01]

Please wait for raymes@ to take a look at this before you update the patch based
on my suggestion (i.e. one nit comment).

Also, a few suggestions :

[*] Please file a bug for this. The one currently mentioned in the description
isn't valid anymore.

[*] Please update the title and the description of the CL to match what we are
trying to accomplish here.

Otherwise, changes look fine to me. Thanks for your work! :)

https://codereview.chromium.org/615853003/diff/20001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/615853003/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:130: const char kJSSelectAllType[] = "selectAll";
How about adding a comment here? Looks to be a practice that is being followed
here. Please see comments/code above.

https://codereview.chromium.org/615853003/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:491: // TODO(raymes): Implement this scroll
behavior in JS:
Is this TODO still valid?

[Deepak, 2014-10-02 12:40:32]

PTAL.

https://codereview.chromium.org/615853003/diff/20001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/615853003/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:130: const char kJSSelectAllType[] = "selectAll";
On 2014/10/02 12:27:01, Nikhil wrote:
> How about adding a comment here? Looks to be a practice that is being followed
> here. Please see comments/code above.

Done.

https://codereview.chromium.org/615853003/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:491: // TODO(raymes): Implement this scroll
behavior in JS:
On 2014/10/02 12:27:00, Nikhil wrote:
> Is this TODO still valid?

I think this is handles in the js by arrow keys and page up down, As comments
was already their, raymus , please tell us your opinion.

[raymes, 2014-10-02 14:18:56]

lgtm with the following minor changes

Also I updated the CL description a bit, I hope that's ok!

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

https://codereview.chromium.org/615853003/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:222: if (e.ctrlKey) {
Should be e.ctrlKey || e.metaKey

https://codereview.chromium.org/615853003/diff/40001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/615853003/diff/40001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:130: // SelectAll content (Page -> Plugin)
nit: // Select all text in the document (Page -> Plugin)

[Deepak, 2014-10-02 14:57:35]

Thanks @raymus for review & your time.
I have done changes as suggested.
PTAL.

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

https://codereview.chromium.org/615853003/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:222: if (e.ctrlKey) {
On 2014/10/02 14:18:56, raymes wrote:
> Should be e.ctrlKey || e.metaKey

Done.

https://codereview.chromium.org/615853003/diff/40001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/615853003/diff/40001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:130: // SelectAll content (Page -> Plugin)
On 2014/10/02 14:18:56, raymes wrote:
> nit: // Select all text in the document (Page -> Plugin)

Done.

[raymes, 2014-10-02 15:00:26]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-10-02 15:01:49]

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

[raymes, 2014-10-02 15:01:52]

Once you have 'lgtm' from all reviewers and address any additional comments,
feel free to land

[commit-bot: I haz the power, 2014-10-02 15:09:43]

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

[commit-bot: I haz the power, 2014-10-02 15:09:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Nikhil, 2014-10-02 15:55:25]

deepak.m1@ - Could you please file a bug for this? Or update the description
with the right one if you've already filed a bug for this? I say this because
the one currently mentioned in the description i.e. 419306
(https://code.google.com/p/chromium/issues/detail?id=419306) doesn't seem right.
419306 is about moving rotation logic and is already marked as 'WONTFIX'. I
believe it would be better to have a separate bug for this.

Also, you'd need OWNERS review for pdf.js to land this.

[Deepak, 2014-10-02 17:20:50]

On 2014/10/02 15:55:25, Nikhil wrote:
> deepak.m1@ - Could you please file a bug for this? Or update the description
> with the right one if you've already filed a bug for this? I say this because
> the one currently mentioned in the description i.e. 419306
> (https://code.google.com/p/chromium/issues/detail?id=419306) doesn't seem
right.
> 419306 is about moving rotation logic and is already marked as 'WONTFIX'. I
> believe it would be better to have a separate bug for this.
> 
> Also, you'd need OWNERS review for pdf.js to land this.

Ok,Nikhil,I will file a new issue for this changes.
And will add pdf.js file owner for review.
Thanks.

[Deepak, 2014-10-03 05:51:30]

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

[Deepak, 2014-10-03 05:56:23]

@xiyuan PTAL.
@Nikhil, New bug 419986 logged for tracking this changes.And updated in review
also.
Thanks.

[xiyuan, 2014-10-03 06:08:51]

lgtm

[Deepak, 2014-10-03 06:09:32]

On 2014/10/03 06:08:51, xiyuan wrote:
> lgtm

Thanks xiyuan.

[Deepak, 2014-10-03 06:09:38]

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

[commit-bot: I haz the power, 2014-10-03 06:09:50]

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

[commit-bot: I haz the power, 2014-10-03 06:13:27]

Committed patchset #4 (id:60001) as f329b8c43c794d1fca04a3ad7f8874695afc162c

[commit-bot: I haz the power, 2014-10-03 06:15:19]

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