ChromeDriver: Handle key events properly on Mac

content/browser/devtools/protocol/native_input_event_builder_mac.mm

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/strings/sys_string_conversions.h"
+#include "content/browser/devtools/protocol/native_input_event_builder.h"
+#include "third_party/WebKit/public/platform/WebInputEvent.h"
+#include <Cocoa/Cocoa.h>
+
+namespace content {
+namespace protocol {
+
+// Mac requires a native event to emulate key events. This method gives
+// only crude capabilities (see: crbug.com/667387).
+gfx::NativeEvent NativeInputEventBuilder::Build(
+    const NativeWebKeyboardEvent& event) {
+  NSEventType type = NSKeyUp;
+  if (event.type() == blink::WebInputEvent::RawKeyDown ||
+      event.type() == blink::WebInputEvent::KeyDown)
+    type = NSKeyDown;
+  const blink::WebUChar* textStartAddr = &event.text[0];
+  const int textLength =
+      std::find(textStartAddr,
+                textStartAddr + NativeWebKeyboardEvent::textLengthCap, '\0') -

[erikchen, 2017/02/15 22:21:16]

What if '\0' isn't found? It looks like textLength is set to
NativeWebKeyboardEvent::textLengthCap, which is probably not what you want?

[allada, 2017/02/17 18:30:58]

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

[erikchen, 2017/02/17 19:15:50]

My point was that if you don't find '\0', you probably want to error out, or
perhaps set length = 0?

[allada, 2017/03/06 23:34:33]

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.

+      textStartAddr;
+  NSString* character =
+      base::SysUTF16ToNSString(base::string16(textStartAddr, textLength));

[erikchen, 2017/02/15 22:21:16]

This could be more than one character. Do you want to check that there's only 1?

[allada, 2017/02/17 18:30:58]

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"

+  int modifiers = event.modifiers();
+  NSUInteger flags =
+      (modifiers & blink::WebInputEvent::ShiftKey ? NSShiftKeyMask : 0) |
+      (modifiers & blink::WebInputEvent::ControlKey ? NSControlKeyMask : 0) |
+      (modifiers & blink::WebInputEvent::AltKey ? NSAlternateKeyMask : 0) |
+      (modifiers & blink::WebInputEvent::MetaKey ? NSCommandKeyMask : 0);
+
+  return [[NSEvent keyEventWithType:type
+                           location:NSZeroPoint
+                      modifierFlags:flags
+                          timestamp:0
+                       windowNumber:0

[erikchen, 2017/02/15 22:21:16]

Is it okay to leave all these fields empty? 

[allada, 2017/02/17 18:30:58]

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?

[erikchen, 2017/02/17 19:15:49]

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?

[allada, 2017/03/06 23:34:33]

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".

+                            context:nil
+                         characters:character
+        charactersIgnoringModifiers:character
+                          isARepeat:NO
+                            keyCode:event.nativeKeyCode] retain];

[erikchen, 2017/02/15 22:21:16]

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.

[allada, 2017/02/17 18:30:58]

Per our offline discussion, I made appropriate changes.

+};
+
+}  // namespace protocol
+}  // namespace content