Add experimental Support for DOM3 KeyboardEvent key value

Add experimental Support for DOM3 KeyboardEvent key value

DOM KeyboardEvents has a new field |key| which gives value as per the DOM3 KeyboardEvent key specification.

It uses embedder API, which fetches |key| value based on a native key from the chromium side.

KeyboardEvent.idl is updated to include |key| field as per the spec.

Also removes the URL order change introduced in WebPluginContainerTest.cpp for patch 663523002.

Intent to ship and implement: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/keyboardevent/blink-dev/Znyl5a96oDc/_kSn1rQ55isJ

R=Wez, rbyers@chromium.org, mkwst@chromium.org, pfeldman@chromium.org
BUG=227231
TEST=fast/events/keyboardevent-key.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194806

[Habib Virji, 2015-02-18 14:18:29]

habib.virji@samsung.com changed reviewers:
+ bokan@chromium.org, dgozman@chromium.org, garykac@chromium.org

[Habib Virji, 2015-02-18 14:18:30]

Support for DOM3 Key event, replicates similar to DOM code event, landed a week
back.

[dgozman, 2015-02-18 14:27:48]

https://codereview.chromium.org/933323002/diff/1/Source/devtools/front_end/sc...
File Source/devtools/front_end/screencast/ScreencastView.js (right):

https://codereview.chromium.org/933323002/diff/1/Source/devtools/front_end/sc...
Source/devtools/front_end/screencast/ScreencastView.js:282:
this._target.inputAgent().dispatchKeyEvent(type, this._modifiersForEvent(event),
event.timeStamp / 1000, text, text ? text.toLowerCase() : undefined,
While you are here, please change this code to:

this._target.inputAgent().invoke_dispatchKeyEvent({
    type: ...,
    modifiers: ...,
    timestamp: ...,
    text: ...,
    unmodifiedText: ...,
    keyIdentifier: ...,
    code: ...,
    key: ...,
    windowsVirtualKeyCode: ...,
    nativeVirtualKeyCode: ...,
    autoRepeat: ...,
    isKeypad: ...,
    isSystemKey: ...});

[Habib Virji, 2015-02-18 15:33:46]

https://codereview.chromium.org/933323002/diff/1/Source/devtools/front_end/sc...
File Source/devtools/front_end/screencast/ScreencastView.js (right):

https://codereview.chromium.org/933323002/diff/1/Source/devtools/front_end/sc...
Source/devtools/front_end/screencast/ScreencastView.js:282:
this._target.inputAgent().dispatchKeyEvent(type, this._modifiersForEvent(event),
event.timeStamp / 1000, text, text ? text.toLowerCase() : undefined,
On 2015/02/18 14:27:48, dgozman wrote:
> While you are here, please change this code to:
> 
> this._target.inputAgent().invoke_dispatchKeyEvent({
>     type: ...,
>     modifiers: ...,
>     timestamp: ...,
>     text: ...,
>     unmodifiedText: ...,
>     keyIdentifier: ...,
>     code: ...,
>     key: ...,
>     windowsVirtualKeyCode: ...,
>     nativeVirtualKeyCode: ...,
>     autoRepeat: ...,
>     isKeypad: ...,
>     isSystemKey: ...});

Done.

[Habib Virji, 2015-03-03 17:25:59]

@bokan/rbyers/wez: are these changes okay. There is missing functionality on
chromium side to support DomKey and have uploaded patch for the same. Please
review blink side changes if they are okay.

[Wez, 2015-03-04 23:00:26]

https://codereview.chromium.org/933323002/diff/20001/Source/core/events/Keybo...
File Source/core/events/KeyboardEvent.cpp (right):

https://codereview.chromium.org/933323002/diff/20001/Source/core/events/Keybo...
Source/core/events/KeyboardEvent.cpp:102: , m_isAutoRepeat(key.isAutoRepeat())
Not specific to this CL but perhaps rbyers@ can comment - can we use constructor
delegation now in Blink, to simplify these?

https://codereview.chromium.org/933323002/diff/20001/Source/core/events/Keybo...
File Source/core/events/KeyboardEvent.h (right):

https://codereview.chromium.org/933323002/diff/20001/Source/core/events/Keybo...
Source/core/events/KeyboardEvent.h:56: static
PassRefPtrWillBeRawPtr<KeyboardEvent> create(const AtomicString& type, const
KeyboardEventInit& initializer)
Do we need to update this creator function as well?

https://codereview.chromium.org/933323002/diff/20001/Source/core/events/Keybo...
File Source/core/events/KeyboardEvent.idl (right):

https://codereview.chromium.org/933323002/diff/20001/Source/core/events/Keybo...
Source/core/events/KeyboardEvent.idl:30: [RuntimeEnabled=KeyboardEventKey]
readonly attribute DOMString     key;
nit: Suggest lining up these two lines, for readability (i.e. aligning
"readonly", etc)

https://codereview.chromium.org/933323002/diff/20001/Source/devtools/protocol...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/933323002/diff/20001/Source/devtools/protocol...
Source/devtools/protocol.json:4304: { "name": "key", "type": "string",
"optional": true, "description": "Unique DOM defined string value based on the
current state of all active keys and key input modes  (e.g., 'Shift') (default:
\"\")." },
I'd suggest rephrasing this:

"Unique DOM defined string value describing the meaning of the key in the
context of active modifiers, keyboard layout, etc (e.g. 'AltGr')" ...

https://codereview.chromium.org/933323002/diff/20001/Source/web/tests/WebInpu...
File Source/web/tests/WebInputEventConversionTest.cpp (right):

https://codereview.chromium.org/933323002/diff/20001/Source/web/tests/WebInpu...
Source/web/tests/WebInputEventConversionTest.cpp:795: const std::string
baseURL("http://www.test6.com/");
Why did these change?

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:621: // This method converts from the supplied DOM
key enum to the
"... from the supplied embedder |key| enum to the corresponding DOM |key|
string."

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:623: // based on the value defined in
ui/events/keycodes/dom3/dom_key_data.h.
That header is a Chromium header - at the level if this interface there's no
need to specify details of the embedder (i.e. Chromium)'s values - they are
opaque to Blink.

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:624: // Returns null string, if DOM key value is not
found.
Do you mean returns null if the |domKey| value is not recognized? Or returns an
empty string?

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:627: // This method converts from the suppled DOM Key
value to the
nit: Capitalization of "Key" vs "key" in the previous comment.

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:629: // ui/events/keycodes/dom3/dom_key_data.h.
See above; the returned domEnum values are opaque to Blink.

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:630: // Returns 0, if DOM key enum is not found.
nit: Remove the comma

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:631: virtual int domEnumFromKeyString(const
WebString& keyString) { return 0; }
domKeyEnumFromString

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent.h
File public/web/WebInputEvent.h (right):

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent...
public/web/WebInputEvent.h:250: // The DOM key enum of the key pressed as passed
by the embedder. DOM
This (and the above domCode field) are embedder enum values representing the DOM
|key| and |code| of the key. As noted elsewhere, we shouldn't refer to the
Chromium code locations to document these - just mention that the values are
opaque to Blink, controlled purely by the embedder (i.e. by Chromium).

[Habib Virji, 2015-03-09 15:14:27]

Thanks Wez, updated comments as below suggested. Only comment not addressed is
chromium path specified and that's due to comment received during Dom |code| CL.

https://codereview.chromium.org/933323002/diff/20001/Source/core/events/Keybo...
File Source/core/events/KeyboardEvent.idl (right):

https://codereview.chromium.org/933323002/diff/20001/Source/core/events/Keybo...
Source/core/events/KeyboardEvent.idl:30: [RuntimeEnabled=KeyboardEventKey]
readonly attribute DOMString     key;
On 2015/03/04 23:00:25, Wez wrote:
> nit: Suggest lining up these two lines, for readability (i.e. aligning
> "readonly", etc)

Done.

https://codereview.chromium.org/933323002/diff/20001/Source/devtools/protocol...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/933323002/diff/20001/Source/devtools/protocol...
Source/devtools/protocol.json:4304: { "name": "key", "type": "string",
"optional": true, "description": "Unique DOM defined string value based on the
current state of all active keys and key input modes  (e.g., 'Shift') (default:
\"\")." },
On 2015/03/04 23:00:25, Wez wrote:
> I'd suggest rephrasing this:
> 
> "Unique DOM defined string value describing the meaning of the key in the
> context of active modifiers, keyboard layout, etc (e.g. 'AltGr')" ...

Done.

https://codereview.chromium.org/933323002/diff/20001/Source/web/tests/WebInpu...
File Source/web/tests/WebInputEventConversionTest.cpp (right):

https://codereview.chromium.org/933323002/diff/20001/Source/web/tests/WebInpu...
Source/web/tests/WebInputEventConversionTest.cpp:795: const std::string
baseURL("http://www.test6.com/");
In my previous CL i altered the sequence. So correcting in this CL.

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:621: // This method converts from the supplied DOM
key enum to the
On 2015/03/04 23:00:26, Wez wrote:
> "... from the supplied embedder |key| enum to the corresponding DOM |key|
> string."

Done.

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:623: // based on the value defined in
ui/events/keycodes/dom3/dom_key_data.h.
On 2015/03/04 23:00:25, Wez wrote:
In dom |code| CL, rbyers had suggested to specify where enum value are defined:
https://codereview.chromium.org/663523002/diff/220001/public/web/WebInputEven....
That's why included the path.

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:624: // Returns null string, if DOM key value is not
found.
On 2015/03/04 23:00:26, Wez wrote:
> Do you mean returns null if the |domKey| value is not recognized? Or returns
an
> empty string?

It is empty, corrected.

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:627: // This method converts from the suppled DOM Key
value to the
On 2015/03/04 23:00:25, Wez wrote:
> nit: Capitalization of "Key" vs "key" in the previous comment.

Done.

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:629: // ui/events/keycodes/dom3/dom_key_data.h.
On 2015/03/04 23:00:26, Wez wrote:
> See above; the returned domEnum values are opaque to Blink.

Please suggest, rbyers did mention to include even if it is on chromium side. Is
it okay to keep it or should i remove it?

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:630: // Returns 0, if DOM key enum is not found.
On 2015/03/04 23:00:25, Wez wrote:
> nit: Remove the comma

Done.

https://codereview.chromium.org/933323002/diff/20001/public/platform/Platform...
public/platform/Platform.h:631: virtual int domEnumFromKeyString(const
WebString& keyString) { return 0; }
On 2015/03/04 23:00:26, Wez wrote:
> domKeyEnumFromString

Done.

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent.h
File public/web/WebInputEvent.h (right):

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent...
public/web/WebInputEvent.h:250: // The DOM key enum of the key pressed as passed
by the embedder. DOM
As previously stated, it was added as rbyers suggested. Please suggest if to
remove.

[bokan, 2015-03-09 15:22:48]

lgtm

[Habib Virji, 2015-03-13 09:31:24]

@rbyers: can you have a look if changes are okay?

[Habib Virji, 2015-04-20 14:20:17]

@Wez, rbyers: Can you please suggest how to proceed on this CL?

[Rick Byers, 2015-04-20 19:49:47]

Sorry I missed your previous ping!  This seems reasonable to me.  LGTM with a
couple minor questions.

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent.h
File public/web/WebInputEvent.h (right):

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent...
public/web/WebInputEvent.h:250: // The DOM key enum of the key pressed as passed
by the embedder. DOM
On 2015/03/09 15:14:27, Habib Virji wrote:
> As previously stated, it was added as rbyers suggested. Please suggest if to
> remove. 

I don't have a strong preference.  It's not like there is really any hard
separation between chromium and blink anymore (especially with the repos
merging) so I don't see any realistic downside to specifying chromium paths to
help developers understand what's going on (DEPS rules will prevent you from
including chromium headers into blink, so I don't think there's a risk of
developers doing dumb things with this knowledge).  But if wez@ feels strongly,
then I'm OK being vague as well.

https://codereview.chromium.org/933323002/diff/40001/Source/web/WebInputEvent...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/933323002/diff/40001/Source/web/WebInputEvent...
Source/web/WebInputEventConversion.cpp:322: m_key =
Platform::current()->domKeyStringFromEnum(e.domKey);
Wow, I'm surprised we do this every time we create a KeyboardEvent.  I assumed
the point of calling back into the embedder to map between enums and strings was
that we'd do it lazily - only when the application actually requested the
string.  But since this is the existing pattern for m_code, and I don't think
keyboard events are particularly perf sensitive (i.e. don't send them at very
high rates), then keeping it as simple/consistent as possible is certainly best.

https://codereview.chromium.org/933323002/diff/40001/Source/web/tests/WebInpu...
File Source/web/tests/WebInputEventConversionTest.cpp (right):

https://codereview.chromium.org/933323002/diff/40001/Source/web/tests/WebInpu...
Source/web/tests/WebInputEventConversionTest.cpp:734: const std::string
baseURL("http://www.test5.com/");
Why are you making these baseURL changes?

https://codereview.chromium.org/933323002/diff/40001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/933323002/diff/40001/public/platform/Platform...
public/platform/Platform.h:623: virtual WebString domKeyStringFromEnum(int
domKey) { return WebString(); }
Since this feature is 'experimental' I assume it's fine to land (and potentially
ship) the blink side before the chromium side lands, right?  The feature will be
somewhat broken without the chromium CL, but only when pages attempt to use
strings, right?  That's probably fine, but we should aim to have the chromium
side landed in the same milestone (i.e. both blink and chromium sides need to
land before M44 branch on May 15th).  A bunch of users do run with
--enable-experimental-web-platform-features and so this could trigger bugs on
webpages that are surprised to see empty strings.

[Habib Virji, 2015-04-29 16:00:50]

Thanks Rick for reviewing. Apologise for a late response. This time i am bit
late in responding as catching up with the work I was doing few months back.
There is no code update except updated to the latest master and have replied to
your below queries.

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent.h
File public/web/WebInputEvent.h (right):

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent...
public/web/WebInputEvent.h:250: // The DOM key enum of the key pressed as passed
by the embedder. DOM
@wez: Do you have a suggestion on this?

https://codereview.chromium.org/933323002/diff/40001/Source/web/WebInputEvent...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/933323002/diff/40001/Source/web/WebInputEvent...
Source/web/WebInputEventConversion.cpp:322: m_key =
Platform::current()->domKeyStringFromEnum(e.domKey);
I agree this is not the best use of the embedded API. I will look into alternate
way of setting this value. Will like to land this CL as things to start working.

https://codereview.chromium.org/933323002/diff/40001/Source/web/tests/WebInpu...
File Source/web/tests/WebInputEventConversionTest.cpp (right):

https://codereview.chromium.org/933323002/diff/40001/Source/web/tests/WebInpu...
Source/web/tests/WebInputEventConversionTest.cpp:734: const std::string
baseURL("http://www.test5.com/");
This is rectifying mistake i did in my previous push where I modified by mistake
the URL.

https://codereview.chromium.org/933323002/diff/40001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/933323002/diff/40001/public/platform/Platform...
public/platform/Platform.h:623: virtual WebString domKeyStringFromEnum(int
domKey) { return WebString(); }
Yes Rick, i will be working this week to push all the changes related to DomKey
. If successful will send a patch around May 15, to remove experimental flag.

[Rick Byers, 2015-04-29 17:34:26]

rbyers@chromium.org changed reviewers:
+ aelias@chromium.org, vollick@chromium.org

[Rick Byers, 2015-04-29 17:34:27]

Thanks, still LGTM

+aelias for Source/web OWNERS
+vollick for Source/platform OWNERS
+dgozman for Source/devtools OWNERS

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent.h
File public/web/WebInputEvent.h (right):

https://codereview.chromium.org/933323002/diff/20001/public/web/WebInputEvent...
public/web/WebInputEvent.h:250: // The DOM key enum of the key pressed as passed
by the embedder. DOM
On 2015/04/29 16:00:50, Habib Virji wrote:
> @wez: Do you have a suggestion on this?

Since we haven't heard from wez@ on this yet, I suggest you just land it the way
it is.  But if you prefer you can take the comment out.  It's a minor issue, not
worth delaying your CL for further.

https://codereview.chromium.org/933323002/diff/40001/Source/web/WebInputEvent...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/933323002/diff/40001/Source/web/WebInputEvent...
Source/web/WebInputEventConversion.cpp:322: m_key =
Platform::current()->domKeyStringFromEnum(e.domKey);
On 2015/04/29 16:00:50, Habib Virji wrote:
> I agree this is not the best use of the embedded API. I will look into
alternate
> way of setting this value. Will like to land this CL as things to start
working.

Yes, please do.  Maybe add a TODO comment here referencing a new bug that you
file saying to explore deferring this call.  This may be a case of premature
optimization though - if you can show (eg. using chrome://tracing) that the cost
is trivial, then it may not be worth changing (depending how hard/ugly it is to
fix).

https://codereview.chromium.org/933323002/diff/40001/Source/web/tests/WebInpu...
File Source/web/tests/WebInputEventConversionTest.cpp (right):

https://codereview.chromium.org/933323002/diff/40001/Source/web/tests/WebInpu...
Source/web/tests/WebInputEventConversionTest.cpp:734: const std::string
baseURL("http://www.test5.com/");
On 2015/04/29 16:00:50, Habib Virji wrote:
> This is rectifying mistake i did in my previous push where I modified by
mistake
> the URL. 

Do you mean a previous CL that you landed?  If so mention/link to it in your CL
description.  If you mean a previous patch set for this CL, then something has
gone wrong - I'm looking only at the diff of the latest patch set to what's in
the tree today.

[aelias_OOO_until_Jul13, 2015-04-29 22:07:09]

Source/web lgtm

[Habib Virji, 2015-04-30 09:07:24]

Thanks Rick, added bug id and updated CL description.

https://codereview.chromium.org/933323002/diff/40001/Source/web/WebInputEvent...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/933323002/diff/40001/Source/web/WebInputEvent...
Source/web/WebInputEventConversion.cpp:322: m_key =
Platform::current()->domKeyStringFromEnum(e.domKey);
On 2015/04/29 17:34:27, Rick Byers wrote:
> On 2015/04/29 16:00:50, Habib Virji wrote:
> > I agree this is not the best use of the embedded API. I will look into
> alternate
> > way of setting this value. Will like to land this CL as things to start
> working.
> 
> Yes, please do.  Maybe add a TODO comment here referencing a new bug that you
> file saying to explore deferring this call.  This may be a case of premature
> optimization though - if you can show (eg. using chrome://tracing) that the
cost
> is trivial, then it may not be worth changing (depending how hard/ugly it is
to
> fix).

Done.

https://codereview.chromium.org/933323002/diff/40001/Source/web/tests/WebInpu...
File Source/web/tests/WebInputEventConversionTest.cpp (right):

https://codereview.chromium.org/933323002/diff/40001/Source/web/tests/WebInpu...
Source/web/tests/WebInputEventConversionTest.cpp:734: const std::string
baseURL("http://www.test5.com/");
It was previous CL, related to Keyboard Code. Added in the description. Thanks.

[dgozman, 2015-04-30 09:21:09]

https://codereview.chromium.org/933323002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/screencast/ScreencastView.js (right):

https://codereview.chromium.org/933323002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/screencast/ScreencastView.js:290:
windowsVirtualKeyCode: event.keyCode /* windowsVirtualKeyCode */,
nit: you can drop this comment.

https://codereview.chromium.org/933323002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/screencast/ScreencastView.js:291:
nativeVirtualKeyCode: event.keyCode /* nativeVirtualKeyCode */,
ditto

https://codereview.chromium.org/933323002/diff/80001/Source/devtools/protocol...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/933323002/diff/80001/Source/devtools/protocol...
Source/devtools/protocol.json:4334: { "name": "key", "type": "string",
"optional": true, "description": "Unique DOM defined string value describing the
meaning of the key in the context of active modifiers, keyboard layout, etc
(e.g., 'AltGr') (default: \"\")." },
Note that you will not be able to commit this without changing
content/browser/devtools/protocol/input_handler.{h,cc} first. I think it would
be easier for you to split devtools patch off this, and add support to devtools
protocol separately.

[Habib Virji, 2015-04-30 10:11:52]

https://codereview.chromium.org/933323002/diff/80001/Source/devtools/protocol...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/933323002/diff/80001/Source/devtools/protocol...
Source/devtools/protocol.json:4334: { "name": "key", "type": "string",
"optional": true, "description": "Unique DOM defined string value describing the
meaning of the key in the context of active modifiers, keyboard layout, etc
(e.g., 'AltGr') (default: \"\")." },
On 2015/04/30 09:21:09, dgozman wrote:
> Note that you will not be able to commit this without changing
> content/browser/devtools/protocol/input_handler.{h,cc} first. I think it would
> be easier for you to split devtools patch off this, and add support to
devtools
> protocol separately.

Thanks dgozman, it makes sense. I will try to land this CL. Then send a CL for
content/browser/devtools/protocol/input_handler.{h,cc}  changes, followed by
Blink changes.

[Ian Vollick, 2015-04-30 14:54:33]

On 2015/04/30 at 10:11:52, habib.virji wrote:
>
https://codereview.chromium.org/933323002/diff/80001/Source/devtools/protocol...
> File Source/devtools/protocol.json (right):
> 
>
https://codereview.chromium.org/933323002/diff/80001/Source/devtools/protocol...
> Source/devtools/protocol.json:4334: { "name": "key", "type": "string",
"optional": true, "description": "Unique DOM defined string value describing the
meaning of the key in the context of active modifiers, keyboard layout, etc
(e.g., 'AltGr') (default: \"\")." },
> On 2015/04/30 09:21:09, dgozman wrote:
> > Note that you will not be able to commit this without changing
> > content/browser/devtools/protocol/input_handler.{h,cc} first. I think it
would
> > be easier for you to split devtools patch off this, and add support to
devtools
> > protocol separately.
> 
> Thanks dgozman, it makes sense. I will try to land this CL. Then send a CL for
content/browser/devtools/protocol/input_handler.{h,cc}  changes, followed by
Blink changes.

platform lgtm

[Habib Virji, 2015-04-30 15:08:00]

The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run

[Habib Virji, 2015-04-30 15:08:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
aelias@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/933323002/#ps100001
(title: "Remove DevTool change, will land them separately.")

[commit-bot: I haz the power, 2015-04-30 15:08:25]

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

[Habib Virji, 2015-04-30 15:18:50]

The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run

[Habib Virji, 2015-04-30 15:18:50]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
vollick@chromium.org, bokan@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/933323002/#ps120001
(title: "Added Keyboard Event Key Tests in TestExpectation")

[commit-bot: I haz the power, 2015-04-30 15:19:19]

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

[Rick Byers, 2015-04-30 17:32:43]

On 2015/04/30 15:19:19, I haz the power (commit-bot) wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/933323002/120001

Note that you still have one trivial change in Source/devtools that either needs
to be undone or l-g-t-m'd by a devtools OWNER.

[commit-bot: I haz the power, 2015-04-30 18:10:13]

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

[commit-bot: I haz the power, 2015-04-30 18:10:14]

Dry run: Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/61377)

[Habib Virji, 2015-05-01 10:41:11]

The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run

[Habib Virji, 2015-05-01 10:41:11]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
vollick@chromium.org, bokan@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/933323002/#ps160001
(title: "Updated global-interface-listing to include key")

[commit-bot: I haz the power, 2015-05-01 10:41:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/933323002/160001

[commit-bot: I haz the power, 2015-05-01 13:01:56]

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

[commit-bot: I haz the power, 2015-05-01 13:01:57]

Dry run: This issue passed the CQ dry run.

[Habib Virji, 2015-05-01 13:02:31]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2015-05-01 13:02:42]

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

[commit-bot: I haz the power, 2015-05-01 13:03:30]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194806

[Noel Gordon, 2015-05-01 14:03:04]

The webexposed/global-interface-listing.html is failing on WebKit_Win7 at Build
#29258

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/2925...

---
E:\b\build\slave\WebKit_Win7\build\layout-test-results\webexposed/global-interface-listing-expected.txt
+++
E:\b\build\slave\WebKit_Win7\build\layout-test-results\webexposed/global-interface-listing-actual.txt
@@ -3081,6 +3081,7 @@
     getter charCode
     getter code
     getter ctrlKey
+    getter key
     getter keyCode
     getter keyIdentifier
     getter keyLocation

Yet your patch that added 'key' is still in queue on that bot -- it's still
being processed in next build #29259

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/29259

so it has not really landed on WebKit_Win7 yet.  Odd.

[Habib Virji, 2015-05-01 14:16:11]

On 2015/05/01 14:03:04, noel gordon wrote:
> The webexposed/global-interface-listing.html is failing on WebKit_Win7 at
Build
> #29258
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/2925...
> 
> ---
>
E:\b\build\slave\WebKit_Win7\build\layout-test-results\webexposed/global-interface-listing-expected.txt
> +++
>
E:\b\build\slave\WebKit_Win7\build\layout-test-results\webexposed/global-interface-listing-actual.txt
> @@ -3081,6 +3081,7 @@
>      getter charCode
>      getter code
>      getter ctrlKey
> +    getter key
>      getter keyCode
>      getter keyIdentifier
>      getter keyLocation
> 
> Yet your patch that added 'key' is still in queue on that bot -- it's still
> being processed in next build #29259
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/29259
> 
> so it has not really landed on WebKit_Win7 yet.  Odd.

That's strange. If it has not landed then global-interface-listing, should not
generate error for the Missing interface Key.

[jbroman, 2015-05-01 14:51:51]

On 2015/05/01 14:16:11, Habib Virji wrote:
> On 2015/05/01 14:03:04, noel gordon wrote:
> > The webexposed/global-interface-listing.html is failing on WebKit_Win7 at
> Build
> > #29258
> > 
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/2925...
> > 
> > ---
> >
>
E:\b\build\slave\WebKit_Win7\build\layout-test-results\webexposed/global-interface-listing-expected.txt
> > +++
> >
>
E:\b\build\slave\WebKit_Win7\build\layout-test-results\webexposed/global-interface-listing-actual.txt
> > @@ -3081,6 +3081,7 @@
> >      getter charCode
> >      getter code
> >      getter ctrlKey
> > +    getter key
> >      getter keyCode
> >      getter keyIdentifier
> >      getter keyLocation
> > 
> > Yet your patch that added 'key' is still in queue on that bot -- it's still
> > being processed in next build #29259
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/29259
> > 
> > so it has not really landed on WebKit_Win7 yet.  Odd.
> 
> That's strange. If it has not landed then global-interface-listing, should not
> generate error for the Missing interface Key.

Looks like an infra bug to me. Filed crbug.com/483472.

[Habib Virji, 2015-05-01 15:26:50]

On 2015/04/30 17:32:43, Rick Byers wrote:
> On 2015/04/30 15:19:19, I haz the power (commit-bot) wrote:
> > Dry run: CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/933323002/120001
> 
> Note that you still have one trivial change in Source/devtools that either
needs
> to be undone or l-g-t-m'd by a devtools OWNER.

Thanks Rick. Done

[Wez, 2015-05-06 01:32:13]

Habib, so sorry, I totally missed your reviews in my queue. :(

https://codereview.chromium.org/933323002/diff/160001/LayoutTests/fast/events...
File LayoutTests/fast/events/keyboardevent-key.html (right):

https://codereview.chromium.org/933323002/diff/160001/LayoutTests/fast/events...
LayoutTests/fast/events/keyboardevent-key.html:22: lastKeyboardPressEvent = ev;
You never test against lastKeyboardPressEvent, so you don't need it, surely?

https://codereview.chromium.org/933323002/diff/160001/LayoutTests/fast/events...
LayoutTests/fast/events/keyboardevent-key.html:31:
shouldEvaluateTo('lastKeyboardUpEvent.key', '\'Control\'');
Why are you testing the last keyup here, but the last keydown for the events
that follow?

https://codereview.chromium.org/933323002/diff/160001/LayoutTests/fast/events...
LayoutTests/fast/events/keyboardevent-key.html:41:
shouldEvaluateTo('lastKeyboardDownEvent.key', '\'AltGraph\'');
The left Alt key should only generate AltGraph under specific layouts - what's
the layout this test runs under?

https://codereview.chromium.org/933323002/diff/160001/Source/web/WebInputEven...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/933323002/diff/160001/Source/web/WebInputEven...
Source/web/WebInputEventConversion.cpp:309: // TODO: BUG482880 Fix this
initialization to lazy initialization.
Do you mean "Fix |code| and |key| to be lazily-intialized"?

https://codereview.chromium.org/933323002/diff/160001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/933323002/diff/160001/public/platform/Platfor...
public/platform/Platform.h:641: // This method converts from the supplied DOM
|key| enum to the
Suggest: "... from the embedder-supplied |domKey| enum value to ..."

https://codereview.chromium.org/933323002/diff/160001/public/platform/Platfor...
public/platform/Platform.h:642: // corresponding DOM |key| string value for the
key pressed. |domKey| values are
nit: Remove "for the key pressed" - the call accepts just a string containing a
DOM UI Events |key| meaning; it doesn't contain anything specific to a key
press, release, etc.

Also suggest wrapping consistently to 80 chars, since that's what you've done
for the rest of the comment. :)

https://codereview.chromium.org/933323002/diff/160001/public/platform/Platfor...
public/platform/Platform.h:643: // based on the value defined in
ui/events/keycodes/dom3/dom_key_data.h.
Reword: "The DOM |key| enum <-> string mappings for Chromium are defined in ..."

or, better still, just remove the reference to the Chromium path, since that's a
Chromium- (and in general an embedder-) specific detail.

https://codereview.chromium.org/933323002/diff/160001/public/platform/Platfor...
public/platform/Platform.h:644: // Returns empty string, if DOM key value is not
found.
nit: No need for comma between "string" and "if". Add "the" between "if" and
"DOM", and between "Returns" and "empty".

https://codereview.chromium.org/933323002/diff/160001/public/platform/Platfor...
public/platform/Platform.h:648: // embedder's DOM |key| enum for the key
pressed. |keyString| is defined in
Suggest: "... to the embedder's corresponding enum value."

https://codereview.chromium.org/933323002/diff/160001/public/platform/Platfor...
public/platform/Platform.h:650: // Returns 0 if DOM key enum is not found.
nit: Suggest "Returns 0 if |keyString| is not recognized."

https://codereview.chromium.org/933323002/diff/160001/public/web/WebInputEvent.h
File public/web/WebInputEvent.h (right):

https://codereview.chromium.org/933323002/diff/160001/public/web/WebInputEven...
public/web/WebInputEvent.h:259: // The DOM key enum of the key pressed as passed
by the embedder. DOM
Suggest "Embedder-specific value identifying the DOM |key| for the event.

https://codereview.chromium.org/933323002/diff/160001/public/web/WebInputEven...
public/web/WebInputEvent.h:260: // key enum are defined in
ui/events/keycodes/dom3/dom_key_data.h
The path referred to is in Chromium, whereas this is in Blink code, so this (and
the comment on domCode) should be specific that these are Chromium paths, not
Blink.

[Habib Virji, 2015-05-06 15:38:17]

No issues, Wez. Thanks for the input. Since this CL has landed and it is mostly
wording and test changes. I am uploading a new patch (tomorrow) that will fix
these issues that you mentioned and the rewording that you have highlighted
here. It all relies on chromium patch to be landed, will appreciate if you can
have a look at https://codereview.chromium.org/929053004/