[CRD iOS] Send key events to the session.

[CRD iOS] Send key events to the session.

This CL enables the iOS client to send key events (keyboard interactions) to
the remote host. This is the first pass for milestone 1, with the goal of a rough 
session experience and a follow up CL will come at a later date to polish this.

Review-Url: https://codereview.chromium.org/2868383003
Cr-Commit-Position: refs/heads/master@{#473677}
Committed: https://chromium.googlesource.com/chromium/src/+/1e49b618d05df533363bf177dc99bd1da871f76d

[nicholss, 2017-05-12 18:54:15]

nicholss@chromium.org changed reviewers:
+ yuweih@chromium.org

[nicholss, 2017-05-12 18:54:16]

PTAL, thanks!

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/B...
File remoting/client/input/BUILD.gn (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/B...
remoting/client/input/BUILD.gn:5: source_set("input") {
Note we should start moving input related client code into this place.

Also note, the existing input_strategy is too generic, it should be renamed to
touch_input_strategy or pointer_input_strategy (something like that)

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
File remoting/client/input/keyboard_input_strategy.h (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_input_strategy.h:13: #define ALT_RIGHT 0x01
These are not used yet.

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
File remoting/client/input/keyboard_interpreter.h (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_interpreter.h:26: void HandleTextEvent(const
std::string& text, uint8_t modifiers);
Modifiers are not used yet.

[joedow, 2017-05-12 19:13:16]

joedow@chromium.org changed reviewers:
+ joedow@chromium.org

[joedow, 2017-05-12 19:13:18]

Added some drive-by comments.  It would probably be good to have Gary take a
look as well since he is so familiar with keyboard input.

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/chromot...
File remoting/client/chromoting_session.cc (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/chromot...
remoting/client/chromoting_session.cc:210: }
Drive-by: This code looks a bit like the function above (param names) and the
above function has logic for 'software keyboards'.  Why not implement
'NativeDeviceKeycodeToUsbKeycode' for iOS and reuse the existing method?

To add on, if you decide to keep this method, please document it so people
reading the code know why this override was added (since it is similar to the
method above).

Edit: I just saw that you implemented that method for iOS, do you still need a
new method here?

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
File remoting/client/input/keyboard_input_strategy.h (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_input_strategy.h:13: #define ALT_RIGHT 0x01
On 2017/05/12 18:54:16, nicholss wrote:
> These are not used yet.

drive-by: other native code uses ui::DomCode::ALT_RIGHT as an id, why is this
different?

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_input_strategy.h:19: struct KeyEvent {
Could you use the KeyEvent struct in the protobuf instead of defining a new
type?

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
File remoting/client/input/keyboard_interpreter.h (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_interpreter.h:11: 
nit: remove newline

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_interpreter.h:21:
KeyboardInterpreter(ChromotingSession* input_stub);
single param c'tors should be marked explicit.

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/native_...
File remoting/client/native_device_keymap.cc (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/native_...
remoting/client/native_device_keymap.cc:16: return NULL;
nit: use nullptr instead.

[nicholss, 2017-05-12 20:45:58]

OMG I just added the text keyboard and you can send emoji now.

PTAL.

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/chromot...
File remoting/client/chromoting_session.cc (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/chromot...
remoting/client/chromoting_session.cc:210: }
On 2017/05/12 19:13:17, joedow wrote:
> Drive-by: This code looks a bit like the function above (param names) and the
> above function has logic for 'software keyboards'.  Why not implement
> 'NativeDeviceKeycodeToUsbKeycode' for iOS and reuse the existing method?
> 
> To add on, if you decide to keep this method, please document it so people
> reading the code know why this override was added (since it is similar to the
> method above).
> 
> Edit: I just saw that you implemented that method for iOS, do you still need a
> new method here?

I have updated to use usb_key_code as the param name, the android version sends
device keycode which we do not have in iOS and need to convert ascii to usb
keycode directly. The above function assumes to much.

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
File remoting/client/input/keyboard_input_strategy.h (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_input_strategy.h:13: #define ALT_RIGHT 0x01
On 2017/05/12 19:13:17, joedow wrote:
> On 2017/05/12 18:54:16, nicholss wrote:
> > These are not used yet.
> 
> drive-by: other native code uses ui::DomCode::ALT_RIGHT as an id, why is this
> different?

those are not what I want. I am making a bit mask to send in so I can say send
this key event and wrap it with some modifiers.

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_input_strategy.h:19: struct KeyEvent {
On 2017/05/12 19:13:17, joedow wrote:
> Could you use the KeyEvent struct in the protobuf instead of defining a new
> type?

Maybe.  the side-effects of the fields in the proto are more than I want to deal
with so, yes I can use it as a shell, but it will not be an honest usage of the
data it might contain.

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
File remoting/client/input/keyboard_interpreter.h (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_interpreter.h:11: 
On 2017/05/12 19:13:17, joedow wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/input/k...
remoting/client/input/keyboard_interpreter.h:21:
KeyboardInterpreter(ChromotingSession* input_stub);
On 2017/05/12 19:13:17, joedow wrote:
> single param c'tors should be marked explicit.

Done.

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/native_...
File remoting/client/native_device_keymap.cc (right):

https://codereview.chromium.org/2868383003/diff/80001/remoting/client/native_...
remoting/client/native_device_keymap.cc:16: return NULL;
On 2017/05/12 19:13:17, joedow wrote:
> nit: use nullptr instead.

Acknowledged.

[Yuwei, 2017-05-15 19:38:07]

Thanks for adding support for the poop emoji :)

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_session.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/chromo...
remoting/client/chromoting_session.cc:208: SendKeyEventInternal(usb_key_code,
key_down);
Could we just move the SendKeyEventInternal implementation here?

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_session.h (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/chromo...
remoting/client/chromoting_session.h:123: bool SendKeyEvent(int usb_key_code,
bool key_down);
FYI it's not recommended to overload functions in Chromium. It could also be
confusing to the reader when they read both SendKeyEvent(int, int, int) and
SendKeyEvent(int, int) and had hard time realizing the difference between native
keycode and usb keycode O_o

Not sure what's the best name though. SendKeyEventWithUsbkeycode?

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/simple_keyboard_input_strategy.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/simple_keyboard_input_strategy.cc:34:
input_stub_->SendKeyEvent(0, key.keycode, key.keydown);
In case of the backspace key, could we just call SendKeyEvent(int, int)?

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/simple_keyboard_input_strategy.cc:52:
keys.push(modifier_down);
Could you just use list-initialization, i.e. `keys.push({keycode.modifier,
true})`?

Ditto below.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/simple_keyboard_input_strategy.h (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/simple_keyboard_input_strategy.h:16: class
SimpleKeyboardInputStrategy : public KeyboardInputStrategy {
Could you document why is this strategy "simple" compared to the text input
strategy?

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/text_keyboard_input_strategy.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/text_keyboard_input_strategy.cc:40: KeyEvent key_down;
push({...})?

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
File remoting/client/native_device_keymap.h (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
remoting/client/native_device_keymap.h:20: KeycodeWithModifier
AsciiToUsbKeycodeWithModifier(const char& ascii);
A char is not harder to copy than a pointer. just use char instead of const
char&?

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
File remoting/client/native_device_keymap_ios.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
remoting/client/native_device_keymap_ios.cc:153: NOTIMPLEMENTED();
input_stub_->SendKeyEvent(0, key.keycode, key.keydown) will call into this?

https://codereview.chromium.org/2868383003/diff/200001/remoting/ios/app/clien...
File remoting/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/ios/app/clien...
remoting/ios/app/client_connection_view_controller.mm:34: static const CGFloat
kKeyboardAnimationTime = 0.3;
0.3f? Although the compiler might be clever enough to skip the conversion...

https://codereview.chromium.org/2868383003/diff/200001/remoting/ios/app/host_...
File remoting/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/ios/app/host_...
remoting/ios/app/host_view_controller.mm:25: static const CGFloat
kKeyboardAnimationTime = 0.3;
f?

[Do not use (sergeyu), 2017-05-15 22:15:11]

sergeyu@google.com changed reviewers:
+ sergeyu@google.com

[Do not use (sergeyu), 2017-05-15 22:15:12]

This CL is rather big. Is it possible to split it into multiple CLs? E.g. you
could first land KeyboardInterpreter and then enable it in a separate CL.

Pleases add at least some unittests for the new classes.

Also CL description doesn't make it clear what's the goal of this CL. See
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
for some suggestions

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/keyboard_input_strategy.h (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/keyboard_input_strategy.h:25: virtual void
HandleTextEvent(const std::string& text, uint8_t modifiers) = 0;
Please add documentation for modifiers. Where are the corresponding values
defined? Is there any reason it needs to be uint8_t instead of int?

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/keyboard_input_strategy.h:31: #endif  //
REMOTING_CLIENT_INPUT_KEYBOARD_INPUT_STRATEGY_H_
nit: empty line here please. Same in other files

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/keyboard_interpreter.h (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/keyboard_interpreter.h:17: // This is a class for
interpreting raw keyboard input.
This comment is doesn't add much information from what one can get just by
looking at the class name. Please extend it to make it clear what this class is
responsible for and how it's should be used.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
File remoting/client/native_device_keymap.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
remoting/client/native_device_keymap.cc:16: KeycodeWithModifier
AsciiToUsbKeycodeWithModifier(const char& ascii) {
I don't think we want to use reference for this parameter. Char is cheaper to
pass by value

[nicholss, 2017-05-15 23:06:11]

I have removed some things from this CL, I just don't need some of the ascii to
usbcode code anymore.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_session.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/chromo...
remoting/client/chromoting_session.cc:208: SendKeyEventInternal(usb_key_code,
key_down);
On 2017/05/15 19:38:06, Yuwei wrote:
> Could we just move the SendKeyEventInternal implementation here?

I realized I don't need to do this anymore.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/chromo...
File remoting/client/chromoting_session.h (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/chromo...
remoting/client/chromoting_session.h:123: bool SendKeyEvent(int usb_key_code,
bool key_down);
On 2017/05/15 19:38:06, Yuwei wrote:
> FYI it's not recommended to overload functions in Chromium. It could also be
> confusing to the reader when they read both SendKeyEvent(int, int, int) and
> SendKeyEvent(int, int) and had hard time realizing the difference between
native
> keycode and usb keycode O_o
> 
> Not sure what's the best name though. SendKeyEventWithUsbkeycode?

Acknowledged.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/keyboard_input_strategy.h (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/keyboard_input_strategy.h:25: virtual void
HandleTextEvent(const std::string& text, uint8_t modifiers) = 0;
On 2017/05/15 22:15:12, Do not use (sergeyu) wrote:
> Please add documentation for modifiers. Where are the corresponding values
> defined? Is there any reason it needs to be uint8_t instead of int?

I am not sure how I am going to use them yet. The original plan was to use a bit
mask hence why I am using an unsigned int here. But the implementation details
for modifiers is set for next milestone so I wanted to pass on dealing with
them. There is a todo in the impl and I will document what we decide at that
time.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/keyboard_input_strategy.h:31: #endif  //
REMOTING_CLIENT_INPUT_KEYBOARD_INPUT_STRATEGY_H_
On 2017/05/15 22:15:12, Do not use (sergeyu) wrote:
> nit: empty line here please. Same in other files

Strange, I have one locally in all files.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/keyboard_interpreter.h (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/keyboard_interpreter.h:17: // This is a class for
interpreting raw keyboard input.
On 2017/05/15 22:15:12, Do not use (sergeyu) wrote:
> This comment is doesn't add much information from what one can get just by
> looking at the class name. Please extend it to make it clear what this class
is
> responsible for and how it's should be used.

I am not sure I like having the indirection anymore, I can remove this class
entirely now. The original plan was to deal with keyboard events with keycodes
but then I found the new method to send text events... So I was waiting to see
how the implementation worked out to see if I still needed this class to help
out. I am not sure yet.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/simple_keyboard_input_strategy.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/simple_keyboard_input_strategy.cc:52:
keys.push(modifier_down);
On 2017/05/15 19:38:06, Yuwei wrote:
> Could you just use list-initialization, i.e. `keys.push({keycode.modifier,
> true})`?
> 
> Ditto below.

fancy!

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/simple_keyboard_input_strategy.h (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/simple_keyboard_input_strategy.h:16: class
SimpleKeyboardInputStrategy : public KeyboardInputStrategy {
On 2017/05/15 19:38:06, Yuwei wrote:
> Could you document why is this strategy "simple" compared to the text input
> strategy?

It was simple because it was just ascii... I think I will just delete it.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/text_keyboard_input_strategy.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/text_keyboard_input_strategy.cc:40: KeyEvent key_down;
On 2017/05/15 19:38:06, Yuwei wrote:
> push({...})?

Acknowledged.

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
File remoting/client/native_device_keymap.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
remoting/client/native_device_keymap.cc:16: KeycodeWithModifier
AsciiToUsbKeycodeWithModifier(const char& ascii) {
On 2017/05/15 22:15:12, Do not use (sergeyu) wrote:
> I don't think we want to use reference for this parameter. Char is cheaper to
> pass by value

I don't need the method anymore. But good to know!

https://codereview.chromium.org/2868383003/diff/200001/remoting/ios/app/clien...
File remoting/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/ios/app/clien...
remoting/ios/app/client_connection_view_controller.mm:34: static const CGFloat
kKeyboardAnimationTime = 0.3;
On 2017/05/15 19:38:07, Yuwei wrote:
> 0.3f? Although the compiler might be clever enough to skip the conversion...

##.f should be the same as ##.0, but style wise I have seen more ##.f if it is a
whole number and 0.# if < 1.0

So .f is only needed for whole numbers

[Yuwei, 2017-05-16 00:47:30]

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/text_keyboard_input_strategy.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/text_keyboard_input_strategy.cc:40: KeyEvent key_down;
On 2017/05/15 23:06:11, nicholss wrote:
> On 2017/05/15 19:38:06, Yuwei wrote:
> > push({...})?
> 
> Acknowledged.

You probably want to use "Done" for things you have done :)

https://codereview.chromium.org/2868383003/diff/200001/remoting/ios/app/clien...
File remoting/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/ios/app/clien...
remoting/ios/app/client_connection_view_controller.mm:34: static const CGFloat
kKeyboardAnimationTime = 0.3;
On 2017/05/15 23:06:11, nicholss wrote:
> On 2017/05/15 19:38:07, Yuwei wrote:
> > 0.3f? Although the compiler might be clever enough to skip the conversion...
> 
> ##.f should be the same as ##.0, but style wise I have seen more ##.f if it is
a
> whole number and 0.# if < 1.0
> 
> So .f is only needed for whole numbers

Technically speaking 0.3 is a double precision float while 0.3f is single.

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/input/...
File remoting/client/input/text_keyboard_input_strategy.cc (right):

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/input/...
remoting/client/input/text_keyboard_input_strategy.cc:37: std::queue<KeyEvent>
keys;
I'm not sure whether we still need KeyEvent... In this case it can be

input_stub_->SendKeyEvent(0, BACKSPACE, true);
input_stub_->SendKeyEvent(0, BACKSPACE, false);

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/native...
File remoting/client/native_device_keymap.cc (right):

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/native...
remoting/client/native_device_keymap.cc:11: NOTIMPLEMENTED();
Will this be used by WebApp? If not I think we can just remove this file?

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/native...
File remoting/client/native_device_keymap.h (right):

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/native...
remoting/client/native_device_keymap.h:13: struct KeycodeWithModifier {
Is this still being used?

https://codereview.chromium.org/2868383003/diff/220001/remoting/ios/client_ke...
File remoting/ios/client_keyboard.h (right):

https://codereview.chromium.org/2868383003/diff/220001/remoting/ios/client_ke...
remoting/ios/client_keyboard.h:25: @property(weak, nonatomic)
id<ClientKeyboardDelegate> delegate;
Have you considered just storing and using KeyboardInterpreter* here?

[nicholss, 2017-05-16 17:13:39]

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
File remoting/client/input/text_keyboard_input_strategy.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/input/...
remoting/client/input/text_keyboard_input_strategy.cc:40: KeyEvent key_down;
On 2017/05/16 00:47:30, Yuwei wrote:
> On 2017/05/15 23:06:11, nicholss wrote:
> > On 2017/05/15 19:38:06, Yuwei wrote:
> > > push({...})?
> > 
> > Acknowledged.
> 
> You probably want to use "Done" for things you have done :)

Acknowledged.

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/input/...
File remoting/client/input/text_keyboard_input_strategy.cc (right):

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/input/...
remoting/client/input/text_keyboard_input_strategy.cc:37: std::queue<KeyEvent>
keys;
On 2017/05/16 00:47:30, Yuwei wrote:
> I'm not sure whether we still need KeyEvent... In this case it can be
> 
> input_stub_->SendKeyEvent(0, BACKSPACE, true);
> input_stub_->SendKeyEvent(0, BACKSPACE, false);

perhaps, It might refactor out after I figure out how the meta mods will work.

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/native...
File remoting/client/native_device_keymap.cc (right):

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/native...
remoting/client/native_device_keymap.cc:11: NOTIMPLEMENTED();
On 2017/05/16 00:47:30, Yuwei wrote:
> Will this be used by WebApp? If not I think we can just remove this file?

The app will not link for windows builds that also build the client build. So I
don't think removing the stub is good.

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/native...
File remoting/client/native_device_keymap.h (right):

https://codereview.chromium.org/2868383003/diff/220001/remoting/client/native...
remoting/client/native_device_keymap.h:13: struct KeycodeWithModifier {
Nope! Gone.

https://codereview.chromium.org/2868383003/diff/220001/remoting/ios/client_ke...
File remoting/ios/client_keyboard.h (right):

https://codereview.chromium.org/2868383003/diff/220001/remoting/ios/client_ke...
remoting/ios/client_keyboard.h:25: @property(weak, nonatomic)
id<ClientKeyboardDelegate> delegate;
On 2017/05/16 00:47:30, Yuwei wrote:
> Have you considered just storing and using KeyboardInterpreter* here?

I have, there is a comment about that from last review round. I would like to
hold off on flattening the ClientKeyboardDelegate until mod keys and external
keyboards have finished being implemented just incase there is something funky,
and then if ClientKeyboardDelegate is still not doing much work I will axe it.

[Yuwei, 2017-05-16 18:49:22]

LGTM for my concerns :) You may still want to resolve Sergey's feedback though:

1. Fix the CL description.
2. Consider having some unittests.

[Sergey Ulanov, 2017-05-16 20:15:07]

sergeyu@chromium.org changed reviewers:
- sergeyu@google.com

[nicholss, 2017-05-16 21:29:27]

Description was changed from

==========
[CRD iOS] Send key events to the session.
==========

to

==========
[CRD iOS] Send key events to the session.

This CL enables the iOS client to send key events (keyboard interactions) to
the remote host. This is the first pass for milestone 1, with the goal of a
rough 
session experience and a follow up CL will come at a later date to polish this.
==========

[nicholss, 2017-05-16 21:30:43]

On 2017/05/16 18:49:22, Yuwei wrote:
> LGTM for my concerns :) You may still want to resolve Sergey's feedback
though:
> 
> 1. Fix the CL description.

Fixed a bit.

> 2. Consider having some unittests.

This will be a follow up CL. I am working on two new CLs to move input related
files into input and
then a second CL to wire up new unit tests.

[nicholss, 2017-05-16 21:45:06]

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
File remoting/client/native_device_keymap_ios.cc (right):

https://codereview.chromium.org/2868383003/diff/200001/remoting/client/native...
remoting/client/native_device_keymap_ios.cc:153: NOTIMPLEMENTED();
On 2017/05/15 19:38:07, Yuwei wrote:
> input_stub_->SendKeyEvent(0, key.keycode, key.keydown) will call into this?

Yes, I removed the NOTIMPLEMENTED() btw.

[nicholss, 2017-05-16 22:38:13]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-05-16 22:39:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 22:47:03]

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

[commit-bot: I haz the power, 2017-05-16 22:47:04]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nicholss, 2017-05-16 23:34:48]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 23:36:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 23:57:45]

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

[commit-bot: I haz the power, 2017-05-16 23:57:46]

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

[nicholss, 2017-05-17 16:34:32]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 16:35:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nicholss, 2017-05-17 16:36:18]

Patchset #15 (id:280001) has been deleted

[commit-bot: I haz the power, 2017-05-17 16:40:40]

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

[commit-bot: I haz the power, 2017-05-17 16:40:41]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[nicholss, 2017-05-17 16:43:08]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 16:44:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 16:49:37]

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

[commit-bot: I haz the power, 2017-05-17 16:49:38]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[nicholss, 2017-05-17 17:07:47]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 17:08:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nicholss, 2017-05-17 17:20:58]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 17:22:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 17:36:29]

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

[commit-bot: I haz the power, 2017-05-17 17:36:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nicholss, 2017-05-17 17:42:02]

Patchset #17 (id:340001) has been deleted

[nicholss, 2017-05-17 17:42:12]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 17:43:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 18:07:28]

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

[commit-bot: I haz the power, 2017-05-17 18:07:28]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nicholss, 2017-05-17 19:07:24]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 19:09:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 19:27:40]

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

[commit-bot: I haz the power, 2017-05-17 19:27:41]

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

[nicholss, 2017-05-17 21:03:42]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 21:05:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nicholss, 2017-05-17 21:25:54]

PTAL for client_input_injector additions

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
File remoting/client/input/client_input_injector.h (right):

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
remoting/client/input/client_input_injector.h:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
This file is new.

[Yuwei, 2017-05-17 21:32:39]

LGTM with nit

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
File remoting/client/input/client_input_injector.h (right):

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
remoting/client/input/client_input_injector.h:8: #include <stdint.h>
Remove stdint.h?

[Yuwei, 2017-05-17 21:41:20]

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
File remoting/client/input/keyboard_interpreter.cc (right):

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
remoting/client/input/keyboard_interpreter.cc:8: #include
"remoting/client/input/client_input_injector.h"
You probably don't need to include client_input_injector here since everything
is done with only the pointer.

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
remoting/client/input/keyboard_interpreter.cc:13: class ClientInputInjector;
Why is there an extra forward-declare?

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
File remoting/client/input/text_keyboard_input_strategy.h (right):

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
remoting/client/input/text_keyboard_input_strategy.h:9: #include
"remoting/client/chromoting_session.h"
You can remove this include now.

[commit-bot: I haz the power, 2017-05-17 21:42:26]

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

[commit-bot: I haz the power, 2017-05-17 21:42:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nicholss, 2017-05-17 21:59:18]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 22:00:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 04:42:36]

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

[commit-bot: I haz the power, 2017-05-18 04:42:37]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-05-22 18:52:45]

Thanks!

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
File remoting/client/input/keyboard_interpreter.cc (right):

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
remoting/client/input/keyboard_interpreter.cc:8: #include
"remoting/client/input/client_input_injector.h"
On 2017/05/17 21:41:20, Yuwei wrote:
> You probably don't need to include client_input_injector here since everything
> is done with only the pointer.

Done.

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
remoting/client/input/keyboard_interpreter.cc:13: class ClientInputInjector;
On 2017/05/17 21:41:20, Yuwei wrote:
> Why is there an extra forward-declare?

Done.

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
File remoting/client/input/text_keyboard_input_strategy.h (right):

https://codereview.chromium.org/2868383003/diff/400001/remoting/client/input/...
remoting/client/input/text_keyboard_input_strategy.h:9: #include
"remoting/client/chromoting_session.h"
On 2017/05/17 21:41:20, Yuwei wrote:
> You can remove this include now.

Done.

[nicholss, 2017-05-22 18:53:23]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-05-22 18:53:23]

The patchset sent to the CQ was uploaded after l-g-t-m from yuweih@chromium.org
Link to the patchset: https://codereview.chromium.org/2868383003/#ps440001
(title: "Update based on feedback.")

[commit-bot: I haz the power, 2017-05-22 18:54:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 20:30:54]

CQ is committing da patch.

Bot data: {"patchset_id": 440001, "attempt_start_ts": 1495479203266290,
"parent_rev": "894bec4e5ec23ba1c78f85303d25b6e90092058f", "commit_rev":
"1e49b618d05df533363bf177dc99bd1da871f76d"}

[commit-bot: I haz the power, 2017-05-22 20:31:05]

Description was changed from

==========
[CRD iOS] Send key events to the session.

This CL enables the iOS client to send key events (keyboard interactions) to
the remote host. This is the first pass for milestone 1, with the goal of a
rough 
session experience and a follow up CL will come at a later date to polish this.
==========

to

==========
[CRD iOS] Send key events to the session.

This CL enables the iOS client to send key events (keyboard interactions) to
the remote host. This is the first pass for milestone 1, with the goal of a
rough 
session experience and a follow up CL will come at a later date to polish this.

Review-Url: https://codereview.chromium.org/2868383003
Cr-Commit-Position: refs/heads/master@{#473677}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e49b618d05df533363bf177dc99...
==========

[commit-bot: I haz the power, 2017-05-22 20:31:06]

Committed patchset #21 (id:440001) as
https://chromium.googlesource.com/chromium/src/+/1e49b618d05df533363bf177dc99...