Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
663523002:
Adding support for DOM3 KeyboardEvents Code in KeyboardEvents (Closed)
|
Created:
6 years, 2 months ago by Habib Virji Modified:
5 years, 10 months ago Reviewers:
jochen (gone - plz use gerrit), Wez, bokan, Rick Byers, kpschoedel, Mike West, garykac, dgozman, pfeldman CC:
aandrey+blink_chromium.org, apavlov+blink_chromium.org, arv+blink, blink-reviews, blink-reviews-events_chromium.org, caseq+blink_chromium.org, Inactive, devtools-reviews_chromium.org, dglazkov+blink, eae+blinkwatch, eustas+blink_chromium.org, jamesr, loislo+blink_chromium.org, lushnikov+blink_chromium.org, malch+blink_chromium.org, mkwst+moarreviews_chromium.org, paulirish+reviews_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, vsevik+blink_chromium.org, yurys+blink_chromium.org, tkent Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdding support for DOM3 KeyboardEvents Code
KeyboardEvents has a new field |code| which gives value as per the DOM3 KeyboardEvent code specification.
It uses Embedder API keyboardDOMCodeValue, which fetches code value based on a native key value from the chromium side.
KeyboardEvent.idl is updated to include code field as per the spec.
Intent to ship and implement: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/keyboardevent/blink-dev/Znyl5a96oDc/_kSn1rQ55isJ
R=garykac, Wez
BUG=227231
TEST=fast/events/keyboardevent-code.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189514
Patch Set 1 #Patch Set 2 : Build fix #Patch Set 3 : Test fix #
Total comments: 2
Patch Set 4 : Use native code instead of usb code mapping. Also updated to match firefox values. #
Total comments: 11
Patch Set 5 : Access DOM Code value using embedder API #
Total comments: 8
Patch Set 6 : Update to name of embedder API and cleanup #Patch Set 7 : Added code in relevant constructors and init functions #
Total comments: 2
Patch Set 8 : #
Total comments: 5
Patch Set 9 : Updated to use domCode instead of native domCode #
Total comments: 30
Patch Set 10 : Added new embedder API and code review comments update #
Total comments: 29
Patch Set 11 : "Updated test for keypress and keyup. Added DOM3 code attribute as runtime enabled feature. DOMcode… #
Total comments: 4
Patch Set 12 : Updated unittest to use different urls and destructor to register same url again #
Total comments: 11
Patch Set 13 : Updated comments and resolved return values for the DOM code value and DOM code enum. #Patch Set 14 : Embedder API are part of Platform.h instead of WebViewClient.h #
Total comments: 13
Patch Set 15 : "Updated to the latest master" #Patch Set 16 : Added KeyboardEvent test as a failure in LayoutTests #Patch Set 17 : Added keyboardTest failure for virtual/slimmingpaint #Messages
Total messages: 83 (20 generated)
Also has small changes on the chromium side. https://codereview.chromium.org/658183002/
habib.virji@samsung.com changed reviewers: + haraken@chromium.org
habib.virji@samsung.com changed reviewers: + tkent@chromium.org
habib.virji@samsung.com changed reviewers: + jamesr@chromium.org
PTAL
vsevik@chromium.org changed reviewers: + vsevik@chromium.org
https://codereview.chromium.org/663523002/diff/40001/Source/devtools/Inspecto... File Source/devtools/Inspector-1.1.json (right): https://codereview.chromium.org/663523002/diff/40001/Source/devtools/Inspecto... Source/devtools/Inspector-1.1.json:3839: { "name": "usbCode", "type": "integer", "optional": true, "description": "Usb code (default: 0)." }, This file should not be changed, this is a fixed version of the protocol. https://codereview.chromium.org/663523002/diff/40001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/663523002/diff/40001/Source/devtools/protocol... Source/devtools/protocol.json:4084: { "name": "usbCode", "type": "integer", "optional": true, "description": "Usb code (default: 0)." }, USB code (default: 0)
wez@chromium.org changed required reviewers: + garykac@chromium.org
Updated code to not use usb code and use instead native code. All values are matching with firefox.
habib.virji@samsung.com changed reviewers: + garykac@google.com, wez@chromium.org, wez@google.com
habib.virji@samsung.com changed required reviewers: - garykac@chromium.org
wez@chromium.org changed reviewers: - garykac@google.com, wez@google.com
wez@chromium.org changed required reviewers: + garykac@chromium.org
(Fixed reviewers to include only Gary's and my @chromium Ids and to require approval from Gary)
https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... File Source/core/events/KeyboardCode.h (right): https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... Source/core/events/KeyboardCode.h:49: const NativeCodeToCodeMapEntry keyCodeMap[] = { As previously discussed, Chromium already has this table; please add an embedder-provided API that Blink can call to do any necessary remapping; that way Chromium can just pass in e.g. the nativeKeyCode and let Blink call back out to turn that into DOM3 code. https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... File Source/core/events/KeyboardEvent.cpp (right): https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:89: static inline String nativeCodeToKeyCode(PlatformEvent::Type type, int nativeCode) This takes a nativeKeyCode and returns a DOM |code| string, not a |keyCode| value - perhaps name it nativeCodeToDomCode https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:94: return code; Why would you ever call this for a Char event? https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:99: return code; Why not return the new String directly? https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:103: return code; If you reach here then code is always "" - why do you need a local variable as well (and above). https://codereview.chromium.org/663523002/diff/60001/Source/web/WebInputEvent... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/663523002/diff/60001/Source/web/WebInputEvent... Source/web/WebInputEventConversion.cpp:685: Why the extra blank line?
habib.virji@samsung.com changed reviewers: - haraken@chromium.org, jamesr@chromium.org, tkent@chromium.org, vsevik@chromium.org
@wez: Thanks for the review. Implemented a new embedder API to get dom3 value. Would appreciate if you can have a look and suggest if it's okay. Requires embedder API CL: https://codereview.chromium.org/658183002/ https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... File Source/core/events/KeyboardCode.h (right): https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... Source/core/events/KeyboardCode.h:49: const NativeCodeToCodeMapEntry keyCodeMap[] = { I have added a new embedder API, this file is removed now. Please have a look and suggest if changes are okay. https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... File Source/core/events/KeyboardEvent.cpp (right): https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:94: return code; It is due to KeyboardEvent constructor getting called for a char event. This function is not needed any more. https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:99: return code; On 2014/10/25 00:23:02, Wez wrote: > Why not return the new String directly? Acknowledged. https://codereview.chromium.org/663523002/diff/60001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:103: return code; On 2014/10/25 00:23:02, Wez wrote: > If you reach here then code is always "" - why do you need a local variable as > well (and above). Acknowledged. https://codereview.chromium.org/663523002/diff/60001/Source/web/WebInputEvent... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/663523002/diff/60001/Source/web/WebInputEvent... Source/web/WebInputEventConversion.cpp:685: On 2014/10/25 00:23:02, Wez wrote: > Why the extra blank line? Acknowledged.
https://codereview.chromium.org/663523002/diff/80001/Source/core/events/Keybo... File Source/core/events/KeyboardEvent.cpp (right): https://codereview.chromium.org/663523002/diff/80001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:26: #include "KeyboardCode.h" What is this include required for? https://codereview.chromium.org/663523002/diff/80001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:139: void KeyboardEvent::initKeyboardEvent(const AtomicString& type, bool canBubble, bool cancelable, AbstractView* view, Looks like this and the constructor above also need updating to take a |code|? https://codereview.chromium.org/663523002/diff/80001/Source/platform/Platform... File Source/platform/PlatformKeyboardEvent.h (right): https://codereview.chromium.org/663523002/diff/80001/Source/platform/Platform... Source/platform/PlatformKeyboardEvent.h:54: , m_code("") Why are you not adding |code| to the constructor signature?
Thanks Wez. Updated as per review comments. Please suggests if changes are okay? https://codereview.chromium.org/663523002/diff/80001/Source/core/events/Keybo... File Source/core/events/KeyboardEvent.cpp (right): https://codereview.chromium.org/663523002/diff/80001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:26: #include "KeyboardCode.h" On 2014/11/20 22:17:33, Wez wrote: > What is this include required for? Done. It was previous include file that i created, include was left by mistake. https://codereview.chromium.org/663523002/diff/80001/Source/core/events/Keybo... Source/core/events/KeyboardEvent.cpp:139: void KeyboardEvent::initKeyboardEvent(const AtomicString& type, bool canBubble, bool cancelable, AbstractView* view, On 2014/11/20 22:17:33, Wez wrote: > Looks like this and the constructor above also need updating to take a |code|? Done. Have added in above constructor and updated init function. Previously omitted as above constructor is used in test functions and did not knew if code will be useful for them. Have updated code now to include code. https://codereview.chromium.org/663523002/diff/80001/Source/platform/Platform... File Source/platform/PlatformKeyboardEvent.h (right): https://codereview.chromium.org/663523002/diff/80001/Source/platform/Platform... Source/platform/PlatformKeyboardEvent.h:54: , m_code("") On 2014/11/20 22:17:33, Wez wrote: > Why are you not adding |code| to the constructor signature? Since the constructor is used by inspector, there was a review comment mentioning not to change constructor signature in a inspector-1.1.json, so did not wanted to disturb the constructor behavior. Have added back again, to make it consistent. Plesae suggest if it's okay.
@wez: can you please have a look.
@wez/garykac: ping
This looks reasonable to me, but you'll need approval from Blink owners to land. https://codereview.chromium.org/663523002/diff/80001/Source/web/WebInputEvent... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/663523002/diff/80001/Source/web/WebInputEvent... Source/web/WebInputEventConversion.cpp:357: void PlatformKeyboardEventBuilder::setDOMCode(const char* code) Could we add the code member to WebKeyboardEvent and remove the setDOMCode() setter here? https://codereview.chromium.org/663523002/diff/120001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/120001/public/web/WebViewClien... public/web/WebViewClient.h:144: virtual const char* keyboardDOMCodeValue(int nativeValue) { return ""; } Add a comment to explain that this converts from the embedded-supplied nativeValue to the DOM |code| value.
Thanks wez, updated comment. But has not added in WebKeyboardEvent as will increase size of the struct. https://codereview.chromium.org/663523002/diff/80001/Source/web/WebInputEvent... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/663523002/diff/80001/Source/web/WebInputEvent... Source/web/WebInputEventConversion.cpp:357: void PlatformKeyboardEventBuilder::setDOMCode(const char* code) On 2014/12/02 04:23:30, Wez wrote: > Could we add the code member to WebKeyboardEvent and remove the setDOMCode() > setter here? I was trying to avoid, as that will increase the size of the WebKeyboardEvent struct size. https://codereview.chromium.org/663523002/diff/120001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/120001/public/web/WebViewClien... public/web/WebViewClient.h:144: virtual const char* keyboardDOMCodeValue(int nativeValue) { return ""; } On 2014/12/02 04:23:30, Wez wrote: > Add a comment to explain that this converts from the embedded-supplied > nativeValue to the DOM |code| value. Done.
habib.virji@samsung.com changed required reviewers: - garykac@chromium.org
habib.virji@samsung.com changed reviewers: + dgozman@chromium.org, rbyers@chromium.org
On 2014/12/02 at 11:58:49, Habib Virji wrote: > habib.virji@samsung.com changed reviewers: > + dgozman@chromium.org, rbyers@chromium.org Added dgozman for inspector related changes and rbyers for DOM3 keyboard event changes.
On 2014/12/02 11:59:27, Habib Virji wrote: > On 2014/12/02 at 11:58:49, Habib Virji wrote: > > mailto:habib.virji@samsung.com changed reviewers: > > + mailto:dgozman@chromium.org, mailto:rbyers@chromium.org > > Added dgozman for inspector related changes and rbyers for DOM3 keyboard event > changes. New APIs need to go through the blink feature process: http://www.chromium.org/blink#launch-process. Please wait for approval on an "intent to ship" before landing any IDL changes for this. I don't have keyboard-specific context, but if you get your approval to ship, and wez/garykac approve the patch from a keyboard perspective, then I can review as a blink/core OWNER.
On 2014/12/02 at 15:24:48, rbyers wrote: > On 2014/12/02 11:59:27, Habib Virji wrote: > > On 2014/12/02 at 11:58:49, Habib Virji wrote: > > > mailto:habib.virji@samsung.com changed reviewers: > > > + mailto:dgozman@chromium.org, mailto:rbyers@chromium.org > > > > Added dgozman for inspector related changes and rbyers for DOM3 keyboard event > > changes. > > New APIs need to go through the blink feature process: http://www.chromium.org/blink#launch-process. Please wait for approval on an "intent to ship" before landing any IDL changes for this. I don't have keyboard-specific context, but if you get your approval to ship, and wez/garykac approve the patch from a keyboard perspective, then I can review as a blink/core OWNER. Thanks rbyers for looking at this: 1. DOM3 Intent to implement and launch has been approved sometime back: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/keyboarde... 2. wez looked okay with changes in https://codereview.chromium.org/663523002#msg22.
On 2014/12/02 15:34:05, Habib Virji wrote: > On 2014/12/02 at 15:24:48, rbyers wrote: > > On 2014/12/02 11:59:27, Habib Virji wrote: > > > On 2014/12/02 at 11:58:49, Habib Virji wrote: > > > > mailto:habib.virji@samsung.com changed reviewers: > > > > + mailto:dgozman@chromium.org, mailto:rbyers@chromium.org > > > > > > Added dgozman for inspector related changes and rbyers for DOM3 keyboard > event > > > changes. > > > > New APIs need to go through the blink feature process: > http://www.chromium.org/blink#launch-process. Please wait for approval on an > "intent to ship" before landing any IDL changes for this. I don't have > keyboard-specific context, but if you get your approval to ship, and wez/garykac > approve the patch from a keyboard perspective, then I can review as a blink/core > OWNER. > > Thanks rbyers for looking at this: > > 1. DOM3 Intent to implement and launch has been approved sometime back: > https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/keyboarde... Oh good! I didn't realize that (I thought there had been some concern/debate, but looking at my e-mail history I was wrong). > 2. wez looked okay with changes in > https://codereview.chromium.org/663523002#msg22. Yes, but he didn't actually approve the patch. Wez, are you comfortable LG'ing this CL?
On 2014/12/02 15:52:55, Rick Byers wrote: > On 2014/12/02 15:34:05, Habib Virji wrote: > > On 2014/12/02 at 15:24:48, rbyers wrote: > > > On 2014/12/02 11:59:27, Habib Virji wrote: > > > > On 2014/12/02 at 11:58:49, Habib Virji wrote: > > > > > mailto:habib.virji@samsung.com changed reviewers: > > > > > + mailto:dgozman@chromium.org, mailto:rbyers@chromium.org > > > > > > > > Added dgozman for inspector related changes and rbyers for DOM3 keyboard > > event > > > > changes. > > > > > > New APIs need to go through the blink feature process: > > http://www.chromium.org/blink#launch-process. Please wait for approval on an > > "intent to ship" before landing any IDL changes for this. I don't have > > keyboard-specific context, but if you get your approval to ship, and > wez/garykac > > approve the patch from a keyboard perspective, then I can review as a > blink/core > > OWNER. > > > > Thanks rbyers for looking at this: > > > > 1. DOM3 Intent to implement and launch has been approved sometime back: > > > https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/keyboarde... > > Oh good! I didn't realize that (I thought there had been some concern/debate, > but looking at my e-mail history I was wrong). And please add a link to this thread to the CL description. > > 2. wez looked okay with changes in > > https://codereview.chromium.org/663523002#msg22. > > Yes, but he didn't actually approve the patch. Wez, are you comfortable LG'ing > this CL?
https://codereview.chromium.org/663523002/diff/140001/LayoutTests/fast/events... File LayoutTests/fast/events/keyboardevent-code.html (right): https://codereview.chromium.org/663523002/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/keyboardevent-code.html:29: shouldEvaluateTo('lastKeyboardEvent.code', '\'KeyA\'') ; Is it possible to have tests for other keyboard layouts? For example, with a French layout, pressing the key labeled 'a' would produce a |code| of 'KeyQ' since it is in a different physical location. https://codereview.chromium.org/663523002/diff/140001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.h (right): https://codereview.chromium.org/663523002/diff/140001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.h:116: String m_code; While we need to expose the |code| string as part of the public API, can we use |code| enums internally so that we're not copying strings all over the place? These enums for |code| were just recently added: https://codereview.chromium.org/641753003
Thanks garykac, for having a look. Can you clarify regarding enum value usage, as would require a conversion table for converting enum value to the string value on the blink side. https://codereview.chromium.org/663523002/diff/140001/LayoutTests/fast/events... File LayoutTests/fast/events/keyboardevent-code.html (right): https://codereview.chromium.org/663523002/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/keyboardevent-code.html:29: shouldEvaluateTo('lastKeyboardEvent.code', '\'KeyA\'') ; On 2014/12/02 16:29:32, garykac wrote: > Is it possible to have tests for other keyboard layouts? > > For example, with a French layout, pressing the key labeled 'a' would produce a > |code| of 'KeyQ' since it is in a different physical location. I will look into it and update if it's possible. https://codereview.chromium.org/663523002/diff/140001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.h (right): https://codereview.chromium.org/663523002/diff/140001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.h:116: String m_code; On 2014/12/02 16:29:32, garykac wrote: > While we need to expose the |code| string as part of the public API, can we use > |code| enums internally so that we're not copying strings all over the place? > > These enums for |code| were just recently added: > https://codereview.chromium.org/641753003 The only reason for not passing int value was it will need table again on the blink side, to convert enum value to the string. Is it okay to have a table for a conversion purpose?
Thanks garykac, for having a look. Can you clarify regarding enum value usage, as would require a conversion table for converting enum value to the string value on the blink side.
kpschoedel@chromium.org changed reviewers: + kpschoedel@chromium.org
https://codereview.chromium.org/663523002/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/663523002/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1004: evt.setKeyboardEventDOMCodeValue(m_client->keyboardDOMCodeValue(evt.nativeVirtualKeyCode())); This uses WebKeyboardEvent.nativeKeyCode, which concerns me, because in some places this field is set to the platform scan code (as the use here expects) and in other places this field is set to the Windows VK code. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2014/12/02 at 17:00:52, kpschoedel wrote: > https://codereview.chromium.org/663523002/diff/140001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/663523002/diff/140001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:1004: evt.setKeyboardEventDOMCodeValue(m_client->keyboardDOMCodeValue(evt.nativeVirtualKeyCode())); > This uses WebKeyboardEvent.nativeKeyCode, which concerns me, because in some places this field is set to the platform scan code (as the use here expects) and in other places this field is set to the Windows VK code. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... @Kevin: WHat do you reckon will be a better way. IMO, best way would be to pass dom code as part of the WebKeyboardEvent, but that will impact the struct size. Do you have any other suggestion? (Or) possible solution could be pass enum as gary suggested and then have another embedder function to get the mapping value?
On 2014/12/02 17:37:32, Habib Virji wrote: > @Kevin: WHat do you reckon will be a better way. IMO, best way would be to pass > dom code as part of the WebKeyboardEvent, but that will impact the struct size. > Do you have any other suggestion? > > (Or) possible solution could be pass enum as gary suggested and then have > another embedder function to get the mapping value? I'm afraid I don't know Blink at all, so I don't know what this field is really intended to be.
On 2014/12/02 17:37:32, Habib Virji wrote: > On 2014/12/02 at 17:00:52, kpschoedel wrote: > > > https://codereview.chromium.org/663523002/diff/140001/Source/web/WebViewImpl.cpp > > File Source/web/WebViewImpl.cpp (right): > > > > > https://codereview.chromium.org/663523002/diff/140001/Source/web/WebViewImpl.... > > Source/web/WebViewImpl.cpp:1004: > evt.setKeyboardEventDOMCodeValue(m_client->keyboardDOMCodeValue(evt.nativeVirtualKeyCode())); > > This uses WebKeyboardEvent.nativeKeyCode, which concerns me, because in some > places this field is set to the platform scan code (as the use here expects) and > in other places this field is set to the Windows VK code. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > @Kevin: WHat do you reckon will be a better way. IMO, best way would be to pass > dom code as part of the WebKeyboardEvent, but that will impact the struct size. > Do you have any other suggestion? In general I wouldn't worry about making the struct size bigger when it's really the right thing to do (not that I'm implying that's the case here - I haven't looked into the details of the discussion). Events are short-lived and we rarely have many at any given time. I've done some simple tests in the past verifying that an event struct would have to get a LOT bigger to have noticeable perf impact. Here's a current example where we'd adding an additional event field that's technically redundant, but we want to let chromium dictate the semantics to blink explicitly rather than have blink infer them: https://codereview.chromium.org/759073002 (in fact, I originally landed the CL that made blink infer the semantics and the bugs that resulted showed clearly it was a mistake). > (Or) possible solution could be pass enum as gary suggested and then have > another embedder function to get the mapping value?
On 2014/12/02 16:39:05, Habib Virji wrote: > Thanks garykac, for having a look. Can you clarify regarding enum value usage, > as would require a conversion table for converting enum value to the string > value on the blink side. I haven't looked at the blink code in a while, but AIUI this should be encoded as an enum throughout blink. Previously they expressed a strong preference for enums over strings. > The only reason for not passing int value was it will need table again on the > blink side, to convert enum value to the string. Is it okay to have a table for > a conversion purpose? Blink should be using enums for |code| internally everywhere - there's no need for it to be passing around strings. The only time we need to convert from enum->string when we're creating the keyboard event that the user sees. (Note: If blink doesn't currently use enums everywhere, then it should be changed so that it does. But that can be a separate cl.)
On 2014/12/02 19:14:04, garykac wrote: > On 2014/12/02 16:39:05, Habib Virji wrote: > > Thanks garykac, for having a look. Can you clarify regarding enum value usage, > > as would require a conversion table for converting enum value to the string > > value on the blink side. > > I haven't looked at the blink code in a while, but AIUI this should be encoded > as an enum throughout blink. Previously they expressed a strong preference for > enums over strings. > > > The only reason for not passing int value was it will need table again on the > > blink side, to convert enum value to the string. Is it okay to have a table > for > > a conversion purpose? > > Blink should be using enums for |code| internally everywhere - there's no need > for it to be passing around strings. > > The only time we need to convert from enum->string when we're creating the > keyboard event that the user sees. > > (Note: If blink doesn't currently use enums everywhere, then it should be > changed so that it does. But that can be a separate cl.) +tkent as I believe he has more experience with keyboard events in blink then I do. I talked briefly with Habib on IRC. Poking at this a bit it looks to me like what we probably want is: 1) A keycode enum in blink, eg. public/platform/WebKeyboardEventCode.h 2) COMPILE_ASSERTS (ideally tool-generated) to verify all the values in this new enum match exactly those on the chromium side (DomCode) 3) Add a keycode field of this type to WebKeyboardEvent and PlatformKeyboardEvent 4) core's KeyboardEvent still needs to store the code as a string since JS can create instances with arbitrary strings. 5) for the common case, we should probably lazily populate this string field from the PlatformKeyboardEvent's keyCode value - i.e. avoid ever creating a string for the key code unless JavaScript asks for it. Sound reasonable?
On 2014/12/03 16:24:43, Rick Byers wrote: > On 2014/12/02 19:14:04, garykac wrote: > > On 2014/12/02 16:39:05, Habib Virji wrote: > > > Thanks garykac, for having a look. Can you clarify regarding enum value > usage, > > > as would require a conversion table for converting enum value to the string > > > value on the blink side. > > > > I haven't looked at the blink code in a while, but AIUI this should be encoded > > as an enum throughout blink. Previously they expressed a strong preference for > > enums over strings. > > > > > The only reason for not passing int value was it will need table again on > the > > > blink side, to convert enum value to the string. Is it okay to have a table > > for > > > a conversion purpose? > > > > Blink should be using enums for |code| internally everywhere - there's no need > > for it to be passing around strings. > > > > The only time we need to convert from enum->string when we're creating the > > keyboard event that the user sees. > > > > (Note: If blink doesn't currently use enums everywhere, then it should be > > changed so that it does. But that can be a separate cl.) > > +tkent as I believe he has more experience with keyboard events in blink then I > do. > > I talked briefly with Habib on IRC. Poking at this a bit it looks to me like > what we probably want is: > > 1) A keycode enum in blink, eg. public/platform/WebKeyboardEventCode.h > 2) COMPILE_ASSERTS (ideally tool-generated) to verify all the values in this new > enum match exactly those on the chromium side (DomCode) > 3) Add a keycode field of this type to WebKeyboardEvent and > PlatformKeyboardEvent > 4) core's KeyboardEvent still needs to store the code as a string since JS can > create instances with arbitrary strings. > 5) for the common case, we should probably lazily populate this string field > from the PlatformKeyboardEvent's keyCode value - i.e. avoid ever creating a > string for the key code unless JavaScript asks for it. > > Sound reasonable? That sounds unnecessarily complex; the enum can be treated by Blink as an opaque "platform" code, with the value determined by the embedded, so that we only need a single table, and a method provided by the embedded to allow their codes to be resolved to string forms. Let's discuss this off CL with Kevin, Gary et al.
Message was sent while issue was closed.
Closed, since have not heard from anyone. Please suggest if there are some consensus.
Message was sent while issue was closed.
On 2014/12/17 16:26:36, Habib Virji wrote: > Closed, since have not heard from anyone. Please suggest if there are some > consensus. Hi Habib, apologies for the lack of clarity on this. Speaking to Rick & Gary offline, we're agreed on: - Having Chromium define the enum values used to represent each |code|. - Having Chromium pass those to Blink, and provide an embedder API for Blink to turn them to DOMStrings. - Having Blink treat the enum values as opaque integers. Since Blink should only be asking the embedder API to resolve codes that were passed to Blink by Chromium, we should log a warning if it ever asks Chromium to resolve an unknown value. The main downside of not using enums in Blink is that we can't then tell if Blink code that checks for a particular |code| string value is checking for something that can ever actually be generated. That issue is mitigated by the fact that Blink should always be implementing web behaviours based on |key|, rather than |code|. If we do find specific cases where |code| values need to be tested against then we can add a debug check that it's a code string known to the embedder API, for example.
On 2014/12/22 18:09:56, Wez wrote: > On 2014/12/17 16:26:36, Habib Virji wrote: > > Closed, since have not heard from anyone. Please suggest if there are some > > consensus. > > Hi Habib, apologies for the lack of clarity on this. > > Speaking to Rick & Gary offline, we're agreed on: > - Having Chromium define the enum values used to represent each |code|. > - Having Chromium pass those to Blink, and provide an embedder API for Blink to > turn them to DOMStrings. > - Having Blink treat the enum values as opaque integers. Since Blink should only > be asking the embedder API to resolve codes that were passed to Blink by > Chromium, we should log a warning if it ever asks Chromium to resolve an unknown > value. > > The main downside of not using enums in Blink is that we can't then tell if > Blink code that checks for a particular |code| string value is checking for > something that can ever actually be generated. That issue is mitigated by the > fact that Blink should always be implementing web behaviours based on |key|, > rather than |code|. If we do find specific cases where |code| values need to be > tested against then we can add a debug check that it's a code string known to > the embedder API, for example. Thanks Wez for your response, I have updated the code 1. Now code used domCode to fetch domCode string value. It does not rely on the native keyCode as previous code. DomCode is opaque to the blink side and it just relies on the domCode to get string value. DomCode is not exposed in KeyboardEvent. 2. Have used large values to pass domCode value. 3. Only thing is debug check, which side should it handle blink or chromium, that would need update.
Thanks for updating this and apologies for the delay in re-reviewing it. Rick, Kevin, do either of you know if there's a cleaner way to allow the input conversion routines to take advantage of the embedded enum->string and string->enum APIs? https://codereview.chromium.org/663523002/diff/160001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.idl (right): https://codereview.chromium.org/663523002/diff/160001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.idl:41: // FIXME: this does not match the version in the DOM spec. Do callers have to specify the parameter fields by name, or can they set them purely by position? It looks like initKeyboardEvent() is actually deprecated in DOM3 anyway, so do we even need to update this interface (especially given it's not even consistent with the DOM spec...) https://codereview.chromium.org/663523002/diff/160001/Source/core/inspector/I... File Source/core/inspector/InspectorInputAgent.h (right): https://codereview.chromium.org/663523002/diff/160001/Source/core/inspector/I... Source/core/inspector/InspectorInputAgent.h:57: virtual void dispatchKeyEvent(ErrorString*, const String& type, const int* modifiers, const double* timestamp, const String* text, const String* unmodifiedText, const String* keyIdentifier, const int* domCode, const int* windowsVirtualKeyCode, const int* nativeVirtualKeyCode, const bool* autoRepeat, const bool* isKeypad, const bool* isSystemKey) override; Why is domCode an int* rather than a String*? https://codereview.chromium.org/663523002/diff/160001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/663523002/diff/160001/Source/devtools/protoco... Source/devtools/protocol.json:4301: { "name": "code", "type": "integer", "optional": true, "description": "Unique dom code for each physical key (e.g., '0x07002c') (default: 0)." }, s/dom/DOM The codes aren't defined by DOM, though - DOM defines the string values. If you want this to stay an integer then it's really an opaque value used by the embedder to represent a DOM |code| value. https://codereview.chromium.org/663523002/diff/160001/Source/platform/Platfor... File Source/platform/PlatformKeyboardEvent.h (right): https://codereview.chromium.org/663523002/diff/160001/Source/platform/Platfor... Source/platform/PlatformKeyboardEvent.h:81: int domCode() const { return m_domCode; } Why do we need both domcode and code() here? https://codereview.chromium.org/663523002/diff/160001/Source/platform/Platfor... Source/platform/PlatformKeyboardEvent.h:82: String code() const { return m_code; } Where does m_code get set? https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:316: m_domCode = e.domCode; Is there any point in storing the numeric form of this value? It's opaque (i.e. meaningless) to Blink, and you're going to use it to det the m_code string in the WebViewImpl later on anyway. https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:681: domCode = event.keyEvent()->domCode(); Similarly to above, can we arrange this translation via the embedder API, from string back to number, rather than passing the numeric value around in [Platform]KeyboardEvent as well as the string value? https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.h (right): https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... Source/web/WebInputEventConversion.h:78: void setKeyboardEventDOMCodeValue(const char*); Given that this is a member of PlatformKeyboardEvent, can we call it just setDomCode()? Do we actually need a setter or can we pull the value from the WebKeyboardEvent in the constructor? https://codereview.chromium.org/663523002/diff/160001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/663523002/diff/160001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:966: evt.setKeyboardEventDOMCodeValue(m_client->keyboardDOMCodeValue(evt.domCode())); It reads very oddly to be passing evt.domCode() into the conversion routine; really what you're passing is the event.domCode value, I think. Do we need to do the conversion from numeric value to code here, or could we defer it until we need to convert from PlatformKeyboardEvent to DOM KeyboardEvent? https://codereview.chromium.org/663523002/diff/160001/Source/web/tests/WebPlu... File Source/web/tests/WebPluginContainerTest.cpp (right): https://codereview.chromium.org/663523002/diff/160001/Source/web/tests/WebPlu... Source/web/tests/WebPluginContainerTest.cpp:202: PlatformKeyboardEvent platformKeyboardEventC(PlatformEvent::RawKeyDown, "", "", "67", 67, 67, 0, false, false, false, modifierKey, 0.0); Better to supply a properly "dummy" value for domCode, e.g. 0 (i.e. Undefined) here. https://codereview.chromium.org/663523002/diff/160001/Source/web/tests/WebPlu... Source/web/tests/WebPluginContainerTest.cpp:211: PlatformKeyboardEvent platformKeyboardEventInsert(PlatformEvent::RawKeyDown, "", "", "45", 45, 45, 0, false, false, false, modifierKey, 0.0); Here too. https://codereview.chromium.org/663523002/diff/160001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/160001/public/web/WebViewClien... public/web/WebViewClient.h:139: virtual const char* keyboardDOMCodeValue(long domCode) { return ""; } Suggest domCodeStringFromEnum() or something along those lines.
Thanks Wez and sorry for the late response. I have updated code. New patch has following changes - Previously I was facing issue with invoking embedder API in WebInputEventConventor, since it did not had instance of WebViewClient. I am now passing widget instance. All other inputs, do pass widget instance. - KeyboardInit.idl does not have code parameter in initKeyboard function, this effects all KeyboardEvent.* files. - InpsectorInputAgent, protocol.json are updated to match PlatformKeyboardEvent constructor. - New embedder API is added, to allow code string to dom enum value conversion. This is possible because of passing widget instance. https://codereview.chromium.org/663523002/diff/160001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.idl (right): https://codereview.chromium.org/663523002/diff/160001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.idl:41: // FIXME: this does not match the version in the DOM spec. My understanding is they can be set based on the position. I am planning to do cleanup, post landing this CL. Removed code from init keyboard event. https://codereview.chromium.org/663523002/diff/160001/Source/core/inspector/I... File Source/core/inspector/InspectorInputAgent.h (right): https://codereview.chromium.org/663523002/diff/160001/Source/core/inspector/I... Source/core/inspector/InspectorInputAgent.h:57: virtual void dispatchKeyEvent(ErrorString*, const String& type, const int* modifiers, const double* timestamp, const String* text, const String* unmodifiedText, const String* keyIdentifier, const int* domCode, const int* windowsVirtualKeyCode, const int* nativeVirtualKeyCode, const bool* autoRepeat, const bool* isKeypad, const bool* isSystemKey) override; On 2015/01/08 01:14:47, Wez wrote: > Why is domCode an int* rather than a String*? Since this is the value that was the input, while string value is obtained via chromium. Since these values are input, was using int for representing the opaque values. https://codereview.chromium.org/663523002/diff/160001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/663523002/diff/160001/Source/devtools/protoco... Source/devtools/protocol.json:4301: { "name": "code", "type": "integer", "optional": true, "description": "Unique dom code for each physical key (e.g., '0x07002c') (default: 0)." }, Corrected text. Also as you pointed, do not see need of using int and using string instead to represent |code| value. https://codereview.chromium.org/663523002/diff/160001/Source/platform/Platfor... File Source/platform/PlatformKeyboardEvent.h (right): https://codereview.chromium.org/663523002/diff/160001/Source/platform/Platfor... Source/platform/PlatformKeyboardEvent.h:81: int domCode() const { return m_domCode; } Yes, you are right. Value is already present in the WebKeyboardEvent, it does not need it here, have removed this code. https://codereview.chromium.org/663523002/diff/160001/Source/platform/Platfor... Source/platform/PlatformKeyboardEvent.h:82: String code() const { return m_code; } It is getting set in WebViewImpl.cc. https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:316: m_domCode = e.domCode; The only reason I was saving this is, as I did not found a way of using embedder API to fetch domCode value for WebKeyboardEvent, as there is no access to the instance of the WebViewClient. In new patch I have modified this now, and does not require storing domCode. https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:366: } This code is now removed. https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:681: domCode = event.keyEvent()->domCode(); New code has embedder API for this purpose. https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.h (right): https://codereview.chromium.org/663523002/diff/160001/Source/web/WebInputEven... Source/web/WebInputEventConversion.h:78: void setKeyboardEventDOMCodeValue(const char*); We can set the value, but m_code is declared as private thus restricting it assigning. Have removed setter here and instead using setter in PlatformKeyboardEvent. https://codereview.chromium.org/663523002/diff/160001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/663523002/diff/160001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:966: evt.setKeyboardEventDOMCodeValue(m_client->keyboardDOMCodeValue(evt.domCode())); I have deferred it, please have a look. It now happens in WebInputEventConversion. https://codereview.chromium.org/663523002/diff/160001/Source/web/tests/WebPlu... File Source/web/tests/WebPluginContainerTest.cpp (right): https://codereview.chromium.org/663523002/diff/160001/Source/web/tests/WebPlu... Source/web/tests/WebPluginContainerTest.cpp:202: PlatformKeyboardEvent platformKeyboardEventC(PlatformEvent::RawKeyDown, "", "", "67", 67, 67, 0, false, false, false, modifierKey, 0.0); On 2015/01/08 01:14:48, Wez wrote: > Better to supply a properly "dummy" value for domCode, e.g. 0 (i.e. Undefined) > here. Done. https://codereview.chromium.org/663523002/diff/160001/Source/web/tests/WebPlu... Source/web/tests/WebPluginContainerTest.cpp:211: PlatformKeyboardEvent platformKeyboardEventInsert(PlatformEvent::RawKeyDown, "", "", "45", 45, 45, 0, false, false, false, modifierKey, 0.0); On 2015/01/08 01:14:48, Wez wrote: > Here too. Done. https://codereview.chromium.org/663523002/diff/160001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/160001/public/web/WebViewClien... public/web/WebViewClient.h:139: virtual const char* keyboardDOMCodeValue(long domCode) { return ""; } On 2015/01/08 01:14:48, Wez wrote: > Suggest domCodeStringFromEnum() or something along those lines. Done.
https://codereview.chromium.org/663523002/diff/160001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/663523002/diff/160001/Source/devtools/protoco... Source/devtools/protocol.json:4301: { "name": "code", "type": "integer", "optional": true, "description": "Unique dom code for each physical key (e.g., '0x07002c') (default: 0)." }, This is a public method documented at https://developer.chrome.com/devtools/docs/protocol/1.1/input#command-dispatc.... It should be useful for developers to pass the |code| parameter, otherwise we should not change public protocol methods.
https://codereview.chromium.org/663523002/diff/160001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/663523002/diff/160001/Source/devtools/protoco... Source/devtools/protocol.json:4301: { "name": "code", "type": "integer", "optional": true, "description": "Unique dom code for each physical key (e.g., '0x07002c') (default: 0)." }, It can be set by developer. The reason it was updated, since InspectorInputAgent::dispatchKeyEvent in turns call PlatformKeyboardEvent. Since PlatformKeyboardEvent has been updated to handle new domCode string parameter, it needed to match.
@wez:ping
This looks to be heading in the right direction - thanks for continuing to work on it, Habib - but I'm not very familiar with a lot of the Blink conventions & the WebView glue, so it'll help if one of the reviewers more familiar with those can take a look too. https://codereview.chromium.org/663523002/diff/160001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.idl (right): https://codereview.chromium.org/663523002/diff/160001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.idl:41: // FIXME: this does not match the version in the DOM spec. On 2015/01/12 15:34:17, Habib Virji wrote: > My understanding is they can be set based on the position. > > I am planning to do cleanup, post landing this CL. Removed code from init > keyboard event. Gary is more familiar with the UI Events spec for this, so best to ask him to take a look and make sure the initializer, etc are being set up correctly here. https://codereview.chromium.org/663523002/diff/160001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/663523002/diff/160001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:966: evt.setKeyboardEventDOMCodeValue(m_client->keyboardDOMCodeValue(evt.domCode())); On 2015/01/12 15:34:17, Habib Virji wrote: > I have deferred it, please have a look. It now happens in > WebInputEventConversion. SGTM - please check w/ e.g. rbyers@ that the frameView() references throughout this patch are correct - I'm not sure whether mainFrameImpl() can always be relied upon. https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:317: } This leaves the |code| empty, which is also the state it's in if we create a KeyboardEvent without a code - the DOM UIEvents spec requires unknown codes to return "Undefined" or something along those lines, IIRC - perhaps check w/ garykac@ what the right thing is to return if there is no |code|, e.g. should the event just act as though it has no |code| field? https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:328: return 0; This means that the embedder cannot use zero as one of the enum values, otherwise this wouldn't be distinguishable from an actual code; maybe document that on the domEnumFromCodeString() API interface? https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/Keyboa... File Source/web/tests/KeyboardTest.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/Keyboa... Source/web/tests/KeyboardTest.cpp:68: WebViewImpl* webViewImpl = webViewHelper.initializeAndLoad(baseURL + fileName, true); Doesn't this need cleaning up somewhere? https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:70: WebViewImpl* webViewImpl = webViewHelper.initializeAndLoad(baseURL + fileName, true); Do you need to tear this page down somewhere? It looks like you're creating a new page for every call to this helper? https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClien... public/web/WebViewClient.h:138: // the string value of the domCode nit: You're missing the full-stop at the line end. https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClien... public/web/WebViewClient.h:142: // the dom enum value And you're missing the full-stop here too. This comment is not very clear; you mean it converts from a supplied DOM |code| string to the embedder's enum value for that key, I think?
rbyers@chromium.org changed reviewers: + bokan@chromium.org
Sorry for the delay in chiming in. To help you get faster reviews from a blink expert I've added bokan@. Dave, can you please take a look ASAP? Once you're happy I can do an OWNER review for Source/core and public/ (we'll also need to find an Source/web OWNER). I've reviewed the public/ and .idl API changes and added a couple questions/comments. https://codereview.chromium.org/663523002/diff/180001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.idl (right): https://codereview.chromium.org/663523002/diff/180001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.idl:29: readonly attribute DOMString code; This should be initialized by the constructor as well (and there should be a test that verifies it can be set with a constructor). Please put your new API surface area behind a RuntimeEnabledFeature (http://www.chromium.org/blink/runtime-enabled-features) that's initially 'experimental'. Once all chromium/blink CLs have landed and some basic compat testing has been done across platforms, we can switch it to 'Stable'. Worst case and we discover some compat breakage we can just flip it back to 'Experimental' (rather than have to revert your CLs) while we address issues. https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClien... public/web/WebViewClient.h:143: virtual long domEnumFromCodeString(const char* codeString) { return 0; } We don't pass strings as 'char*' anywhere else in the blink API that I can see (normally we have UTF16 and questions of lifetime management to address). Perhaps we should be using WebString here and above instead? Or do we want to keep this as lightweight as possible and require the embedder to simply return pointers into a static table (no dynamic allocation permitted)?
https://codereview.chromium.org/663523002/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/keyboardevent-code.html (right): https://codereview.chromium.org/663523002/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/keyboardevent-code.html:7: <body> <head>,<body>,<html> should be omitted. See http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... https://codereview.chromium.org/663523002/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/keyboardevent-code.html:14: textarea.addEventListener('keypress', recordKeyEvent, false); Could you add at least one case in the test below for keyup and keypress events? https://codereview.chromium.org/663523002/diff/180001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.cpp:125: const String &keyIdentifier, const String &code, unsigned location, bool ctrlKey, bool altKey, bool shiftKey, bool metaKey) Nit: should be 'const String&' (no space between type and &) Please also change occurrences of this you didn't add (like below). https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... File Source/web/WebInputEvent.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... Source/web/WebInputEvent.cpp:46: int keyboardData[14]; sizeof(long) == sizeof(int) on typical 32 bit builds so this will likely break. Even if it doesn't, it's a bad assumption to make. Does domCode have to be a long? https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:310: static void getDomCodeStringFromEnum(Widget* widget, String& code, long domCode) Can we return the string, rather than using an output parameter? It'd be clearer. https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/Keyboa... File Source/web/tests/KeyboardTest.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/Keyboa... Source/web/tests/KeyboardTest.cpp:68: WebViewImpl* webViewImpl = webViewHelper.initializeAndLoad(baseURL + fileName, true); On 2015/01/17 02:35:10, Wez wrote: > Doesn't this need cleaning up somewhere? webViewHelper will clean up on destruction. That said, please move webViewHelper to be a private member of the KeyboardTest class and move the rest of the boilerplate (registerMockedURLFromBaseURL, initializeAndLoad, etc.) into the individual tests cases. For example of how this should look: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:70: WebViewImpl* webViewImpl = webViewHelper.initializeAndLoad(baseURL + fileName, true); On 2015/01/17 02:35:10, Wez wrote: > Do you need to tear this page down somewhere? > > It looks like you're creating a new page for every call to this helper? Ditto here https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClien... public/web/WebViewClient.h:137: // This method converts from the embedded-supplied domCode to Nit: embedder-supplied
Thanks wez/rbyers/bokan for reviewing. I am working on the changes suggested, and will upload changes soon.
Sorry for delay in updating. Done following changes. - Updated test for keypress and keyp - Added DOM3 code attribute as runtime enabled feature. - DOMcode returns String and if not found sets "Undefined" - Updated KeyboadTest to load pages in each test. https://codereview.chromium.org/663523002/diff/160001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.idl (right): https://codereview.chromium.org/663523002/diff/160001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.idl:41: // FIXME: this does not match the version in the DOM spec. Okay will wait for Gary response on this. https://codereview.chromium.org/663523002/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/keyboardevent-code.html (right): https://codereview.chromium.org/663523002/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/keyboardevent-code.html:7: <body> On 2015/01/20 17:05:58, bokan wrote: > <head>,<body>,<html> should be omitted. See > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... Done. https://codereview.chromium.org/663523002/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/keyboardevent-code.html:14: textarea.addEventListener('keypress', recordKeyEvent, false); have added now keypress and keyup check were relevant in each keys section. https://codereview.chromium.org/663523002/diff/180001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.cpp:125: const String &keyIdentifier, const String &code, unsigned location, bool ctrlKey, bool altKey, bool shiftKey, bool metaKey) On 2015/01/20 17:05:58, bokan wrote: > Nit: should be 'const String&' (no space between type and &) > Please also change occurrences of this you didn't add (like below). Done. https://codereview.chromium.org/663523002/diff/180001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.idl (right): https://codereview.chromium.org/663523002/diff/180001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.idl:29: readonly attribute DOMString code; Are you refering to below initKeyboardEvent? My understanding is wez has suggested to be removed, as this interface is obsolete. If refering to KeyboardEvent() constructors, it does initialize code value, except for the one initialized by initiKeyboardEvent. Have added this API changes in runtime enabled feature. https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... File Source/web/WebInputEvent.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... Source/web/WebInputEvent.cpp:46: int keyboardData[14]; It does work with int. Earlier there was discussion if int will be enough, so modified it. I suppose it will be enough. Have changed back to int. https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:310: static void getDomCodeStringFromEnum(Widget* widget, String& code, long domCode) On 2015/01/20 17:05:58, bokan wrote: > Can we return the string, rather than using an output parameter? It'd be > clearer. Done. https://codereview.chromium.org/663523002/diff/180001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:317: } I have sent mail to Gary with the questions. For timebeing it is done now in two steps: - If some invalid key is entered: domCodeSringFromEnum, will return "Undefined" for such keys. - In case domCodeString cannot execute, it will still return "Undefined" from Blink. (Though not sure if it's right thing, will amend based on Gary's comment). https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/Keyboa... File Source/web/tests/KeyboardTest.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/Keyboa... Source/web/tests/KeyboardTest.cpp:68: WebViewImpl* webViewImpl = webViewHelper.initializeAndLoad(baseURL + fileName, true); On 2015/01/20 17:05:58, bokan wrote: > On 2015/01/17 02:35:10, Wez wrote: > > Doesn't this need cleaning up somewhere? > > webViewHelper will clean up on destruction. That said, please move webViewHelper > to be a private member of the KeyboardTest class and move the rest of the > boilerplate (registerMockedURLFromBaseURL, initializeAndLoad, etc.) into the > individual tests cases. For example of how this should look: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Done. https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/663523002/diff/180001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:70: WebViewImpl* webViewImpl = webViewHelper.initializeAndLoad(baseURL + fileName, true); On 2015/01/17 02:35:10, Wez wrote: > Do you need to tear this page down somewhere? > > It looks like you're creating a new page for every call to this helper? Acknowledged. https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClien... public/web/WebViewClient.h:137: // This method converts from the embedded-supplied domCode to On 2015/01/20 17:05:59, bokan wrote: > Nit: embedder-supplied Acknowledged. https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClien... public/web/WebViewClient.h:138: // the string value of the domCode On 2015/01/17 02:35:10, Wez wrote: > nit: You're missing the full-stop at the line end. Acknowledged. https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClien... public/web/WebViewClient.h:142: // the dom enum value On 2015/01/17 02:35:10, Wez wrote: > And you're missing the full-stop here too. > > This comment is not very clear; you mean it converts from a supplied DOM |code| > string to the embedder's enum value for that key, I think? Done. https://codereview.chromium.org/663523002/diff/180001/public/web/WebViewClien... public/web/WebViewClient.h:143: virtual long domEnumFromCodeString(const char* codeString) { return 0; } I have changed to WebString.
lgtm modulo URL mocking issues. https://codereview.chromium.org/663523002/diff/200001/Source/web/tests/Keyboa... File Source/web/tests/KeyboardTest.cpp (right): https://codereview.chromium.org/663523002/diff/200001/Source/web/tests/Keyboa... Source/web/tests/KeyboardTest.cpp:124: const std::string baseURL("http://www.test5.com/"); You'll likely get the same problem here with trying to re-register the same URL multiple times. Adding Platform::current()->unitTestSupport()->unregisterAllMockedURLs(); in the destructor should help. https://codereview.chromium.org/663523002/diff/200001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/663523002/diff/200001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:72: const std::string baseURL("http://www.test0.com/"); I seem to recall this test file failing recently because the same URL was registered twice. The test below has http://www.test0.com as well. Please number all the test URLs in this file sequentially.
Thanks bokan addressed below comments. https://codereview.chromium.org/663523002/diff/200001/Source/web/tests/Keyboa... File Source/web/tests/KeyboardTest.cpp (right): https://codereview.chromium.org/663523002/diff/200001/Source/web/tests/Keyboa... Source/web/tests/KeyboardTest.cpp:124: const std::string baseURL("http://www.test5.com/"); On 2015/01/23 15:35:25, bokan wrote: > You'll likely get the same problem here with trying to re-register the same URL > multiple times. Adding > Platform::current()->unitTestSupport()->unregisterAllMockedURLs(); in the > destructor should help. Done. https://codereview.chromium.org/663523002/diff/200001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/663523002/diff/200001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:72: const std::string baseURL("http://www.test0.com/"); Have added all test to have URL in sequential order.
A few nits about the public API comments / names (since it's important to be precise at the blink/embedder boundary). Otherwise Source/core and public/ look good (assuming Gary/Wez are happy with the semantics). You'll also need Source/web, Source/platform and Source/devtools OWNERS though. https://codereview.chromium.org/663523002/diff/220001/LayoutTests/fast/events... File LayoutTests/fast/events/keyboardevent-code.html (right): https://codereview.chromium.org/663523002/diff/220001/LayoutTests/fast/events... LayoutTests/fast/events/keyboardevent-code.html:22: lastKeyboardPressEvent = ev; nit: remove extra space https://codereview.chromium.org/663523002/diff/220001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/663523002/diff/220001/public/web/WebInputEven... public/web/WebInputEvent.h:240: // The enum code value of the key pressed as passed by the embedder. Please add a comment saying where exactly the enum is defined (even if that's in chromium code). https://codereview.chromium.org/663523002/diff/220001/public/web/WebInputEven... public/web/WebInputEvent.h:240: // The enum code value of the key pressed as passed by the embedder. nit: maybe "DOM code enum value" just to be precise about what "enum" we're talking about? https://codereview.chromium.org/663523002/diff/220001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/220001/public/web/WebViewClien... public/web/WebViewClient.h:137: // This method converts from the supplied enum value for the key I'm OK using an opaque 'int' here (there are some other examples in the blink Web API), but only if we're as precise as possible in the comments about where the meaning of this value is defined. Also, for both APIs, please define what should happen with unknown/invalid input. https://codereview.chromium.org/663523002/diff/220001/public/web/WebViewClien... public/web/WebViewClient.h:139: virtual WebString domCodeStringFromEnum(int domCode) { return ""; } Again, let's be precise about the "enum" in question here. What's the best term for consistency with chromium - "DOM code value" perhaps? Perhaps domCodeStringFromDomCodeValue?
https://codereview.chromium.org/663523002/diff/220001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/220001/public/web/WebViewClien... public/web/WebViewClient.h:139: virtual WebString domCodeStringFromEnum(int domCode) { return ""; } On 2015/01/23 20:38:30, Rick Byers wrote: > Again, let's be precise about the "enum" in question here. What's the best term > for consistency with chromium - "DOM code value" perhaps? > > Perhaps domCodeStringFromDomCodeValue? IMO "DOM code string" and "DOM code enum" make most sense; "DOM code value" sounds ambiguous as to whether it means the string or the opaque Chromium enum. kpschoedel@, does that make sense to you?
On 2015/01/23 20:59:49, Wez wrote: > IMO "DOM code string" and "DOM code enum" make most sense; "DOM code value" > sounds ambiguous as to whether it means the string or the opaque Chromium enum. > > kpschoedel@, does that make sense to you? Sounds fine. In ui/events I've most commonly written "DomCode" (the enum class name) and "DOM |code| string", but as I touch files int he future I'll update comments to whatever convention arises here.
@dgozman: Will it be okay for you to review Source/devtools changes? The changes are due to update of PlatformKeyboardEvent constructor.
habib.virji@samsung.com changed reviewers: + mkwst@chromium.org
Thanks Rick, Wez and Kevin for reviewing. I have updated comments and have followed naming convention of DOM code value and DOM code enum through out the comments. Also added web, devtools and platform code reviewers. https://codereview.chromium.org/663523002/diff/220001/LayoutTests/fast/events... File LayoutTests/fast/events/keyboardevent-code.html (right): https://codereview.chromium.org/663523002/diff/220001/LayoutTests/fast/events... LayoutTests/fast/events/keyboardevent-code.html:22: lastKeyboardPressEvent = ev; On 2015/01/23 20:38:30, Rick Byers wrote: > nit: remove extra space Done. https://codereview.chromium.org/663523002/diff/220001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/663523002/diff/220001/public/web/WebInputEven... public/web/WebInputEvent.h:240: // The enum code value of the key pressed as passed by the embedder. On 2015/01/23 20:38:30, Rick Byers wrote: > Please add a comment saying where exactly the enum is defined (even if that's in > chromium code). Done. https://codereview.chromium.org/663523002/diff/220001/public/web/WebInputEven... public/web/WebInputEvent.h:240: // The enum code value of the key pressed as passed by the embedder. On 2015/01/23 20:38:30, Rick Byers wrote: > nit: maybe "DOM code enum value" just to be precise about what "enum" we're > talking about? Done. https://codereview.chromium.org/663523002/diff/220001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/663523002/diff/220001/public/web/WebViewClien... public/web/WebViewClient.h:137: // This method converts from the supplied enum value for the key On 2015/01/23 20:38:30, Rick Byers wrote: > I'm OK using an opaque 'int' here (there are some other examples in the blink > Web API), but only if we're as precise as possible in the comments about where > the meaning of this value is defined. > > Also, for both APIs, please define what should happen with unknown/invalid > input. Done. https://codereview.chromium.org/663523002/diff/220001/public/web/WebViewClien... public/web/WebViewClient.h:139: virtual WebString domCodeStringFromEnum(int domCode) { return ""; } On 2015/01/23 20:38:30, Rick Byers wrote: > Again, let's be precise about the "enum" in question here. What's the best term > for consistency with chromium - "DOM code value" perhaps? > > Perhaps domCodeStringFromDomCodeValue? Done.
Thanks Rick, Wez and Kevin for reviewing. I have updated comments and have followed naming convention of DOM code value and DOM code enum through out the comments. Also added web, devtools and platform code reviewers.
@Mike West: Can you please review web and platform related changes for this CL. Thanks.
dgozman@chromium.org changed reviewers: + pfeldman@chromium.org
core/inspector lgtm +pfeldman@ for devtools/protocol.json
habib.virji@samsung.com changed reviewers: + jochen@chromium.org
devtools lgtm
Code has been updated as Antoine (piman) has suggested to use blink::Platform instead of WebViewClient. Reason being "It doesn't access any field from RenderViewImpl. The returned values don't depend on which RenderView this is talking to." Previously a new parameter was added in WebKeyboardEvent and PlatformEvent in WebInputConversion, this is no longer required as we are calling embedder API using blink::Platform. @pfeldman: devtool is not changed. @dgozman: core/inspector is not changed.
https://codereview.chromium.org/663523002/diff/260001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.cpp (right): https://codereview.chromium.org/663523002/diff/260001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.cpp:125: const String& keyIdentifier, const String& code, unsigned location, bool ctrlKey, bool altKey, bool shiftKey, bool metaKey) Nit: Why reformat this? https://codereview.chromium.org/663523002/diff/260001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.cpp:139: const String& keyIdentifier, unsigned location, bool ctrlKey, bool altKey, bool shiftKey, bool metaKey) Nit: Why reformat this? https://codereview.chromium.org/663523002/diff/260001/public/platform/Platform.h File public/platform/Platform.h (right): https://codereview.chromium.org/663523002/diff/260001/public/platform/Platfor... public/platform/Platform.h:611: virtual WebString domCodeStringFromEnum(int domCode) { return WebString(); } "DOM Code String" is ambiguous. Could you rename these to something which makes it clear that they're key-code related? https://codereview.chromium.org/663523002/diff/260001/public/platform/Platfor... public/platform/Platform.h:617: virtual int domEnumFromCodeString(const WebString& codeString) { return 0; } It's a bit strange that we're calling this an enum, when it's completely opaque to Blink. Blink has an int, and doesn't have any understanding of the enum that lives at the platform layer. Can the enum live at the platform layer instead? https://codereview.chromium.org/663523002/diff/260001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/663523002/diff/260001/public/web/WebInputEven... public/web/WebInputEvent.h:242: int domCode; Nit: "domKeyCode"? https://codereview.chromium.org/663523002/diff/260001/public/web/WebViewClient.h File public/web/WebViewClient.h (left): https://codereview.chromium.org/663523002/diff/260001/public/web/WebViewClien... public/web/WebViewClient.h:137: Nit: This doesn't have anything to do with the current patch, right? Please drop it to keep the diffs clean.
Thanks Mike, have replied to your comments inline. https://codereview.chromium.org/663523002/diff/260001/Source/core/events/Keyb... File Source/core/events/KeyboardEvent.cpp (right): https://codereview.chromium.org/663523002/diff/260001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.cpp:125: const String& keyIdentifier, const String& code, unsigned location, bool ctrlKey, bool altKey, bool shiftKey, bool metaKey) Since it was more than 4 spaces, pre-submit was giving error. https://codereview.chromium.org/663523002/diff/260001/Source/core/events/Keyb... Source/core/events/KeyboardEvent.cpp:139: const String& keyIdentifier, unsigned location, bool ctrlKey, bool altKey, bool shiftKey, bool metaKey) Since it was more than 4 spaces, pre-submit was giving error. https://codereview.chromium.org/663523002/diff/260001/public/platform/Platform.h File public/platform/Platform.h (right): https://codereview.chromium.org/663523002/diff/260001/public/platform/Platfor... public/platform/Platform.h:611: virtual WebString domCodeStringFromEnum(int domCode) { return WebString(); } Will it be okay to rename them DOMKeyboardCodeString? This was suggestion which wez suggested. This was kept to keep it consistent with chromium. https://codereview.chromium.org/663523002/diff/260001/public/platform/Platfor... public/platform/Platform.h:617: virtual int domEnumFromCodeString(const WebString& codeString) { return 0; } We went through that discussion https://codereview.chromium.org/663523002/#msg43, but wez suggested to have it in chromium side. https://codereview.chromium.org/663523002/diff/260001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/663523002/diff/260001/public/web/WebInputEven... public/web/WebInputEvent.h:242: int domCode; There are 2 DOM3 Keyboard Event specs, http://www.w3.org/TR/DOM-Level-3-Events-code/ and http://www.w3.org/TR/DOM-Level-3-Events-key/. Perhaps, when DOM3-Event-Key it will be named domKey. Then it will become bit difficult to differentiate between these two. https://codereview.chromium.org/663523002/diff/260001/public/web/WebViewClient.h File public/web/WebViewClient.h (left): https://codereview.chromium.org/663523002/diff/260001/public/web/WebViewClien... public/web/WebViewClient.h:137: That's true, will push in new change.
Thanks for moving the API to blink::Platform - I agree this is better, sorry I didn't suggest it. I'm happy with public/ and will stamp it if mkwst is satisfied with your responses. I don't want to nitpick TOO much over the names :-) https://codereview.chromium.org/663523002/diff/260001/public/platform/Platform.h File public/platform/Platform.h (right): https://codereview.chromium.org/663523002/diff/260001/public/platform/Platfor... public/platform/Platform.h:617: virtual int domEnumFromCodeString(const WebString& codeString) { return 0; } On 2015/01/29 12:24:14, Habib Virji wrote: > We went through that discussion > https://codereview.chromium.org/663523002/#msg43, but wez suggested to have it > in chromium side. Yeah, I thought Wez's arguments for making it opaque to blink were convincing. We could call it something other than "enum" if we wanted, but chromium calls it that so I'm happy to defer to the chromium keyboard experts (eg. Wez) for naming consistency.
LGTM. Thanks for the explanations.
On 2015/01/30 18:42:15, Mike West wrote: > LGTM. Thanks for the explanations. Great. public/ also LGTM
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663523002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41062)
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663523002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189514 |
