Support standard keyboard keys emulated by the braille drivers.

Support standard keyboard keys emulated by the braille drivers.

This plumbs through keys like backspace and enter and arrow keys.
It also provides Unicode characters for displays with query keyboards etc.

BUG=310285
R=dtseng@chromium.org, kalman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273171

[Peter Lundblad, 2014-05-23 22:22:46]

@dtseng: one step closer to the backspace key! ;)

@kalman: for the api part.
NOTE: This is a private api and nothing is using that part of the api, so the
name change doesn't break anything and makes things clearer.

[not at google - send to devlin, 2014-05-23 22:26:19]

idl lgtm assuming my comment is ignorant

https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/braille_display_private.idl (right):

https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/braille_display_private.idl:34: DOMString?
standardKeyCode;
aren't key codes integers?

[Peter Lundblad, 2014-05-23 22:53:42]

kalman@chromium.org writes:
> idl lgtm assuming my comment is ignorant
> 
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
> File chrome/common/extensions/api/braille_display_private.idl (right):
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
> chrome/common/extensions/api/braille_display_private.idl:34: DOMString?
> standardKeyCode;
> aren't key codes integers?
This is in alignment with
http://www.w3.org/TR/uievents/#keyboard-event-interface, similar to what the
input.ime API does.  It is the modifier-state and locale-independent physical
key code as a nice little string that you can actually understand by looking at
it;)

Thanks,
//Peter

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

-- 

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

[David Tseng, 2014-05-27 21:29:31]

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc
(right):

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc:227:
// With anticipation for the first emoji-enabled braille display.
:)

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc
(right):

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:21:
// Maximum Laitn 1 character keyboard symbol.
Latin

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:38:
event->shift_key.reset(new bool(true));
Do we get any other modifiers?

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:63:
code_string = "Backspace";
Where do these strings come from?

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:105:
return;
DCHECK?

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:142:
event->braille_dots.reset(new int(argument & kAllDots));
When would |argument| have bits not in kAllDots set? Is that an error?

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.h
(right):

https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Any need to add a unit tests for this file (or is it already covered by existing
tests since you moved this function)?

https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/braille_display_private.idl (right):

https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/braille_display_private.idl:34: DOMString?
standardKeyCode;
Had same question as kalman.

https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/braille_display_private.idl:37: DOMString?
standardKeyChar;
Do we need both of these members or can we make the API client do the
conversion? (like in the DOM).

[David Tseng, 2014-05-27 21:32:29]

https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/braille_display_private.idl (right):

https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/braille_display_private.idl:37: DOMString?
standardKeyChar;
Also, is there a non-standard variant? (i.e. can we remove standard from the
name)?

[Peter Lundblad, 2014-05-27 23:08:50]

dtseng@chromium.org writes:
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
> File
>
chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
>
chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc:227:
> // With anticipation for the first emoji-enabled braille display.
> :)
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
> File
> chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc
> (right):
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
>
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:21:
> // Maximum Laitn 1 character keyboard symbol.
> Latin

Done.

>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
>
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:38:
> event->shift_key.reset(new bool(true));
> Do we get any other modifiers?

Maybe at some point in the future.  brlapi currently has these three and
I'd like a way to add the space key to these.  Note that currently all of them
are not
mapped on many displays, but we can change the keyboard mappings in brltty.

>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
>
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:63:
> code_string = "Backspace";
> Where do these strings come from?

http://www.w3.org/TR/uievents/
Added a ref to the idl file and a ref to
ui/events/keycodes/dom4/keycode_converter_data.cc where we
have these listed already.  Unfortunately, we can't just use them, so we have
to duplicate them here.

>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
>
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:105:
> return;
> DCHECK?

I don't think a newer version of brltty supporting another key should be fatal
even in debug builds.  We can add support for such key if it happens.

>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
>
chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.cc:142:
> event->braille_dots.reset(new int(argument & kAllDots));
> When would |argument| have bits not in kAllDots set? Is that an error?

There's a DOTC bit that could be set representing space in some versions of
brltty.  In general, if a future version adds bits, we shouldn't blindly
pass them through, but we should also not fail.

>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
> File
> chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.h
> (right):
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/braille_display_private/brlapi_keycode_map.h:1:
> // Copyright 2014 The Chromium Authors. All rights reserved.
> Any need to add a unit tests for this file (or is it already covered by
> existing tests since you moved this function)?

There's a bunch of api test cases added in this CL.  I thought that would cover
this.  What do you think?

>
https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
> File chrome/common/extensions/api/braille_display_private.idl (right):
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
> chrome/common/extensions/api/braille_display_private.idl:34: DOMString?
> standardKeyCode;
> Had same question as kalman.

...and were you satisfied by the response?  If not, let me know.

>
https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
> chrome/common/extensions/api/braille_display_private.idl:37: DOMString?
> standardKeyChar;
> Do we need both of these members or can we make the API client do the
> conversion? (like in the DOM).

It's non-trivial to go from keyCode to character.  keyCode identifies a
physical
key, independent of keyboard layout and modifier state.  I don't think we
should
replicate that functionality anywhere.

Thanks,
//Peter

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

[pplundblad_chromium.org, 2014-05-27 23:17:37]

dtseng@chromium.org writes:
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
> File chrome/common/extensions/api/braille_display_private.idl (right):
> 
>
https://codereview.chromium.org/297833015/diff/1/chrome/common/extensions/api...
> chrome/common/extensions/api/braille_display_private.idl:37: DOMString?
> standardKeyChar;
> Also, is there a non-standard variant? (i.e. can we remove standard from
> the name)?
> 

I guess braille commands are relatively non-standard compared to
a 'pc' keyboard.  The enum value is 'standard_key', I didn't want to call
it just 'key', hence the prefix on these properties as well.  I am not
particularly
happy about 'standard', so if you have any better suggestion, I am open for
it;)

Thanks,
//Peter

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

[David Tseng, 2014-05-27 23:49:58]

lgtm

[Peter Lundblad, 2014-05-28 00:00:17]

The CQ bit was checked by plundblad@chromium.org

[commit-bot: I haz the power, 2014-05-28 00:01:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/plundblad@chromium.org/297833015/20001

[commit-bot: I haz the power, 2014-05-28 03:20:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-28 04:25:25]

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

[commit-bot: I haz the power, 2014-05-28 04:25:25]

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

[Peter Lundblad, 2014-05-28 04:51:19]

The CQ bit was checked by plundblad@chromium.org

[commit-bot: I haz the power, 2014-05-28 04:51:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/plundblad@chromium.org/297833015/20001

[commit-bot: I haz the power, 2014-05-28 06:04:31]

Change committed as 273171