Eradicate keyIndentifier from devtools/*

Eradicate keyIndentifier from devtools/*

keyIdentifier is going to be deprecated; we need to stop using it.

No functional changes intended. A few instances remain with respect
to extensions and keyIdentifier. Those need to be handled so that
we don't break existing extensions.

BUG=607349
Committed: https://crrev.com/38625c390884254b2edb178e7e0ca1c301817002
Cr-Commit-Position: refs/heads/master@{#401423}

[dtapuska, 2016-06-09 21:41:09]

Description was changed from

==========
Eradicate keyIndentifier from devtools/*

keyIdentifier is going to be deprecated; we need to stop using it.

No functional changes intended. A few instances remain with respect
to extensions and keyIdentifier. Those need to be handled so that
we don't break existing extensions.

BUG=607349
==========

to

==========
Eradicate keyIndentifier from devtools/*

keyIdentifier is going to be deprecated; we need to stop using it.

No functional changes intended. A few instances remain with respect
to extensions and keyIdentifier. Those need to be handled so that
we don't break existing extensions.

BUG=607349
==========

[dtapuska, 2016-06-09 21:41:09]

dtapuska@chromium.org changed reviewers:
+ caseq@chromium.org

[dtapuska, 2016-06-09 21:41:17]

PTAL

[pfeldman, 2016-06-10 01:52:10]

could you hint us on the deprecation schedule?

[dtapuska, 2016-06-10 03:17:26]

On 2016/06/10 01:52:10, pfeldman wrote:
> could you hint us on the deprecation schedule?

It was deprecated in m52. To be removed in m53. This is why I did this work as I
wanted to remove it this week. But there are a few other issues with the ui/
code in chromium which is shared with iOS that doesn't support key.

[dgozman, 2016-06-10 07:39:40]

I think we have to add a polyfill to installBackwardsCompatibility in
Source/devtools/front_end/devtools.js, so that all deployed frontends do not
break.

[caseq, 2016-06-10 16:46:10]

On 2016/06/10 07:39:40, dgozman_slow wrote:
> I think we have to add a polyfill to installBackwardsCompatibility in
> Source/devtools/front_end/devtools.js, so that all deployed frontends do not
> break.

+1 to that -- we embed old front-end code when DevTools connect to an older
remote target, so we expect host platform to keep backward compatibility for the
duration of at least several releases -- so please add a polyfill to
devtools.js.

[dtapuska, 2016-06-10 17:42:37]

On 2016/06/10 16:46:10, caseq wrote:
> On 2016/06/10 07:39:40, dgozman_slow wrote:
> > I think we have to add a polyfill to installBackwardsCompatibility in
> > Source/devtools/front_end/devtools.js, so that all deployed frontends do not
> > break.
> 
> +1 to that -- we embed old front-end code when DevTools connect to an older
> remote target, so we expect host platform to keep backward compatibility for
the
> duration of at least several releases -- so please add a polyfill to
> devtools.js.

Ok I added a getter/setter care to take a look?

[caseq, 2016-06-10 18:59:11]

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/console-test.js
(right):

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/console-test.js:31: });
reformat to use indent of 4:
var event = new KeyboardEvent("keydown", {
    key: "Enter",
    ...
});

Also, I guess you can drop keyIdentifier?

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1011: function
keyCodeToKeyIdentifier(keyCode) {
{ => next line

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1012: switch(keyCode) {
nit: consider using a map?
var keyMap = new Map([[ 0x12, "Alt], ... ]);

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1137: get: function() {
{ => next line

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1138: if
(this.__keyIdentifier === undefined)
use symbols perhaps? I.e. var keyIdentifier = Symbol("keyIdentifier") outside
and use this[keyIdentifier] here instead.

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1142: set:
function(value) {
do we use it? looks like you can drop it.

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js
(right):

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:762: if
(WebInspector.KeyboardShortcut.eventHasCtrlOrMeta(event) && event.key === "z") {
// lower case Z key
so what happens when user has CAPS LOCK on?

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (right):

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:988: case "n":
should the switch be over event.code instead?

[dtapuska, 2016-06-10 20:25:44]

PTAL; I've converted to use the map.

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/console-test.js
(right):

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/console-test.js:31: });
On 2016/06/10 18:59:11, caseq wrote:
> reformat to use indent of 4:
> var event = new KeyboardEvent("keydown", {
>     key: "Enter",
>     ...
> });
> 
> Also, I guess you can drop keyIdentifier?

Done. I believe keyIdentifier is kind of needed until all the code that uses it
is ripped out of blink. If you inject fake commands into blink it still may
require keyIdentifier. But once all the changes in blink have landed then yes.

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1011: function
keyCodeToKeyIdentifier(keyCode) {
On 2016/06/10 18:59:11, caseq wrote:
> { => next line

Done.

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1012: switch(keyCode) {
On 2016/06/10 18:59:11, caseq wrote:
> nit: consider using a map?
> var keyMap = new Map([[ 0x12, "Alt], ... ]);

Done.

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1137: get: function() {
On 2016/06/10 18:59:11, caseq wrote:
> { => next line

Done.

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1138: if
(this.__keyIdentifier === undefined)
On 2016/06/10 18:59:11, caseq wrote:
> use symbols perhaps? I.e. var keyIdentifier = Symbol("keyIdentifier") outside
> and use this[keyIdentifier] here instead.

I removed this

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1142: set:
function(value) {
On 2016/06/10 18:59:11, caseq wrote:
> do we use it? looks like you can drop it.

Done.

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js
(right):

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:762: if
(WebInspector.KeyboardShortcut.eventHasCtrlOrMeta(event) && event.key === "z") {
// lower case Z key
On 2016/06/10 18:59:11, caseq wrote:
> so what happens when user has CAPS LOCK on?

Done.

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (right):

https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:988: case "n":
On 2016/06/10 18:59:11, caseq wrote:
> should the switch be over event.code instead?

So code will always give you the same key on the keyboard; no matter what the
layout is.. do you want your hotkeys to be that? Or keyboard mapping based?

Because I'd expect Ctrl-N to be Next on dvorak or qwerty; not really where the
actual key is on the keyboard.

[caseq, 2016-06-10 20:42:41]

On 2016/06/10 20:25:44, dtapuska wrote:


>
https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:988: case "n":
> On 2016/06/10 18:59:11, caseq wrote:
> > should the switch be over event.code instead?
> 
> So code will always give you the same key on the keyboard; no matter what the
> layout is.. do you want your hotkeys to be that? Or keyboard mapping based?
> 
> Because I'd expect Ctrl-N to be Next on dvorak or qwerty; not really where the
> actual key is on the keyboard.

Right, so wouldn't event.code be always "KeyN"?

[dtapuska, 2016-06-10 20:45:30]

On 2016/06/10 20:42:41, caseq wrote:
> On 2016/06/10 20:25:44, dtapuska wrote:
> 
> 
> >
>
https://codereview.chromium.org/2033403005/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:988: case "n":
> > On 2016/06/10 18:59:11, caseq wrote:
> > > should the switch be over event.code instead?
> > 
> > So code will always give you the same key on the keyboard; no matter what
the
> > layout is.. do you want your hotkeys to be that? Or keyboard mapping based?
> > 
> > Because I'd expect Ctrl-N to be Next on dvorak or qwerty; not really where
the
> > actual key is on the keyboard.
> 
> Right, so wouldn't event.code be always "KeyN"?

No; event.code is the keyboard as a US QWERTY keyboard.

If I press the USB hid key for KeyN in dvorak it produces 

code == KeyN
key == b

But in qwerty it produces
code == KeyN
key == n

[caseq, 2016-06-10 20:57:31]

On 2016/06/10 20:45:30, dtapuska wrote:

lgtm.

> No; event.code is the keyboard as a US QWERTY keyboard.

Ugh, I see, thanks for explaining!

[dtapuska, 2016-06-15 21:40:39]

dtapuska@chromium.org changed reviewers:
+ dgozman@chromium.org

[dtapuska, 2016-06-15 21:40:42]

dgozman@chromium.org: Please review changes in 

chrome/browser/devtools/*

[dgozman, 2016-06-16 06:55:01]

https://codereview.chromium.org/2033403005/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/2033403005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_window.cc:276:
event_data.SetString("keyIdentifier", event.keyIdentifier);
Do we have to pass this? I thought you are getting rid of it.

https://codereview.chromium.org/2033403005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2033403005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/devtools.js:232: * @param {{type:
string, key: string, code: string, keyIdentifier: string, keyCode: number,
modifiers: number}} event
Perhaps we can use the same keyCodeToKeyIdentifier function, and do not even
pass keyIdentifier from devtools_window.cc? This means we'll only have
keyIdentifier in this backwards-compatibility file.

[dtapuska, 2016-06-16 07:36:11]

https://codereview.chromium.org/2033403005/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/2033403005/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/devtools_window.cc:276:
event_data.SetString("keyIdentifier", event.keyIdentifier);
On 2016/06/16 06:55:00, dgozman_slow wrote:
> Do we have to pass this? I thought you are getting rid of it.

Right. I think this can now be removed.

https://codereview.chromium.org/2033403005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2033403005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/devtools.js:232: * @param {{type:
string, key: string, code: string, keyIdentifier: string, keyCode: number,
modifiers: number}} event
On 2016/06/16 06:55:00, dgozman_slow wrote:
> Perhaps we can use the same keyCodeToKeyIdentifier function, and do not even
> pass keyIdentifier from devtools_window.cc? This means we'll only have
> keyIdentifier in this backwards-compatibility file.

Done.

[dgozman, 2016-06-16 13:50:33]

lgtm with comment

https://codereview.chromium.org/2033403005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2033403005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/devtools.js:236:
this._dispatchOnInspectorFrontendAPI("keyEventUnhandled", [event]);
Note that we still have to add keyIdentifier here to support old frontends.
Let's reuse keyCodeToKeyIdentifier function.

[dtapuska, 2016-06-22 18:50:45]

https://codereview.chromium.org/2033403005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2033403005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/devtools.js:236:
this._dispatchOnInspectorFrontendAPI("keyEventUnhandled", [event]);
On 2016/06/16 13:50:33, dgozman wrote:
> Note that we still have to add keyIdentifier here to support old frontends.
> Let's reuse keyCodeToKeyIdentifier function.

Done.

[dtapuska, 2016-06-22 20:38:38]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-06-22 20:38:38]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2033403005/#ps140001
(title: "Call KeyCodeToKeyIdentifier for unhandle key events")

[commit-bot: I haz the power, 2016-06-22 20:39:24]

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

[commit-bot: I haz the power, 2016-06-22 21:58:57]

Description was changed from

==========
Eradicate keyIndentifier from devtools/*

keyIdentifier is going to be deprecated; we need to stop using it.

No functional changes intended. A few instances remain with respect
to extensions and keyIdentifier. Those need to be handled so that
we don't break existing extensions.

BUG=607349
==========

to

==========
Eradicate keyIndentifier from devtools/*

keyIdentifier is going to be deprecated; we need to stop using it.

No functional changes intended. A few instances remain with respect
to extensions and keyIdentifier. Those need to be handled so that
we don't break existing extensions.

BUG=607349
==========

[commit-bot: I haz the power, 2016-06-22 21:58:58]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-22 22:01:25]

Description was changed from

==========
Eradicate keyIndentifier from devtools/*

keyIdentifier is going to be deprecated; we need to stop using it.

No functional changes intended. A few instances remain with respect
to extensions and keyIdentifier. Those need to be handled so that
we don't break existing extensions.

BUG=607349
==========

to

==========
Eradicate keyIndentifier from devtools/*

keyIdentifier is going to be deprecated; we need to stop using it.

No functional changes intended. A few instances remain with respect
to extensions and keyIdentifier. Those need to be handled so that
we don't break existing extensions.

BUG=607349

Committed: https://crrev.com/38625c390884254b2edb178e7e0ca1c301817002
Cr-Commit-Position: refs/heads/master@{#401423}
==========

[commit-bot: I haz the power, 2016-06-22 22:01:27]

Patchset 8 (id:??) landed as
https://crrev.com/38625c390884254b2edb178e7e0ca1c301817002
Cr-Commit-Position: refs/heads/master@{#401423}