Convert Event#keyIdentifier (deprecated) to Event#key

Convert Event#keyIdentifier (deprecated) to Event#key

BUG=607349
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
TBR=rockot@chromium.org

Committed: https://crrev.com/89d8be5e8dac40428d4fc8dab1a7151cdc4bf78d
Cr-Commit-Position: refs/heads/master@{#403715}

[dtapuska, 2016-06-29 16:56:59]

dtapuska@chromium.org changed reviewers:
+ alemate@chromium.org, dbeam@chromium.org, fukino@chromium.org,
rockot@chromium.org

[dtapuska, 2016-06-29 16:57:00]

PTAL.

[Dan Beam, 2016-06-29 18:28:09]

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:375: 
nit: remove extra \n

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:381: if (!('key' in KeyboardEvent.prototype)) {
I think it's cool that you're doing runtime feature detection, BUT, we still
ship this extra code with Chrome for no reason everywhere but iOS :(

can we wrap this in a

  <if expr="is_ios">

and maybe keep the feature detection if they implement it or something?  or
maybe assert(!('key' in KeyboardEvent.prototype')); so that we'll know when they
do implement it (or something that warns rather than throws would probably be
better)?

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:382: var keyGetter = {
can we just inline keyGetter?  -1 global

Object.defineProperty(KeyboardEvent.prototype, 'key', {
  get: function() {
    ...
  },
});

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:386: return String.fromCharCode(this.keyCode);
indent off

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:393: return result;
indent off

[dtapuska, 2016-06-29 20:09:02]

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:375: 
On 2016/06/29 18:28:09, Dan Beam wrote:
> nit: remove extra \n

Done.

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:381: if (!('key' in KeyboardEvent.prototype)) {
On 2016/06/29 18:28:09, Dan Beam wrote:
> I think it's cool that you're doing runtime feature detection, BUT, we still
> ship this extra code with Chrome for no reason everywhere but iOS :(
> 
> can we wrap this in a
> 
>   <if expr="is_ios">
> 
> and maybe keep the feature detection if they implement it or something?  or
> maybe assert(!('key' in KeyboardEvent.prototype')); so that we'll know when
they
> do implement it (or something that warns rather than throws would probably be
> better)?

Done.

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:382: var keyGetter = {
On 2016/06/29 18:28:09, Dan Beam wrote:
> can we just inline keyGetter?  -1 global
> 
> Object.defineProperty(KeyboardEvent.prototype, 'key', {
>   get: function() {
>     ...
>   },
> });

Done.

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:386: return String.fromCharCode(this.keyCode);
On 2016/06/29 18:28:09, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/2104103002/diff/40001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:393: return result;
On 2016/06/29 18:28:09, Dan Beam wrote:
> indent off

Done.

[Alexander Alekseev, 2016-06-29 23:13:04]

alemate@chromium.org changed reviewers:
+ dzhioev@chromium.org

[Alexander Alekseev, 2016-06-29 23:13:05]

+dzhioev as he known this code better.

[Dan Beam, 2016-06-29 23:34:17]

lgtm w/nits and question

https://codereview.chromium.org/2104103002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/downloads.html (right):

https://codereview.chromium.org/2104103002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/downloads.html:33: <command
id="find-command" shortcut="Ctrl|f"><!-- Ctrl+F -->
you can probably remove all these HTML comments now (the shortcut="" itself
looks clear enough)

https://codereview.chromium.org/2104103002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/inline_editable_list_test.html (right):

https://codereview.chromium.org/2104103002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/inline_editable_list_test.html:114: var event = new
KeyboardEvent('keydown', {bubbles: true, cancelable: true, key: key});
this should wrap at 80 cols

https://codereview.chromium.org/2104103002/diff/60001/ui/login/bubble.js
File ui/login/bubble.js (right):

https://codereview.chromium.org/2104103002/diff/60001/ui/login/bubble.js#newc...
ui/login/bubble.js:22: var KeyCodes = {
nit: KeyCodes => Keys

https://codereview.chromium.org/2104103002/diff/60001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/command.js (right):

https://codereview.chromium.org/2104103002/diff/60001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/command.js:127: *   "Delete Backspace-Meta" for
Delete and Command Backspace
shouldn't these - be changed to |?

https://codereview.chromium.org/2104103002/diff/60001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_item.js (right):

https://codereview.chromium.org/2104103002/diff/60001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_item.js:171: if (ident.length != 1) {
what's going on here?

[dtapuska, 2016-06-30 14:16:50]

https://codereview.chromium.org/2104103002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_downloads/downloads.html (right):

https://codereview.chromium.org/2104103002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_downloads/downloads.html:33: <command
id="find-command" shortcut="Ctrl|f"><!-- Ctrl+F -->
On 2016/06/29 23:34:17, Dan Beam wrote:
> you can probably remove all these HTML comments now (the shortcut="" itself
> looks clear enough)

Done.

https://codereview.chromium.org/2104103002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/inline_editable_list_test.html (right):

https://codereview.chromium.org/2104103002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/inline_editable_list_test.html:114: var event = new
KeyboardEvent('keydown', {bubbles: true, cancelable: true, key: key});
On 2016/06/29 23:34:17, Dan Beam wrote:
> this should wrap at 80 cols

Done.

https://codereview.chromium.org/2104103002/diff/60001/ui/login/bubble.js
File ui/login/bubble.js (right):

https://codereview.chromium.org/2104103002/diff/60001/ui/login/bubble.js#newc...
ui/login/bubble.js:22: var KeyCodes = {
On 2016/06/29 23:34:17, Dan Beam wrote:
> nit: KeyCodes => Keys

Done.

https://codereview.chromium.org/2104103002/diff/60001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/command.js (right):

https://codereview.chromium.org/2104103002/diff/60001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/command.js:127: *   "Delete Backspace-Meta" for
Delete and Command Backspace
On 2016/06/29 23:34:17, Dan Beam wrote:
> shouldn't these - be changed to |?

Done.

https://codereview.chromium.org/2104103002/diff/60001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_item.js (right):

https://codereview.chromium.org/2104103002/diff/60001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_item.js:171: if (ident.length != 1) {
On 2016/06/29 23:34:17, Dan Beam wrote:
> what's going on here?

So previously it would check if started with index U+ signifying that it was an
individual character. Now we just check the length to see if it is a single
character. Alt, Backspace, Ctrl, Enter, Meta, Shift, Space are all resources in
the cr menu item bundle.

But 'C' or 'X' or 'Z' are not in the resource bundle and the key is used
directly.

[fukino, 2016-06-30 19:31:04]

file_manager lgtm. Thank you!

[dtapuska, 2016-06-30 20:06:17]

On 2016/06/30 19:31:04, fukino wrote:
> file_manager lgtm. Thank you!

dzhioev@ ping; are you able to have a quick review of your ui/login/*

[dzhioev (left Google), 2016-07-01 00:41:05]

On 2016/06/30 20:06:17, dtapuska wrote:
> On 2016/06/30 19:31:04, fukino wrote:
> > file_manager lgtm. Thank you!
> 
> dzhioev@ ping; are you able to have a quick review of your ui/login/*

ui/login LGTM

[dtapuska, 2016-07-01 00:45:49]

Description was changed from

==========
Convert Event#keyIdentifier (deprecated) to Event#key

BUG=607349
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Convert Event#keyIdentifier (deprecated) to Event#key

BUG=607349
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
TBR=rockot@chromium.org
==========

[dtapuska, 2016-07-01 00:45:59]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-07-01 00:45:59]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2104103002/#ps80001
(title: "Fix nits")

[commit-bot: I haz the power, 2016-07-01 00:46:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 00:58:41]

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

[commit-bot: I haz the power, 2016-07-01 00:58:42]

Try jobs failed on following builders:
  closure_compilation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Dan Beam, 2016-07-01 01:04:29]

https://codereview.chromium.org/2104103002/diff/80001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/2104103002/diff/80001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:382:
Object.defineProperty(KeyboardEvent.prototype, 'key', {
/** @this {KeyboardEvent} */

[dtapuska, 2016-07-04 13:18:48]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-07-04 13:18:49]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
fukino@chromium.org, dzhioev@chromium.org
Link to the patchset: https://codereview.chromium.org/2104103002/#ps100001
(title: "Fix closure error")

[commit-bot: I haz the power, 2016-07-04 13:19:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 14:03:54]

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

[commit-bot: I haz the power, 2016-07-04 14:03:55]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dtapuska, 2016-07-04 19:09:47]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2016-07-04 19:10:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2016-07-04 20:07:07]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-07-04 20:07:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
fukino@chromium.org, dzhioev@chromium.org
Link to the patchset: https://codereview.chromium.org/2104103002/#ps120001
(title: "Patch file manager test")

[commit-bot: I haz the power, 2016-07-04 20:07:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 21:07:28]

Description was changed from

==========
Convert Event#keyIdentifier (deprecated) to Event#key

BUG=607349
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
TBR=rockot@chromium.org
==========

to

==========
Convert Event#keyIdentifier (deprecated) to Event#key

BUG=607349
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
TBR=rockot@chromium.org
==========

[commit-bot: I haz the power, 2016-07-04 21:07:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-04 21:08:47]

Description was changed from

==========
Convert Event#keyIdentifier (deprecated) to Event#key

BUG=607349
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
TBR=rockot@chromium.org
==========

to

==========
Convert Event#keyIdentifier (deprecated) to Event#key

BUG=607349
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
TBR=rockot@chromium.org

Committed: https://crrev.com/89d8be5e8dac40428d4fc8dab1a7151cdc4bf78d
Cr-Commit-Position: refs/heads/master@{#403715}
==========

[commit-bot: I haz the power, 2016-07-04 21:08:48]

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