Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2656903005:
ChromeDriver: Handle key events properly on Mac (Closed)
DescriptionChromeDriver: Handle key events properly on Mac
Fixes devtool's emulation of key input events to emulate
os key event for OSX.
BUG=667387
Review-Url: https://codereview.chromium.org/2656903005
Cr-Commit-Position: refs/heads/master@{#478407}
Committed: https://chromium.googlesource.com/chromium/src/+/fa84ab9619de73a61d305c7459fc186e99b0d291
Patch Set 1 : ChromeDriver: Handle key events properly on Mac #
Total comments: 6
Patch Set 2 : changes #
Total comments: 2
Patch Set 3 : changes #
Total comments: 8
Patch Set 4 : changes #
Total comments: 12
Patch Set 5 : changes #
Total comments: 2
Patch Set 6 : fixes #Patch Set 7 : fixes #
Messages
Total messages: 81 (58 generated)
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix keybindings stuff BUG= ========== to ========== Fix keybindings stuff BUG= ==========
allada@chromium.org changed reviewers: + dgozman@chromium.org
Description was changed from ========== Fix keybindings stuff BUG= ========== to ========== ChromeDriver: Handle key events properly on Mac Fixes ChromeDriver's browser no-check policy for keyboard events. Key events should be properly handled in ChromeDriver. BUG=667387 ==========
allada@chromium.org changed reviewers: + rohitrao@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
allada@chromium.org changed reviewers: + pfeldman@chromium.org
PTL
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
Patchset #1 (id:260001) has been deleted
Patchset #1 (id:280001) has been deleted
Description was changed from ========== ChromeDriver: Handle key events properly on Mac Fixes ChromeDriver's browser no-check policy for keyboard events. Key events should be properly handled in ChromeDriver. BUG=667387 ========== to ========== ChromeDriver: Handle key events properly on Mac Fixes ChromeDriver's browser no-check policy for keyboard events. Key events should be properly handled in ChromeDriver. BUG=667387 ==========
Description was changed from ========== ChromeDriver: Handle key events properly on Mac Fixes ChromeDriver's browser no-check policy for keyboard events. Key events should be properly handled in ChromeDriver. BUG=667387 ========== to ========== ChromeDriver: Handle key events properly on Mac Fixes devtool's emulation of key input events to emulate os key event for OSX. BUG=667387 ==========
allada@chromium.org changed reviewers: + luoe@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2656903005/diff/300001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder_mac.mm (right): https://codereview.chromium.org/2656903005/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:16: NSData* data = [[[NSData alloc] initWithBytes:&event.text[0] Use SysUTF16ToNSString. Also, did event.text instead of &event.text[0] fail compile? https://codereview.chromium.org/2656903005/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:20: return [[NSEvent keyEventWithType:NSKeyDown Why is this always keydown? https://codereview.chromium.org/2656903005/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:22: modifierFlags:0 You should probably specify all of these.
PTaL https://codereview.chromium.org/2656903005/diff/300001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder_mac.mm (right): https://codereview.chromium.org/2656903005/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:16: NSData* data = [[[NSData alloc] initWithBytes:&event.text[0] On 2017/02/08 18:57:56, pfeldman wrote: > Use SysUTF16ToNSString. Also, did event.text instead of &event.text[0] fail > compile? Yes, because it's a fixes size array. https://codereview.chromium.org/2656903005/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:20: return [[NSEvent keyEventWithType:NSKeyDown On 2017/02/08 18:57:56, pfeldman wrote: > Why is this always keydown? I was trying to only implement minimal work, since the code doesn't current actually use this os_event, but it does need it for some DCHECKS. Fixed though. https://codereview.chromium.org/2656903005/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:22: modifierFlags:0 On 2017/02/08 18:57:56, pfeldman wrote: > You should probably specify all of these. Done. modeled after: https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/accelerator_util...
https://codereview.chromium.org/2656903005/diff/320001/base/strings/sys_strin... File base/strings/sys_string_conversions.h (right): https://codereview.chromium.org/2656903005/diff/320001/base/strings/sys_strin... base/strings/sys_string_conversions.h:71: BASE_EXPORT NSString* SysUTF16ToNSString(const string16& utf16, string16 has a length, this is not necessary.
Patchset #3 (id:340001) has been deleted
PTaL https://codereview.chromium.org/2656903005/diff/320001/base/strings/sys_strin... File base/strings/sys_string_conversions.h (right): https://codereview.chromium.org/2656903005/diff/320001/base/strings/sys_strin... base/strings/sys_string_conversions.h:71: BASE_EXPORT NSString* SysUTF16ToNSString(const string16& utf16, On 2017/02/10 19:39:27, pfeldman wrote: > string16 has a length, this is not necessary. Done.
https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder.h (right): https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder.h:16: static gfx::NativeEvent Build(const NativeWebKeyboardEvent& event); These are the same, ifdef in mm instead. https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder_mac.mm (right): https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:20: type = NSKeyUp; NSKeyDown https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:22: NativeWebKeyboardEvent::textLengthCap)); This will force it to be NativeWebKeyboardEvent::textLengthCap even though it was 0-terminated. https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:33: timestamp:0 There is still no timestamp.
rohitrao@chromium.org changed reviewers: + erikchen@chromium.org
erikchen would be a better reviewer than me for event-related code.
The CQ bit was checked by allada@chromium.org to run a CQ dry run
PTL https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder.h (right): https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder.h:16: static gfx::NativeEvent Build(const NativeWebKeyboardEvent& event); On 2017/02/10 22:59:07, pfeldman wrote: > These are the same, ifdef in mm instead. Per our offline talk, the optimizer should optimize out the nullptr part if not mac. If it is mac it will bind in mm file. https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder_mac.mm (right): https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:20: type = NSKeyUp; On 2017/02/10 22:59:08, pfeldman wrote: > NSKeyDown Done. https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:22: NativeWebKeyboardEvent::textLengthCap)); On 2017/02/10 22:59:08, pfeldman wrote: > This will force it to be NativeWebKeyboardEvent::textLengthCap even though it > was 0-terminated. Done. https://codereview.chromium.org/2656903005/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:33: timestamp:0 On 2017/02/10 22:59:08, pfeldman wrote: > There is still no timestamp. no change per our offline talk.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping @erikchen
only reviewed content/ https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder_mac.mm (right): https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:24: textStartAddr + NativeWebKeyboardEvent::textLengthCap, '\0') - What if '\0' isn't found? It looks like textLength is set to NativeWebKeyboardEvent::textLengthCap, which is probably not what you want? https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:27: base::SysUTF16ToNSString(base::string16(textStartAddr, textLength)); This could be more than one character. Do you want to check that there's only 1? https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:39: windowNumber:0 Is it okay to leave all these fields empty? https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:44: keyCode:event.nativeKeyCode] retain]; This code *looks* like it will leak. That's dealt with here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/native_web... These ownership semantics are very unclear. :( I recommend that you return a base::scoped_nsobject from this function, and then explicitly release ownership to the os_event property, which at least doesn't make this function leak when used by other callers.
PTaL @erikchen https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder_mac.mm (right): https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:24: textStartAddr + NativeWebKeyboardEvent::textLengthCap, '\0') - On 2017/02/15 22:21:16, erikchen wrote: > What if '\0' isn't found? It looks like textLength is set to > NativeWebKeyboardEvent::textLengthCap, which is probably not what you want? In this case it is what I want. event.text is a fixed size wchar array. To ensure we only grab the char and do not have potential crash of looking for null character we cap it at NativeWebKeyboardEvent::textLengthCap. See: http://crbug.com/690181 https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:27: base::SysUTF16ToNSString(base::string16(textStartAddr, textLength)); On 2017/02/15 22:21:16, erikchen wrote: > This could be more than one character. Do you want to check that there's only 1? Yes it could be more than 1 character, from what I was reading online it looks like it can accept multiple characters. See: https://developer.apple.com/reference/appkit/nsevent/1533943-keyeventwithtype... under "characters" https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:39: windowNumber:0 On 2017/02/15 22:21:16, erikchen wrote: > Is it okay to leave all these fields empty? It was modeled mostly after this: https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/accelerator_util... I will fill them if you believe strongly, but it does work for my use case and done this way in other code. wdyt? https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:44: keyCode:event.nativeKeyCode] retain]; On 2017/02/15 22:21:16, erikchen wrote: > This code *looks* like it will leak. That's dealt with here: > https://cs.chromium.org/chromium/src/content/browser/renderer_host/native_web... > > These ownership semantics are very unclear. :( > > I recommend that you return a base::scoped_nsobject from this function, and then > explicitly release ownership to the os_event property, which at least doesn't > make this function leak when used by other callers. Per our offline discussion, I made appropriate changes.
https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder_mac.mm (right): https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:24: textStartAddr + NativeWebKeyboardEvent::textLengthCap, '\0') - On 2017/02/17 18:30:58, allada wrote: > On 2017/02/15 22:21:16, erikchen wrote: > > What if '\0' isn't found? It looks like textLength is set to > > NativeWebKeyboardEvent::textLengthCap, which is probably not what you want? > > In this case it is what I want. event.text is a fixed size wchar array. To > ensure we only grab the char and do not have potential crash of looking for null > character we cap it at NativeWebKeyboardEvent::textLengthCap. > > See: http://crbug.com/690181 My point was that if you don't find '\0', you probably want to error out, or perhaps set length = 0? https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:39: windowNumber:0 On 2017/02/17 18:30:58, allada wrote: > On 2017/02/15 22:21:16, erikchen wrote: > > Is it okay to leave all these fields empty? > > It was modeled mostly after this: > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/accelerator_util... > > I will fill them if you believe strongly, but it does work for my use case and > done this way in other code. wdyt? That event is just being used as a wrapper as a call to another chrome-defined function. If this event is being passed to AppKit functions, I'd be pretty worried if these fields weren't filled in. I assume the reason we need an NSEvent is that we are, at some point, passing this event to AppKit. Where does that happen?
ping
On 2017/02/23 01:11:24, pfeldman wrote: > ping I asked a question that was never responded to. """ That event is just being used as a wrapper as a call to another chrome-defined function. If this event is being passed to AppKit functions, I'd be pretty worried if these fields weren't filled in. I assume the reason we need an NSEvent is that we are, at some point, passing this event to AppKit. Where does that happen? Reply """
(Yes, I was pinging allada@, sorry for the confusion!)
PTaL @erikchen Sorry for the long delay, I had some stuff I had to get out. https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... File content/browser/devtools/protocol/native_input_event_builder_mac.mm (right): https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:24: textStartAddr + NativeWebKeyboardEvent::textLengthCap, '\0') - On 2017/02/17 19:15:50, erikchen wrote: > On 2017/02/17 18:30:58, allada wrote: > > On 2017/02/15 22:21:16, erikchen wrote: > > > What if '\0' isn't found? It looks like textLength is set to > > > NativeWebKeyboardEvent::textLengthCap, which is probably not what you want? > > > > In this case it is what I want. event.text is a fixed size wchar array. To > > ensure we only grab the char and do not have potential crash of looking for > null > > character we cap it at NativeWebKeyboardEvent::textLengthCap. > > > > See: http://crbug.com/690181 > > My point was that if you don't find '\0', you probably want to error out, or > perhaps set length = 0? If we don't find \0 I think we do want to use all available characters. The code around this does not seem to be very documented and is quite old (well into webkit days). I do not see much explicitly saying it must be null terminated. I only want to be protected from overflows. https://codereview.chromium.org/2656903005/diff/380001/content/browser/devtoo... content/browser/devtools/protocol/native_input_event_builder_mac.mm:39: windowNumber:0 On 2017/02/17 19:15:49, erikchen wrote: > On 2017/02/17 18:30:58, allada wrote: > > On 2017/02/15 22:21:16, erikchen wrote: > > > Is it okay to leave all these fields empty? > > > > It was modeled mostly after this: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/accelerator_util... > > > > I will fill them if you believe strongly, but it does work for my use case and > > done this way in other code. wdyt? > > That event is just being used as a wrapper as a call to another chrome-defined > function. If this event is being passed to AppKit functions, I'd be pretty > worried if these fields weren't filled in. > > I assume the reason we need an NSEvent is that we are, at some point, passing > this event to AppKit. Where does that happen? I do not believe it does get passed to AppKit. The major reason we need it is because there are some DCHECKS and some places extract data out of it when it's present. Our only for ChromeDriver to ensure browser level events get triggered so developers can test against it (like autofill). I did a bit of research into what it'd take to get the value here and it looks like it might not be worth it until it is needed? I do have a message at the function declaration stating that it's "crude capabilities".
I'm getting pinged about this because it's blocking others. Can I get another review?
lgtm
devtools lgtm https://codereview.chromium.org/2656903005/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/Tests.js (right): https://codereview.chromium.org/2656903005/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/Tests.js:713: SDK.multitargetConsoleModel.addEventListener(SDK.ConsoleModel.Events.MessageAdded, onConsoleMessage, this); This is ConsoleModel.consoleModel now.
done https://codereview.chromium.org/2656903005/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/Tests.js (right): https://codereview.chromium.org/2656903005/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/Tests.js:713: SDK.multitargetConsoleModel.addEventListener(SDK.ConsoleModel.Events.MessageAdded, onConsoleMessage, this); On 2017/06/08 23:52:27, dgozman wrote: > This is ConsoleModel.consoleModel now. Done.
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #6 (id:420001) has been deleted
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2656903005/#ps440001 (title: "fixes")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1497034087325980,
"parent_rev": "f9be8cca22fd8b50adee2dab1767c249a273b76f", "commit_rev":
"fa84ab9619de73a61d305c7459fc186e99b0d291"}
Message was sent while issue was closed.
Description was changed from ========== ChromeDriver: Handle key events properly on Mac Fixes devtool's emulation of key input events to emulate os key event for OSX. BUG=667387 ========== to ========== ChromeDriver: Handle key events properly on Mac Fixes devtool's emulation of key input events to emulate os key event for OSX. BUG=667387 Review-Url: https://codereview.chromium.org/2656903005 Cr-Commit-Position: refs/heads/master@{#478407} Committed: https://chromium.googlesource.com/chromium/src/+/fa84ab9619de73a61d305c7459fc... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:440001) as https://chromium.googlesource.com/chromium/src/+/fa84ab9619de73a61d305c7459fc...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:440001) has been created in https://codereview.chromium.org/2932223002/ by thakis@chromium.org. The reason for reverting is: Speculative, might have caused https://crbug.com/732053. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
