Handling PNaCl KeyboardInputEvent(s) in the key tester app.

Handling PNaCl KeyboardInputEvent(s) in the key tester app.

BUG=407778
Committed: https://crrev.com/31a6f70b8a22930cc749a67a90c6e7e3778f8466
Cr-Commit-Position: refs/heads/master@{#314424}

[Łukasz Anforowicz, 2015-01-28 22:15:34]

lukasza@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Łukasz Anforowicz, 2015-01-28 22:15:35]

Jamie, could you please take a look?

1. The output is not very pretty, but it includes all the data we care about.

2. I am not quite sure how to use this for testing from ChromeOS for
crbug.com/407778 (i.e. "load unpacked extension" won't work when there is no
local storage?).

At any rate - I think that we can iterate on #1 and #2 next, but they probably
shouldn't block the check-in (i.e. as a test tool, the PNaCl part is fully
functional, even if a bit rough around the edges).

https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
File remoting/tools/javascript_key_tester/chord_tracker.js (right):

https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
remoting/tools/javascript_key_tester/chord_tracker.js:54:
this.parentDiv_.insertBefore(this.currentDiv_, this.parentDiv_.firstChild);
By prepending, rather than appending, one doesn't have to scroll to see the most
recent events.

[Jamie, 2015-01-28 22:31:27]

Re 2: For ChromeOS, we will ultimately host this on CWS. In the interim, you can
upload it as a zip file to Google Drive, unpack it on the device and then load
it as you would on any other OS.

https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
File remoting/tools/javascript_key_tester/chord_tracker.js (right):

https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
remoting/tools/javascript_key_tester/chord_tracker.js:54:
this.parentDiv_.insertBefore(this.currentDiv_, this.parentDiv_.firstChild);
On 2015/01/28 22:15:35, lukasza wrote:
> By prepending, rather than appending, one doesn't have to scroll to see the
most
> recent events.

Acknowledged.

https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
File remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc (right):

https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc:47:
PostMessage(out.str());
I think this should be passed as a dictionary (or a JSON-encoded one if it has
to be a string) so that the JS code can format it in the same way that the it
does for the JS key events. You don't need to make the UI pretty in this CL (as
you say, we can iterate on that), but I think it does makes sense to get the API
right. It will also help to keep the C++ code really simple, which is good from
a standpoint of providing it as a minimal repro case.

[Łukasz Anforowicz, 2015-01-29 00:26:37]

I started addressing your feedback, but then I realized that I really like
strings :-)  Can you please take a look at my reply below?  I've also added
Rintaro in case this can be useful to him for touch events.

Thanks,

Lukasz

https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
File remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc (right):

https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc:47:
PostMessage(out.str());
On 2015/01/28 22:31:27, Jamie wrote:
> I think this should be passed as a dictionary (or a JSON-encoded one if it has
> to be a string) so that the JS code can format it in the same way that the it
> does for the JS key events. You don't need to make the UI pretty in this CL
(as
> you say, we can iterate on that), but I think it does makes sense to get the
API
> right. It will also help to keep the C++ code really simple, which is good
from
> a standpoint of providing it as a minimal repro case.

I wrote code that works and passes the following object to javascript ...
 * @typedef {{type: string,
 *            modifiers: number,
 *            keyCode: number,
 *            characterText: string,
 *            code: string}}
... but then I started having second thoughts.

I think I still prefer passing strings, rather than objects:
*) strings are copy&pasteable (i.e. to share tester's output in an email)
*) current chord_tracker.js is written around keydown/keyup events.  I am wary
of trying to recreate such objects out of PNaCl's data.
*) strings are more versatile - can represent both keyboard and touch events; 
if we want to cover touch events in the future, then it is probably easiest to
recognized them and translate to strings in C++/PNaCl layer, rather than having
to deal with recognizing and formatting different kinds of objects in
javascript)

[Jamie, 2015-01-29 01:21:26]

On 2015/01/29 00:26:37, lukasza wrote:
> I started addressing your feedback, but then I realized that I really like
> strings :-)  Can you please take a look at my reply below?  I've also added
> Rintaro in case this can be useful to him for touch events.
> 
> Thanks,
> 
> Lukasz
> 
>
https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
> File remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc
(right):
> 
>
https://codereview.chromium.org/884703006/diff/1/remoting/tools/javascript_ke...
> remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc:47:
> PostMessage(out.str());
> On 2015/01/28 22:31:27, Jamie wrote:
> > I think this should be passed as a dictionary (or a JSON-encoded one if it
has
> > to be a string) so that the JS code can format it in the same way that the
it
> > does for the JS key events. You don't need to make the UI pretty in this CL
> (as
> > you say, we can iterate on that), but I think it does makes sense to get the
> API
> > right. It will also help to keep the C++ code really simple, which is good
> from
> > a standpoint of providing it as a minimal repro case.
> 
> I wrote code that works and passes the following object to javascript ...
>  * @typedef {{type: string,
>  *            modifiers: number,
>  *            keyCode: number,
>  *            characterText: string,
>  *            code: string}}
> ... but then I started having second thoughts.
> 
> I think I still prefer passing strings, rather than objects:
> *) strings are copy&pasteable (i.e. to share tester's output in an email)

I like the idea of providing a copy-pastable version of the output, but I would
prefer that it not be the primary display because it doesn't give a good
at-a-glance overview of what's going on. What I like about the UI for JS event
is that we show only the high-level information from each event, which means we
can lay them out horizontally to indicate when multiple keys are pressed
together.

Passing structured information from C++ to JS gives us the best of both worlds:
we can build up a copyable string more easily in JS than we can in C++ and also
easily extract the most relevant information for the high-level overview.

> *) current chord_tracker.js is written around keydown/keyup events.  I am wary
> of trying to recreate such objects out of PNaCl's data.

I think KEYUP and KEYDOWN are direct analogs, I think.

> *) strings are more versatile - can represent both keyboard and touch events; 
> if we want to cover touch events in the future, then it is probably easiest to
> recognized them and translate to strings in C++/PNaCl layer, rather than
having
> to deal with recognizing and formatting different kinds of objects in
> javascript)

Adding support for other event types is a nice idea, (in particular, we might
want to do something special about CHAR), but that's straightforward if the
events are structured; less so if they're just strings. I think that the only
difference is whether you do the string concatenation in C++ or JS--the rest of
the C++ (to extract the right fields, convert them into a suitable on-the-wire
format, etc) will be needed in either case.

[Sergey Ulanov, 2015-01-29 19:37:34]

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

[Sergey Ulanov, 2015-01-29 19:37:35]

This all looks very similar to this NaCl example:
native_client_sdk/src/examples/api/input_event/input_event.cc .
But that example doesn't show keycodes for input events. Would it be easier to
add that functionality in that example. It already has functionality that may be
useful for us in the future, e.g. touch events.

[Wez, 2015-01-29 19:41:26]

+1 to adding some nice log output to the existing input event sample. :)

On 29 January 2015 at 11:37, <sergeyu@chromium.org> wrote:

> This all looks very similar to this NaCl example:
> native_client_sdk/src/examples/api/input_event/input_event.cc .
> But that example doesn't show keycodes for input events. Would it be
> easier to
> add that functionality in that example. It already has functionality that
> may be
> useful for us in the future, e.g. touch events.
>
> https://codereview.chromium.org/884703006/
>

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

[Łukasz Anforowicz, 2015-01-30 22:45:28]

Jamie, could you please take another look?

Sergey/Wez: it is probably worth flushing out what I've added to
remoting-specific key_tester, because 1) this is what I can use right now to get
data for crbug.com/407778 and 2) the PNaCl SDK example needs some extra work to
be useful (kMaxLogMessageLength=20 truncates data because of a flood of
DidChangeView events).

[Jamie, 2015-01-30 23:25:09]

Mostly fine; the only non-nit is that I don't think it's worth tracking JS and
PNaCl events in separate divs, which should simplify things a bit.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
File remoting/tools/javascript_key_tester/chord_tracker.js (right):

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/chord_tracker.js:37:
this.endIfNoKeysPressed_();
I don't think you need this here: a keydown event can't cause a transition from
keys pressed -> no keys pressed.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
File remoting/tools/javascript_key_tester/main.js (right):

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:8: * @param {number|undefined}
space
{number=} is a simpler annotation, though you should rename it opt_space in that
case. You can them just omit the parameter instead of passing undefined
explicitly.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:11: function
JsonifyJavascriptKeyEvent(event, eventName, space) {
Function names in JS don't start with a capital letter (with the exception of
ctors, since they effectively define types).

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:42: var PNaClEvent = function(){
Nit: Space after ()

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:74: var pnaclEvent = event.data;
Nit: Since this is effectively a cast (insofar as JS has such a concept), we
conventionally annotate it as such:

  var pnaclEvent = /** @type {PNaclEvent} */ (event.data);

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:103: var jsChordTracker = new
ChordTracker(jsLog);
I think two chord trackers is probably overkill. Better just to have one,
logging events to a single div, and perhaps reset it on focus/blur for the PNaCl
element. I don't think we need to track JS events when the plugin has focus (it
might be useful for applications where the plugin doesn't stop propagation of
events, but the Chromoting plugin does do that, and I'd like this to be as close
as possible to that implementation).

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
File remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc (right):

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc:19:
PP_INPUTEVENT_CLASS_MOUSE);
Don't need MOUSE for the time being.

[Łukasz Anforowicz, 2015-01-31 00:44:49]

I addressed the less important comments, but I am not sure what to do about
going back to a single div.  I still see javascript keydown/keyup events even
when calling Request*Filtering*InputEvents and later returning "true" from
HandleInputEvent.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
File remoting/tools/javascript_key_tester/chord_tracker.js (right):

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/chord_tracker.js:37:
this.endIfNoKeysPressed_();
On 2015/01/30 23:25:09, Jamie wrote:
> I don't think you need this here: a keydown event can't cause a transition
from
> keys pressed -> no keys pressed.

Done.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
File remoting/tools/javascript_key_tester/main.js (right):

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:8: * @param {number|undefined}
space
On 2015/01/30 23:25:09, Jamie wrote:
> {number=} is a simpler annotation, though you should rename it opt_space in
that
> case. You can them just omit the parameter instead of passing undefined
> explicitly.

Done.  Renamed to "opt_space" as well.  (are you sure this shouldn't be
"optSpace"?)

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:11: function
JsonifyJavascriptKeyEvent(event, eventName, space) {
On 2015/01/30 23:25:09, Jamie wrote:
> Function names in JS don't start with a capital letter (with the exception of
> ctors, since they effectively define types).

Done.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:42: var PNaClEvent = function(){
On 2015/01/30 23:25:09, Jamie wrote:
> Nit: Space after ()

Done.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:74: var pnaclEvent = event.data;
On 2015/01/30 23:25:09, Jamie wrote:
> Nit: Since this is effectively a cast (insofar as JS has such a concept), we
> conventionally annotate it as such:
> 
>   var pnaclEvent = /** @type {PNaclEvent} */ (event.data);

Done.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:103: var jsChordTracker = new
ChordTracker(jsLog);
On 2015/01/30 23:25:09, Jamie wrote:
> I think two chord trackers is probably overkill. Better just to have one,
> logging events to a single div, and perhaps reset it on focus/blur for the
PNaCl
> element. I don't think we need to track JS events when the plugin has focus
(it
> might be useful for applications where the plugin doesn't stop propagation of
> events, but the Chromoting plugin does do that, and I'd like this to be as
close
> as possible to that implementation).

I switched to using RequestFilteringInputEvents, but javascript events keep
firing.  Therefore I am leaving 2 separate divs and chord trackers for now.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
File remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc (right):

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/pnacl/remoting_key_tester.cc:19:
PP_INPUTEVENT_CLASS_MOUSE);
On 2015/01/30 23:25:09, Jamie wrote:
> Don't need MOUSE for the time being.

Done.

[Jamie, 2015-01-31 01:01:56]

lgtm

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
File remoting/tools/javascript_key_tester/main.js (right):

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:8: * @param {number|undefined}
space
On 2015/01/31 00:44:49, lukasza wrote:
> On 2015/01/30 23:25:09, Jamie wrote:
> > {number=} is a simpler annotation, though you should rename it opt_space in
> that
> > case. You can them just omit the parameter instead of passing undefined
> > explicitly.
> 
> Done.  Renamed to "opt_space" as well.  (are you sure this shouldn't be
> "optSpace"?)

opt_ as a prefix for optional parameters is a common convention, even though (or
maybe because) it goes against the convention for other names.

https://codereview.chromium.org/884703006/diff/20001/remoting/tools/javascrip...
remoting/tools/javascript_key_tester/main.js:103: var jsChordTracker = new
ChordTracker(jsLog);
On 2015/01/31 00:44:49, lukasza wrote:
> On 2015/01/30 23:25:09, Jamie wrote:
> > I think two chord trackers is probably overkill. Better just to have one,
> > logging events to a single div, and perhaps reset it on focus/blur for the
> PNaCl
> > element. I don't think we need to track JS events when the plugin has focus
> (it
> > might be useful for applications where the plugin doesn't stop propagation
of
> > events, but the Chromoting plugin does do that, and I'd like this to be as
> close
> > as possible to that implementation).
> 
> I switched to using RequestFilteringInputEvents, but javascript events keep
> firing.  Therefore I am leaving 2 separate divs and chord trackers for now.

I just tested with the Chromoting app and see the same behaviour, so I think
you're right--let's keep this for now.

[Łukasz Anforowicz, 2015-01-31 01:06:42]

lukasza@chromium.org changed reviewers:
+ teravest@chromium.org

[Łukasz Anforowicz, 2015-01-31 01:06:42]

Justin, could you please take a look to bless the new dependency on ppapi in
remoting/tools/javascript_key_tester/DEPS?

[teravest, 2015-02-02 15:54:06]

https://codereview.chromium.org/884703006/diff/60001/remoting/remoting_key_te...
File remoting/remoting_key_tester.gypi (right):

https://codereview.chromium.org/884703006/diff/60001/remoting/remoting_key_te...
remoting/remoting_key_tester.gypi:7: '../native_client/build/untrusted.gypi',
I believe you want to include build/common_untrusted.gypi here instead of
native_client/build/untrusted.gypi.

https://codereview.chromium.org/884703006/diff/60001/remoting/remoting_key_te...
remoting/remoting_key_tester.gypi:82: 'nacl_untrusted_build': 1,
Are you sure you need to use nacl_untrusted_build here? I'm surprised; other
rules that build pexes don't look quite like this.

https://codereview.chromium.org/884703006/diff/60001/remoting/remoting_key_te...
remoting/remoting_key_tester.gypi:92: 'dependencies': [
Do you need these dependencies and link flags?

[Łukasz Anforowicz, 2015-02-02 22:26:52]

Justin, I addressed your code review feedback - could you please take another
look?

https://codereview.chromium.org/884703006/diff/60001/remoting/remoting_key_te...
File remoting/remoting_key_tester.gypi (right):

https://codereview.chromium.org/884703006/diff/60001/remoting/remoting_key_te...
remoting/remoting_key_tester.gypi:7: '../native_client/build/untrusted.gypi',
On 2015/02/02 15:54:06, teravest wrote:
> I believe you want to include build/common_untrusted.gypi here instead of
> native_client/build/untrusted.gypi.

The include is needed to bring in tc_lib_dir_pnacl_newlib variable.

Including ../build/common_untrusted.gypi also works.

I've changed to ../build/common_untrusted.gypi here and also in
src/remoting/remoting_nacl.gypi

https://codereview.chromium.org/884703006/diff/60001/remoting/remoting_key_te...
remoting/remoting_key_tester.gypi:82: 'nacl_untrusted_build': 1,
On 2015/02/02 15:54:06, teravest wrote:
> Are you sure you need to use nacl_untrusted_build here? I'm surprised; other
> rules that build pexes don't look quite like this.

Fixed (removed this line).  I should have more carefully vet the gyp rules I
copied from remoting_nacl.gyp.

https://codereview.chromium.org/884703006/diff/60001/remoting/remoting_key_te...
remoting/remoting_key_tester.gypi:92: 'dependencies': [
On 2015/02/02 15:54:06, teravest wrote:
> Do you need these dependencies and link flags?

Without -lppapi_cpp I get: undefined reference to pp::Module::Init()

Without -lppapi_stub I get: libnacl.a: error: undefined reference to 'main'

I've removed other link_flags.


I am not sure about the dependencies.  Removing the dependencies doesn't hurt
anything (i.e. ninja says: "no work to do"), and I hope ninja would complain if
there were actual dependencies that were not reflected in the gyp file.  After
removing the dependencies rebuilding is triggered after making a minor edit in
ppapi/cpp/input_event.h, so I guess I can leave these dependencies out.

[teravest, 2015-02-02 22:33:37]

lgtm

https://codereview.chromium.org/884703006/diff/100001/remoting/remoting_key_t...
File remoting/remoting_key_tester.gypi (right):

https://codereview.chromium.org/884703006/diff/100001/remoting/remoting_key_t...
remoting/remoting_key_tester.gypi:91: 'link_flags': [
I feel like you should be able to depend on against
ppapi/native_client/native_client.gyp:ppapi_lib instead of needing to explicitly
have "-lppapi_stub" here, but I haven't tested that myself. 

However, I don't want to make you waste too much time on this, so I guess this
is fine for now.

[Łukasz Anforowicz, 2015-02-02 22:38:06]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-02-02 22:39:31]

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

[commit-bot: I haz the power, 2015-02-02 22:47:16]

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

[commit-bot: I haz the power, 2015-02-02 22:47:17]

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

[Łukasz Anforowicz, 2015-02-03 21:24:09]

I made all 3 remoting_key_tester* targets conditional on disable_nacl==0 and
disable_nacl_untrusted==0.  With unconditional definition of the targets, the
remoting_key_tester_pexe target would fail on android_arm64_dbg_recipe bot in
"gclient runhooks" step, saying that enable_x86_32 is undefined.

I also disabled remoting_key_tester_jscompile target altogether (making most of
its body conditional on "false").
- Even without other targets depending on remoting_key_tester_jscompile, this
  target would get built and we would get errors(i.e.  linux_chromium_rel_ng).
- jscompile target would fail on Windows complaining about no Java

[Łukasz Anforowicz, 2015-02-03 22:52:17]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-02-03 22:53:17]

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

[commit-bot: I haz the power, 2015-02-03 22:58:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-03 22:59:50]

Patchset 7 (id:??) landed as
https://crrev.com/31a6f70b8a22930cc749a67a90c6e7e3778f8466
Cr-Commit-Position: refs/heads/master@{#314424}