Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1408263006:
chromeos: Add ArcInputBridge to components/arc (Closed)
|
Created:
5 years, 1 month ago by denniskempin Modified:
5 years ago Reviewers:
elijahtaylor1, jochen (gone - plz use gerrit), sky, Luis Héctor Chávez, elijahtaylor (use chromium), reveman CC:
blundell+watchlist_chromium.org, droger+watchlist_chromium.org, sdefresne+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@arcxx Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL adds the ArcInputBridge which hooks into
aura::Windows that are running ARC applications to translate
and transmit their ui::Events to the ARC instance.
BUG=chromium:558499
TEST=Mouse, Touch and Keyboard events should be properly
working on an ARC application window.
Committed: https://crrev.com/3b12f96225546fdcae48735e15db9124ce2add95
Cr-Commit-Position: refs/heads/master@{#363941}
Patch Set 1 #Patch Set 2 : Arc: Extend ArcBridgeService with input bridge #
Total comments: 15
Patch Set 3 : Fixes after first end-to-end test #Patch Set 4 : More fixes after testing. cleanup. #Patch Set 5 : added owners #
Total comments: 41
Patch Set 6 : Moved code to exosphere #Patch Set 7 : Moved CreatEBridgeDevice to exo as well #Patch Set 8 : added Get() method #Patch Set 9 : fixed array initializeer #Patch Set 10 : nit fixes #Patch Set 11 : fixed scopedfd #
Total comments: 1
Patch Set 12 : Moved code back to components/arc #
Total comments: 26
Patch Set 13 : Moved IO to IO thread. Fixed style issues pointed out by jochen@ #
Total comments: 7
Patch Set 14 : integration with ArcServiceManager and ExoSurfaces #
Total comments: 81
Patch Set 15 : PImpl pattern. Non-blocking IO on UI thread. Nit fixes. #Patch Set 16 : updated build files #Patch Set 17 : fixed build.gn file #
Total comments: 22
Patch Set 18 : nit fixes. #
Total comments: 2
Patch Set 19 : limited deps #Patch Set 20 : nit fix #
Total comments: 9
Patch Set 21 : use window tracker to remove event handler in dtor #Patch Set 22 : nit fix #Patch Set 23 : rebase #Patch Set 24 : added missing dependency to gn file #Patch Set 25 : removed dependency on ui/events/ozone to fix generic chromium os builder #Patch Set 26 : rebase after mojo changes #Patch Set 27 : removed CL dependency #
Total comments: 1
Patch Set 28 : fixed clang warnings #Patch Set 29 : only attach observer if aura::Env exists #Patch Set 30 : nit fix #Messages
Total messages: 94 (43 generated)
Description was changed from ========== chromeos: ArcInputBridge for ArcBridgeService Adds a new class as part of the ArcBridgeService to relay input events to the arc instance. BUG= ========== to ========== This CL adds a new SendInputEvent method to ArcBridgeService, which can be called with a ui::Event that is to be sent to the ArcInstance. The ArcBridgeService will set up all necessary bridge devices on the instance-side by sending a file descriptor via the RegisterBridgeInputDevice message. ui::Event's are converted into linux evdev input_events's, which are then send over these file descriptors to the arc instance. BUG=b:24308453 TEST=test once the instance-side changes have landed. Make sure calling SendInputEvent will invoke the proper response on the instance. ==========
denniskempin@chromium.org changed reviewers: + denniskempin@chromium.org, elijahtaylor@chromium.org, lhchavez@chromium.org
Luis, Elijah, can you have a quick look for early feedback? I can't test this yet since the ArcBridgeService is not fully working yet.
https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_input_devices.cc (right): https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:16: void BridgeInputDevice::SendEvent(base::TimeDelta time, time is highlighted in the code review tool, maybe we could use a more specific name to avoid confusion with the c library? https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:17: __u16 type, nit: alignment? https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:21: // input events, so we can just fill in the same timestamp. on ARC, we had issues with wall time vs system time. When the machine would suspend and resume, the Android system time would be out of sync with input event times. No solid advice because I'm not looking where these are generated, but please test when possible this scenario https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:45: void KeyboardBridgeInputDevice::OnKeyEvent(ui::KeyEvent* event) { I feel like we should either include the keyboard and mouse device registration at startup or not include this code right now https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:40: LOG(ERROR) << "[ARC] ArcBridgeService"; is it appropriate to log to the error channel these messages? I would think INFO a better choice https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:77: // cause the corresponding devices on the instance-side to be destroyed as how is this guaranteed? do failed reads cause the devices to be deleted? https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:121: LOG(ERROR) << "Cannot create pipe: " << strerror(errno); https://code.google.com/p/chromium/codesearch#chromium/src/base/posix/safe_st... Please see if there is a better replacement for strerror. https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.h:184: nit: whitespace
https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_input_devices.cc (right): https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:16: void BridgeInputDevice::SendEvent(base::TimeDelta time, On 2015/11/02 22:30:29, elijahtaylor1 wrote: > time is highlighted in the code review tool, maybe we could use a more specific > name to avoid confusion with the c library? Done. https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:17: __u16 type, On 2015/11/02 22:30:29, elijahtaylor1 wrote: > nit: alignment? Done. https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:21: // input events, so we can just fill in the same timestamp. On 2015/11/02 22:30:29, elijahtaylor1 wrote: > on ARC, we had issues with wall time vs system time. When the machine would > suspend and resume, the Android system time would be out of sync with input > event times. No solid advice because I'm not looking where these are generated, > but please test when possible this scenario I did notice some problems where android will start dropping touch events because they are 'stale'. I have not figured out why exactly this is happening, especially since Mouse/Keyboard seem to continue working. That will take some more digging for a follow up CL I would suggest. I added a todo. https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:45: void KeyboardBridgeInputDevice::OnKeyEvent(ui::KeyEvent* event) { On 2015/11/02 22:30:29, elijahtaylor1 wrote: > I feel like we should either include the keyboard and mouse device registration > at startup or not include this code right now This comment seems to be on the wrong file but I understand your concern. The problem is right now the InstanceReady message is sent before any instance-side services are loaded, so we cannot register devices at that time. We do not yet have a message for when the services are ready. I'm working on a CL right now. https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:40: LOG(ERROR) << "[ARC] ArcBridgeService"; On 2015/11/02 22:30:29, elijahtaylor1 wrote: > is it appropriate to log to the error channel these messages? I would think INFO > a better choice This was just for debugging. I removed them again. https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:77: // cause the corresponding devices on the instance-side to be destroyed as On 2015/11/02 22:30:29, elijahtaylor1 wrote: > how is this guaranteed? do failed reads cause the devices to be deleted? Yes, the EventHub will close devices in two conditions: - INotify noticed that the /dev/input/ node has been removed. - The fd read returns 0 (EOF) or errno=ENODEV. When the write end of the pipe is being closed, the read end will return EOF. https://codereview.chromium.org/1408263006/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:121: LOG(ERROR) << "Cannot create pipe: " << strerror(errno); On 2015/11/02 22:30:29, elijahtaylor1 wrote: > https://code.google.com/p/chromium/codesearch#chromium/src/base/posix/safe_st... > > Please see if there is a better replacement for strerror. Done. PLOG does automatically append the error description :)
denniskempin@google.com changed reviewers: + elijahtaylor@google.com, jochen@chromium.org - denniskempin@chromium.org, elijahtaylor@chromium.org
PTAL! https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:159: if (bridge_devices_.empty()) { NOTE: this is temporary until we have a CL that allows us to hook into boot states of the instance.
elijahtaylor@chromium.org changed reviewers: + elijahtaylor@chromium.org
Focused mostly on the input code specifically, I did not look a lot at the bridge service changes, Luis is a better reviewer for that. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_input_devices.cc (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:13: static const int y_offset = 32; comment on this, specifically about it being temp or not and how we want to workaround the decoration more dynamically? also, I thought chromium constants were kYOffset style https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:86: SendEvent(time, EV_ABS, ABS_X, event->x()); is there no decoration on the x side that needs to be adjusted? why does y need adjusted, is the title bar being rendered in the window instead of actually being rendered by the system? https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:94: SendMouseButton(event, ui::EF_LEFT_MOUSE_BUTTON, BTN_LEFT, button_value); this is a weird structure. Some of the |event| checking is happening in this function and some in SendMouseButton. It's also weird to have this function call "SendMouseButton" but it's not clear that it will actually have an effect. I think it would make more sense to either just have a separate function called "UpdateButtons" or something and do all this logic there, looping over the buttons and checking flags individually. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:160: SendEvent(time, EV_ABS, ABS_MT_PRESSURE, 100); what is 100?
please don't reference buganizer bugs from the CL description. Instead, file a crbug.com bug and make that bug block the feature launch bug for arc++
please first iterate with a local reviewer on C++ coding style https://codereview.chromium.org/1408263006/diff/80001/components/arc.gypi File components/arc.gypi (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc.gypi#new... components/arc.gypi:17: '../ipc/ipc.gyp:ipc', you also need to depend on ui here. and please also update the BUILD.gn version https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_input_devices.h (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:1: #ifndef CHROMEOS_ARC_BRIDGE_INPUT_DEVICES_H_ COMPONENTS_ARC_ARC_BRIDGE_INPUT_DEVICES_H_ also, missing copyright header https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:16: BridgeInputDevice(base::ScopedFD fd); explicit please add a virtual dtor https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:18: // Send input_event through file descriptor. please document what type code and value are https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:26: }; disallow copy/assign https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:31: static const int kKeyReleased = 0; please move purely private constants to the cc file https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:36: KeyboardBridgeInputDevice(base::ScopedFD fd); explicit / dtor / disallow copy & assign - please review the chromium C++ style guide
https://codereview.chromium.org/1408263006/diff/80001/components/arc.gypi File components/arc.gypi (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc.gypi#new... components/arc.gypi:22: 'arc/arc_bridge_input_devices.cc', Can these be sorted alphabetically? https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_input_devices.cc (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:1: #include "components/arc/arc_bridge_input_devices.h" Also missing copyright block. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:8: #include <linux/input.h> Convention seems to be to put the system includes before the Chrome ones. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:38: int num_written = write(fd_.get(), reinterpret_cast<void*>(&event), Can you add a thread check to make sure you are not doing I/O on non-IO threads? Also add comments on all functions in the .h about what thread they can be used on. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:57: __u16 evdev_code = ui::NativeCodeToEvdevCode(native_code); Huh, this does not need a cast? https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_input_devices.h (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:19: void SendEvent(base::TimeDelta time, __u16 type, __u16 code, __s32 value); Can you use types from stdint instead? I know we're linux-only, but no other part of the codebase seems to be using these types. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:138: PLOG(ERROR) << "Cannot create pipe"; At this point the operation cannot continue. return base::ScopedFD()? Also, HANDLE_EINTR(pipe...)? https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:142: ipc_channel_->Send(new ArcInstanceMsg_RegisterInputDevice( What if this fails (returns false)? Shouldn't you also return an invalid fd? https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.h:170: base::ScopedFD CreateBridgeInputDevice(const std::string& name, I would prefer if the ArcBridgeService did not have to know about this. We can discuss the details, but an option is to have the some code in exosphere that can both listen to events and changes in state of the arc bridge and keep the RegisterInputDevice interface here?
PTAL again. Outstanding issue is threading. Currently I am doing io on the main thread which I've learned is not ok. I'll look into that. https://codereview.chromium.org/1408263006/diff/80001/components/arc.gypi File components/arc.gypi (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc.gypi#new... components/arc.gypi:22: 'arc/arc_bridge_input_devices.cc', On 2015/11/17 17:51:53, Luis Héctor Chávez wrote: > Can these be sorted alphabetically? Done. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_input_devices.cc (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:1: #include "components/arc/arc_bridge_input_devices.h" On 2015/11/17 17:51:53, Luis Héctor Chávez wrote: > Also missing copyright block. Done. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:8: #include <linux/input.h> On 2015/11/17 17:51:53, Luis Héctor Chávez wrote: > Convention seems to be to put the system includes before the Chrome ones. Done. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:13: static const int y_offset = 32; On 2015/11/16 21:08:26, elijahtaylor1 wrote: > comment on this, specifically about it being temp or not and how we want to > workaround the decoration more dynamically? also, I thought chromium constants > were kYOffset style oops. this line should be part of the HACK CL. I don't want this in here but we needed it for the demo. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:38: int num_written = write(fd_.get(), reinterpret_cast<void*>(&event), On 2015/11/17 17:51:53, Luis Héctor Chávez wrote: > Can you add a thread check to make sure you are not doing I/O on non-IO threads? > Also add comments on all functions in the .h about what thread they can be used > on. I'll have to look into that. I did not know we can't do IO from the main thread. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:57: __u16 evdev_code = ui::NativeCodeToEvdevCode(native_code); On 2015/11/17 17:51:53, Luis Héctor Chávez wrote: > Huh, this does not need a cast? I expected the method to return a u16 which is the type of evdev codes. This should cause at least a warning but nothing shows up. that's interesting. Either way, I updated the code to use standard integer types instead. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:86: SendEvent(time, EV_ABS, ABS_X, event->x()); On 2015/11/16 21:08:26, elijahtaylor1 wrote: > is there no decoration on the x side that needs to be adjusted? why does y need > adjusted, is the title bar being rendered in the window instead of actually > being rendered by the system? See above. We did not see any offset in the X direction. We had to attach the event handler to the window itself, so we get events relative to the window frame. If we put the event handler into the client area we do not get all events. I am not quite sure why that is, I will have to talk to someone who knows more about input routing in chrome. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:94: SendMouseButton(event, ui::EF_LEFT_MOUSE_BUTTON, BTN_LEFT, button_value); On 2015/11/16 21:08:26, elijahtaylor1 wrote: > this is a weird structure. Some of the |event| checking is happening in this > function and some in SendMouseButton. It's also weird to have this function > call "SendMouseButton" but it's not clear that it will actually have an effect. > > I think it would make more sense to either just have a separate function called > "UpdateButtons" or something and do all this logic there, looping over the > buttons and checking flags individually. I agree. It made sense in the beginning, not anymore. updated to loop over a static array. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.cc:160: SendEvent(time, EV_ABS, ABS_MT_PRESSURE, 100); On 2015/11/16 21:08:26, elijahtaylor1 wrote: > what is 100? a hack. Updated to use force(). https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_input_devices.h (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:1: #ifndef CHROMEOS_ARC_BRIDGE_INPUT_DEVICES_H_ On 2015/11/17 15:03:44, jochen wrote: > COMPONENTS_ARC_ARC_BRIDGE_INPUT_DEVICES_H_ > > also, missing copyright header Done. respective macro on new file in exo https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:16: BridgeInputDevice(base::ScopedFD fd); On 2015/11/17 15:03:44, jochen wrote: > explicit > > please add a virtual dtor Done. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:18: // Send input_event through file descriptor. On 2015/11/17 15:03:44, jochen wrote: > please document what type code and value are Done. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:19: void SendEvent(base::TimeDelta time, __u16 type, __u16 code, __s32 value); On 2015/11/17 17:51:53, Luis Héctor Chávez wrote: > Can you use types from stdint instead? I know we're linux-only, but no other > part of the codebase seems to be using these types. Done. It's the types used by the struct definition, but at some point we are implicitly type casting anyway, so that's ok. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:26: }; On 2015/11/17 15:03:44, jochen wrote: > disallow copy/assign Done. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:31: static const int kKeyReleased = 0; On 2015/11/17 15:03:44, jochen wrote: > please move purely private constants to the cc file Done. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_input_devices.h:36: KeyboardBridgeInputDevice(base::ScopedFD fd); On 2015/11/17 15:03:44, jochen wrote: > explicit / dtor / disallow copy & assign - please review the chromium C++ style > guide Done. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:138: PLOG(ERROR) << "Cannot create pipe"; On 2015/11/17 17:51:53, Luis Héctor Chávez wrote: > At this point the operation cannot continue. return base::ScopedFD()? > Also, HANDLE_EINTR(pipe...)? Done. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:142: ipc_channel_->Send(new ArcInstanceMsg_RegisterInputDevice( On 2015/11/17 17:51:53, Luis Héctor Chávez wrote: > What if this fails (returns false)? Shouldn't you also return an invalid fd? Done. I'm checking for errors and return -1 as a fd. Which is handled as "no device" by later code. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:159: if (bridge_devices_.empty()) { On 2015/11/16 18:55:49, denniskempin wrote: > NOTE: this is temporary until we have a CL that allows us to hook into boot > states of the instance. removed. https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1408263006/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.h:170: base::ScopedFD CreateBridgeInputDevice(const std::string& name, On 2015/11/17 17:51:54, Luis Héctor Chávez wrote: > I would prefer if the ArcBridgeService did not have to know about this. > > We can discuss the details, but an option is to have the some code in exosphere > that can both listen to events and changes in state of the arc bridge and keep > the RegisterInputDevice interface here? done
https://codereview.chromium.org/1408263006/diff/200001/components/exo/arc/arc... File components/exo/arc/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/200001/components/exo/arc/arc... components/exo/arc/arc_input_bridge.cc:76: if (phase != InstanceBootPhase::SYSTEM_SERVICES_READY) { According to lloyd this won't work at SYSTEM_SERVICES_READY, but we'll have to wait until BOOT_COMPLETED. I'll have to investigate this.
can you please review the c++ style guide and update your code accordingly? also, format the CL description as outlined here: http://dev.chromium.org/developers/contributing-code#TOC-Change-List-Descript... esp. remove the buganizer entry, and use a chromium bug to track this work. https://codereview.chromium.org/1408263006/diff/220001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1408263006/diff/220001/components/arc/DEPS#ne... components/arc/DEPS:6: "+ui" this needs to be added as deps to the build.gn/gypi files https://codereview.chromium.org/1408263006/diff/220001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1408263006/diff/220001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:108: name, device_type, base::FileDescriptor(fd.release(), true))); how is that related to this CL? https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... File components/arc/input/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:18: // input_event values for keyboard events empty line before this one https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:19: static const int kKeyReleased = 0; no static required should those be an enum? https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:37: MouseButtonMapping kMouseButtonMap[] = { empty line before this one https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:45: } empty line before this one, also } // namespace https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:55: DCHECK(arc_bridge_service->state() == ArcBridgeService::State::STOPPED); DCHECK_EQ https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:58: g_arc_input_bridge = this; using observers and weak ptrs implies some weak coupling, but there can only be one input bridge. this is pretty confusing. Either the bridge service should own/create the input bridge, or it should be loosely coupled without some magic global Get() https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:62: if (arc_bridge_service_) { no { } required for single line body https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:215: LOG(ERROR) << "Invalid file descriptor"; please use LOG or VLOG. Can this happen it all? Should that be a DCHECK()? https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... File components/arc/input/arc_input_bridge.h (right): https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.h:19: class ArcInputBridge : private ArcBridgeService::Observer { no private inheritance: https://google.github.io/styleguide/cppguide.html#Inheritance https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.h:24: ArcInputBridge(base::WeakPtr<ArcBridgeService> arc_bridge_service); explicit https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.h:25: virtual ~ArcInputBridge(); should be override
On 2015/12/02 13:09:02, jochen wrote: > can you please review the c++ style guide and update your code accordingly? > > also, format the CL description as outlined here: > http://dev.chromium.org/developers/contributing-code#TOC-Change-List-Descript... > > esp. remove the buganizer entry, and use a chromium bug to track this work. > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/DEPS > File components/arc/DEPS (right): > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/DEPS#ne... > components/arc/DEPS:6: "+ui" > this needs to be added as deps to the build.gn/gypi files > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/arc_bri... > File components/arc/arc_bridge_service_impl.cc (right): > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/arc_bri... > components/arc/arc_bridge_service_impl.cc:108: name, device_type, > base::FileDescriptor(fd.release(), true))); > how is that related to this CL? > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > File components/arc/input/arc_input_bridge.cc (right): > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.cc:18: // input_event values for keyboard > events > empty line before this one > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.cc:19: static const int kKeyReleased = 0; > no static required > > should those be an enum? > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.cc:37: MouseButtonMapping > kMouseButtonMap[] = { > empty line before this one > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.cc:45: } > empty line before this one, also } // namespace > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.cc:55: DCHECK(arc_bridge_service->state() > == ArcBridgeService::State::STOPPED); > DCHECK_EQ > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.cc:58: g_arc_input_bridge = this; > using observers and weak ptrs implies some weak coupling, but there can only be > one input bridge. this is pretty confusing. > > Either the bridge service should own/create the input bridge, or it should be > loosely coupled without some magic global Get() > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.cc:62: if (arc_bridge_service_) { > no { } required for single line body > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.cc:215: LOG(ERROR) << "Invalid file > descriptor"; > please use LOG or VLOG. > > Can this happen it all? Should that be a DCHECK()? > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > File components/arc/input/arc_input_bridge.h (right): > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.h:19: class ArcInputBridge : private > ArcBridgeService::Observer { > no private inheritance: > https://google.github.io/styleguide/cppguide.html#Inheritance > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.h:24: > ArcInputBridge(base::WeakPtr<ArcBridgeService> arc_bridge_service); > explicit > > https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... > components/arc/input/arc_input_bridge.h:25: virtual ~ArcInputBridge(); > should be override Thanks a lot Jochen! I apologize for the messy history of this CL. I was trying to find you on teams/ since I wanted to talk to you about some details of this CL but could not find your LDAP. I will review the code guidelines in regard with this CL again. I do mostly know the guidelines but it's hard for me to spot the mishaps since I am getting mixing them up with the Android or Linux guidelines. I had updated the CL description but noticed that it's not updated from the commit message as I thought it would. I'll do this again.
https://codereview.chromium.org/1408263006/diff/220001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1408263006/diff/220001/components/arc/DEPS#ne... components/arc/DEPS:6: "+ui" On 2015/12/02 13:09:01, jochen wrote: > this needs to be added as deps to the build.gn/gypi files Done. https://codereview.chromium.org/1408263006/diff/220001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1408263006/diff/220001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:108: name, device_type, base::FileDescriptor(fd.release(), true))); On 2015/12/02 13:09:01, jochen wrote: > how is that related to this CL? It's a bug. This part of the code could not be tested until this CL was made. I can turn it into a separate patch set https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... File components/arc/input/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:18: // input_event values for keyboard events On 2015/12/02 13:09:02, jochen wrote: > empty line before this one Done. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:19: static const int kKeyReleased = 0; On 2015/12/02 13:09:02, jochen wrote: > no static required > > should those be an enum? removed static. I would argue against an enum since they are the integer values that we have to put into the struct input_event's. That's the only way in which they are being used, so it seems to make sense to use constants instead of an enum. I don't feel too strongly about this, I'm happy to change it. Let me know. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:37: MouseButtonMapping kMouseButtonMap[] = { On 2015/12/02 13:09:01, jochen wrote: > empty line before this one Done. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:45: } On 2015/12/02 13:09:01, jochen wrote: > empty line before this one, also } // namespace Done. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:55: DCHECK(arc_bridge_service->state() == ArcBridgeService::State::STOPPED); On 2015/12/02 13:09:01, jochen wrote: > DCHECK_EQ Unfortunately DCHECK_EQ can't compare enums. Well it can compare them, but the logging output in case of a failure throws a compiler error. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:58: g_arc_input_bridge = this; On 2015/12/02 13:09:02, jochen wrote: > using observers and weak ptrs implies some weak coupling, but there can only be > one input bridge. this is pretty confusing. > > Either the bridge service should own/create the input bridge, or it should be > loosely coupled without some magic global Get() I agree. I would like the bridge service to own the input bridge, however that would create a two-way dependency. I am talking to Luis and Oshima about this since it is not clear yet what a good approach here would be. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:62: if (arc_bridge_service_) { On 2015/12/02 13:09:01, jochen wrote: > no { } required for single line body Done. In other parts of the code as well. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:215: LOG(ERROR) << "Invalid file descriptor"; On 2015/12/02 13:09:01, jochen wrote: > please use LOG or VLOG. > > Can this happen it all? Should that be a DCHECK()? Done. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... File components/arc/input/arc_input_bridge.h (right): https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.h:19: class ArcInputBridge : private ArcBridgeService::Observer { On 2015/12/02 13:09:02, jochen wrote: > no private inheritance: > https://google.github.io/styleguide/cppguide.html#Inheritance Done. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.h:24: ArcInputBridge(base::WeakPtr<ArcBridgeService> arc_bridge_service); On 2015/12/02 13:09:02, jochen wrote: > explicit Done. https://codereview.chromium.org/1408263006/diff/220001/components/arc/input/a... components/arc/input/arc_input_bridge.h:25: virtual ~ArcInputBridge(); On 2015/12/02 13:09:02, jochen wrote: > should be override Done.
https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... File components/arc/input/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:96: FROM_HERE, base::Bind(&ArcInputBridge::SendInputEventIO, just wondering if you know the average thread switch and dispatch time for these events and what it does for input latency. Not super critical as we really just need something functional, but unfortunate that each event incurs this cost. This is also a frequent event,are there any implications of dispatching them individually over batch processing them? https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:242: time_stamp = ui::EventTimeForNow(); I wonder if this might cause problems because you're generating lots of events per ui::Event with different timestamps. If this is temporary this is probably ok as it seems to work. https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:264: SendKernelEventIO(fd, time, EV_SYN, SYN_REPORT, 0); you should probably use ui::EventTimeForNow here too while you're doing it above
https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... File components/arc/input/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:96: FROM_HERE, base::Bind(&ArcInputBridge::SendInputEventIO, On 2015/12/03 06:35:19, elijahtaylor1 wrote: > just wondering if you know the average thread switch and dispatch time for these > events and what it does for input latency. Not super critical as we really just > need something functional, but unfortunate that each event incurs this cost. > This is also a frequent event,are there any implications of dispatching them > individually over batch processing them? I don't feel too comfortable about this either. Maybe we could get an Exception for the input bridge doing IO on the UI thread. But I do not know what the latency implications are, what I am mostly worried about is that we might get stutter when there is something big working on the IO thread that's blocking input from being delivered. It is at least not noticeable when using the device so far. We could set up some latency comparisons later, but considering this version of the input bridge won't last long it's probably not worth the effort. https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:242: time_stamp = ui::EventTimeForNow(); On 2015/12/03 06:35:19, elijahtaylor1 wrote: > I wonder if this might cause problems because you're generating lots of events > per ui::Event with different timestamps. If this is temporary this is probably > ok as it seems to work. As far as I can tell from the low level stuff we did on CrOS it does not really matter. Usually it's the timestamp of the SYN event that counts for ui purposes. https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:264: SendKernelEventIO(fd, time, EV_SYN, SYN_REPORT, 0); On 2015/12/03 06:35:19, elijahtaylor1 wrote: > you should probably use ui::EventTimeForNow here too while you're doing it above it is calling the method above, so it's using the now timestamp.
lgtm but I think jochen should lgtm too since he's invested a lot in this CL and is better at giving chrome style and practices feedback https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... File components/arc/input/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/240001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:264: SendKernelEventIO(fd, time, EV_SYN, SYN_REPORT, 0); On 2015/12/03 15:43:40, denniskempin wrote: > On 2015/12/03 06:35:19, elijahtaylor1 wrote: > > you should probably use ui::EventTimeForNow here too while you're doing it > above > > it is calling the method above, so it's using the now timestamp. doh, code comprehension failure!
reveman@chromium.org changed reviewers: + reveman@chromium.org
https://codereview.chromium.org/1408263006/diff/260001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/arc_bri... components/arc/arc_bridge_service.h:26: class ArcBridgeService : public base::SupportsWeakPtr<ArcBridgeService> { why is this needed? I don't see it being used https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... File components/arc/input/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:24: // input_event values for keyboard events nit: missing punctuation https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:29: // maximum number of supported multi-touch slots (simultaneous fingers) nit: missing punctuation https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:32: // tracking id of an empty slot nit: missing punctuation https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:36: // todo(denniskempin): communicate maximum during initialization. nit: s/todo/TODO https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:48: MouseButtonMapping kMouseButtonMap[] = { nit: we could save a few lines by: struct MouseButtonMapping { int ui_flag; int evdev_code; } kMouseButtonMap[] = { .. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:67: io_task_runner_(io_task_runner), what happens if origin_task_runner_ == io_task_runner_ ? can we DCHECK to make sure that's not the case? https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:74: weak_factory_.GetWeakPtr())); why do we need to post a task for this if we're already on the correct thread? https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:78: if (arc_bridge_service_) I'm failing to see how arc_bridge_service_ can ever be null. how would that happen and why can't we guarantee that arc_bridge_service_ is alive during the lifetime of this class? https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:83: aura::Env::GetInstance()->AddObserver(this); where is the matching RemoveObserver call? https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:94: CreateBridgeInputDevice("ChromeOS Touchscreen", "touchscreen"); these "*_fd_" members seem to also be used on the IO thread. how is that safe without a mutex? https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:99: scoped_ptr<ui::Event> copy = ui::Event::Clone(*event); Would it make sense to do more processing on the UI thread and minimize the amount of work on the IO thread? I'm thinking that would also mean that you wouldn't have to copy the event here. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:102: weak_factory_.GetWeakPtr(), base::Passed(©))); This doesn't work as weak pointers are not thread safe. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:107: new_window->AddPreTargetHandler(this); nit: call RemovePreTargetHandler before the window is destroyed. I think you should observe the window for that https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:263: // todo(denniskempin): To enable performance tracing of events we will want nit: s/todo/TODO/ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:279: if (num_written != sizeof(struct input_event)) can this be a DCHECK? https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:285: DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()); origin_task_runner? https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... File components/arc/input/arc_input_bridge.h (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:6: #define COMPONENTS_EXO_ARC_ARC_INPUT_BRIDGE_H_ should be COMPONENTS_ARC_INPUT_ARC_INPUT_BRIDGE_H_ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:20: } // namespace aura fyi, typically just namespace aura { class Window; } for simple forward declarations like this but current code is of course fine too https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:52: void OnEvent(ui::Event* event) override; nit: I don't recommend inheriting a public function as protected. also when overriding functions we typically but a comments such as: // Overridden from ui::EventHandler: and other comments are not supposed to be needed as it should be properly documented as part of the interface. I'd move the comment above to the definition in the .cc file. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:55: void OnWindowInitialized(aura::Window* new_window) override; ditto https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:61: void SendInputEventIO(scoped_ptr<ui::Event> event); nit: s/IO/OnIO/ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:64: // descriptor nit: missing punctuation. also, does this comment fit on one line? https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:65: void SendKeyEventIO(ui::KeyEvent* event); nit: s/IO/OnIO/ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:66: void SendTouchEventIO(ui::TouchEvent* event); nit: s/IO/OnIO/ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:67: void SendMouseEventIO(ui::MouseEvent* event); nit: s/IO/OnIO/ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:79: void SendKernelEventIO(const base::ScopedFD& fd, nit: s/IO/OnIO/ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:86: void SendSynReportIO(const base::ScopedFD& fd, base::TimeDelta timestamp); nit: s/IO/OnIO/ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:89: int AcquireTouchSlotIO(ui::TouchEvent* event); nit: s/IO/OnIO/ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:92: int FindTouchSlotIO(int tracking_id); nit: s/IO/OnIO/ https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:95: void OnInstanceBootPhase(InstanceBootPhase phase) override; nit: avoid inheriting a public function as private https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:112: ArcBridgeService* arc_bridge_service_; it's not clear what members below are used on what thread. can this be documented? I'm assuming they are never accessed from more than one thread as there's no mutex here to protect them.. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:140: #endif // COMPONENTS_EXO_ARC_ARC_INPUT_BRIDGE_H_ COMPONENTS_ARC_INPUT_ARC_INPUT_BRIDGE_H_ https://codereview.chromium.org/1408263006/diff/260001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/1408263006/diff/260001/components/exo/surface... components/exo/surface.cc:91: SetName("ExoSurface"); fyi, I already made this change in ToT
lgtm once reveman is happy please make sure that errors are not swallowed into invisible LOG() statements, but propagated to the user (or some component that can actually do something about it). The CL description still references the buganizer entry. please remove that reference, and instead reference a crbug.com entry. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... File components/arc/input/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:119: } else { UNREACHABLE(); }? https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:126: LOG(WARNING) << "No keyboard bridge device available."; please use VLOG/DVLOG for logging. note that logging is now visible to users, so if this can actually happen, we need to either gracefully handle the failure or notify the user somehow https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:204: LOG(WARNING) << "No mouse bridge device available."; same here. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... File components/arc/input/arc_input_bridge.h (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:120: float offset_x_acc; should end in _
https://codereview.chromium.org/1408263006/diff/260001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/DEPS#ne... components/arc/DEPS:6: "+ui" Wasn't the point of the ArcServiceManager to avoid this? (at least in this directory.) https://codereview.chromium.org/1408263006/diff/260001/components/arc/arc_ser... File components/arc/arc_service_manager.h (left): https://codereview.chromium.org/1408263006/diff/260001/components/arc/arc_ser... components/arc/arc_service_manager.h:41: scoped_ptr<ArcBridgeService> arc_bridge_service_; If you want to destroy it last, shouldn't it be declared first?
Description was changed from ========== This CL adds a new SendInputEvent method to ArcBridgeService, which can be called with a ui::Event that is to be sent to the ArcInstance. The ArcBridgeService will set up all necessary bridge devices on the instance-side by sending a file descriptor via the RegisterBridgeInputDevice message. ui::Event's are converted into linux evdev input_events's, which are then send over these file descriptors to the arc instance. BUG=b:24308453 TEST=test once the instance-side changes have landed. Make sure calling SendInputEvent will invoke the proper response on the instance. ========== to ========== This CL adds the ArcBridgeService which hooks into aura::Windows that are running ARC applications to translate and transmit their ui::Events to the ARC instance. BUG=chromium:558499 TEST=Mouse, Touch and Keyboard events should be properly working on an ARC application window. ==========
I fixed most nits. Most importantly I also moved IO back to the UI thread. The FD is setup for non-blocking IO. Please have another look at the build files. I can't really tell whether I added the deps correctly since it's building fine without. https://codereview.chromium.org/1408263006/diff/260001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/DEPS#ne... components/arc/DEPS:6: "+ui" On 2015/12/04 17:59:59, Luis Héctor Chávez wrote: > Wasn't the point of the ArcServiceManager to avoid this? (at least in this > directory.) Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/arc_bri... components/arc/arc_bridge_service.h:26: class ArcBridgeService : public base::SupportsWeakPtr<ArcBridgeService> { On 2015/12/04 03:21:23, reveman wrote: > why is this needed? I don't see it being used You're right, no longer needed. https://codereview.chromium.org/1408263006/diff/260001/components/arc/arc_ser... File components/arc/arc_service_manager.h (left): https://codereview.chromium.org/1408263006/diff/260001/components/arc/arc_ser... components/arc/arc_service_manager.h:41: scoped_ptr<ArcBridgeService> arc_bridge_service_; On 2015/12/04 17:59:59, Luis Héctor Chávez wrote: > If you want to destroy it last, shouldn't it be declared first? yes.. of course! https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... File components/arc/input/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:24: // input_event values for keyboard events On 2015/12/04 03:21:24, reveman wrote: > nit: missing punctuation Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:29: // maximum number of supported multi-touch slots (simultaneous fingers) On 2015/12/04 03:21:24, reveman wrote: > nit: missing punctuation Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:32: // tracking id of an empty slot On 2015/12/04 03:21:23, reveman wrote: > nit: missing punctuation Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:36: // todo(denniskempin): communicate maximum during initialization. On 2015/12/04 03:21:24, reveman wrote: > nit: s/todo/TODO Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:48: MouseButtonMapping kMouseButtonMap[] = { On 2015/12/04 03:21:23, reveman wrote: > nit: we could save a few lines by: > > struct MouseButtonMapping { > int ui_flag; > int evdev_code; > } kMouseButtonMap[] = { .. Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:67: io_task_runner_(io_task_runner), On 2015/12/04 03:21:23, reveman wrote: > what happens if origin_task_runner_ == io_task_runner_ ? can we DCHECK to make > sure that's not the case? no longer necessary. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:74: weak_factory_.GetWeakPtr())); On 2015/12/04 03:21:23, reveman wrote: > why do we need to post a task for this if we're already on the correct thread? aura::Env is created after this class is. Unfortunately there is no hook in the browser parts that allows us to initialize stuff after aura::Env (it's initialized pre message loop run). So we have to post a task that's executed with the message loop. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:78: if (arc_bridge_service_) On 2015/12/04 03:21:23, reveman wrote: > I'm failing to see how arc_bridge_service_ can ever be null. how would that > happen and why can't we guarantee that arc_bridge_service_ is alive during the > lifetime of this class? Yes, now that's guaranteed. done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:83: aura::Env::GetInstance()->AddObserver(this); On 2015/12/04 03:21:24, reveman wrote: > where is the matching RemoveObserver call? I don't think we can. aura::Env is destroyed before this class is. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:94: CreateBridgeInputDevice("ChromeOS Touchscreen", "touchscreen"); On 2015/12/04 03:21:23, reveman wrote: > these "*_fd_" members seem to also be used on the IO thread. how is that safe > without a mutex? Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:99: scoped_ptr<ui::Event> copy = ui::Event::Clone(*event); On 2015/12/04 03:21:23, reveman wrote: > Would it make sense to do more processing on the UI thread and minimize the > amount of work on the IO thread? I'm thinking that would also mean that you > wouldn't have to copy the event here. Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:102: weak_factory_.GetWeakPtr(), base::Passed(©))); On 2015/12/04 03:21:24, reveman wrote: > This doesn't work as weak pointers are not thread safe. Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:107: new_window->AddPreTargetHandler(this); On 2015/12/04 03:21:24, reveman wrote: > nit: call RemovePreTargetHandler before the window is destroyed. I think you > should observe the window for that As we talked over in hangouts. It's common practice not to remove observers before the window is destroyed and the window does not require the observer list to be empty. So we won't have to add additional observer for window destruction. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:119: } On 2015/12/04 14:18:12, jochen wrote: > else { > UNREACHABLE(); > }? It is actually reachable if we are receiving an ui::Event that we can't handle, and that's ok (e.g. gesture events). https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:126: LOG(WARNING) << "No keyboard bridge device available."; On 2015/12/04 14:18:11, jochen wrote: > please use VLOG/DVLOG for logging. > > note that logging is now visible to users, so if this can actually happen, we > need to either gracefully handle the failure or notify the user somehow Done. I searched the code for other occurrences and updated them. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:204: LOG(WARNING) << "No mouse bridge device available."; On 2015/12/04 14:18:12, jochen wrote: > same here. Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:263: // todo(denniskempin): To enable performance tracing of events we will want On 2015/12/04 03:21:23, reveman wrote: > nit: s/todo/TODO/ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:279: if (num_written != sizeof(struct input_event)) On 2015/12/04 03:21:23, reveman wrote: > can this be a DCHECK? Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:285: DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()); On 2015/12/04 03:21:24, reveman wrote: > origin_task_runner? Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... File components/arc/input/arc_input_bridge.h (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:6: #define COMPONENTS_EXO_ARC_ARC_INPUT_BRIDGE_H_ On 2015/12/04 03:21:24, reveman wrote: > should be COMPONENTS_ARC_INPUT_ARC_INPUT_BRIDGE_H_ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:20: } // namespace aura On 2015/12/04 03:21:24, reveman wrote: > fyi, typically just > > namespace aura { > class Window; > } > > for simple forward declarations like this but current code is of course fine too Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:52: void OnEvent(ui::Event* event) override; On 2015/12/04 03:21:24, reveman wrote: > nit: I don't recommend inheriting a public function as protected. > > also when overriding functions we typically but a comments such as: > > // Overridden from ui::EventHandler: > > and other comments are not supposed to be needed as it should be properly > documented as part of the interface. I'd move the comment above to the > definition in the .cc file. Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:55: void OnWindowInitialized(aura::Window* new_window) override; On 2015/12/04 03:21:24, reveman wrote: > ditto Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:61: void SendInputEventIO(scoped_ptr<ui::Event> event); On 2015/12/04 03:21:24, reveman wrote: > nit: s/IO/OnIO/ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:64: // descriptor On 2015/12/04 03:21:24, reveman wrote: > nit: missing punctuation. also, does this comment fit on one line? close. 82. Added dot. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:65: void SendKeyEventIO(ui::KeyEvent* event); On 2015/12/04 03:21:24, reveman wrote: > nit: s/IO/OnIO/ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:66: void SendTouchEventIO(ui::TouchEvent* event); On 2015/12/04 03:21:24, reveman wrote: > nit: s/IO/OnIO/ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:67: void SendMouseEventIO(ui::MouseEvent* event); On 2015/12/04 03:21:24, reveman wrote: > nit: s/IO/OnIO/ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:79: void SendKernelEventIO(const base::ScopedFD& fd, On 2015/12/04 03:21:24, reveman wrote: > nit: s/IO/OnIO/ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:86: void SendSynReportIO(const base::ScopedFD& fd, base::TimeDelta timestamp); On 2015/12/04 03:21:24, reveman wrote: > nit: s/IO/OnIO/ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:89: int AcquireTouchSlotIO(ui::TouchEvent* event); On 2015/12/04 03:21:24, reveman wrote: > nit: s/IO/OnIO/ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:92: int FindTouchSlotIO(int tracking_id); On 2015/12/04 03:21:24, reveman wrote: > nit: s/IO/OnIO/ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:95: void OnInstanceBootPhase(InstanceBootPhase phase) override; On 2015/12/04 03:21:24, reveman wrote: > nit: avoid inheriting a public function as private Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:112: ArcBridgeService* arc_bridge_service_; On 2015/12/04 03:21:24, reveman wrote: > it's not clear what members below are used on what thread. can this be > documented? I'm assuming they are never accessed from more than one thread as > there's no mutex here to protect them.. Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:120: float offset_x_acc; On 2015/12/04 14:18:12, jochen wrote: > should end in _ Done. https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.h:140: #endif // COMPONENTS_EXO_ARC_ARC_INPUT_BRIDGE_H_ On 2015/12/04 03:21:24, reveman wrote: > COMPONENTS_ARC_INPUT_ARC_INPUT_BRIDGE_H_ Done. https://codereview.chromium.org/1408263006/diff/260001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/1408263006/diff/260001/components/exo/surface... components/exo/surface.cc:91: SetName("ExoSurface"); On 2015/12/04 03:21:24, reveman wrote: > fyi, I already made this change in ToT yep, it'll go away with the next rebase
mostly nits, so lgtm and defer the final decision to reveman. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... File components/arc/input/arc_input_bridge_impl.cc (right): https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:248: DCHECK_GE(fd.get(), 0); DCHECK(fd.is_valid())? https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:325: close(fd[1]); // close write end. Maybe it's better to create the base::ScopedFD before this block so that it will be closed if anything goes wrong in the next few ifs. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:343: return base::ScopedFD(write_fd).Pass(); We've moved to using std::move instead of Pass. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... File components/arc/input/arc_input_bridge_impl.h (right): https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.h:96: ArcBridgeService* arc_bridge_service_; Just add a comment about lifetime and why it's safe to use this as-is.
Oh also, Edit issue and reformat the commit message, it should fit on 72 columns.
Description was changed from ========== This CL adds the ArcBridgeService which hooks into aura::Windows that are running ARC applications to translate and transmit their ui::Events to the ARC instance. BUG=chromium:558499 TEST=Mouse, Touch and Keyboard events should be properly working on an ARC application window. ========== to ========== This CL adds the ArcBridgeService which hooks into aura::Windows that are running ARC applications to translate and transmit their ui::Events to the ARC instance. BUG=chromium:558499 TEST=Mouse, Touch and Keyboard events should be properly working on an ARC application window. ==========
https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... File components/arc/input/arc_input_bridge.cc (right): https://codereview.chromium.org/1408263006/diff/260001/components/arc/input/a... components/arc/input/arc_input_bridge.cc:74: weak_factory_.GetWeakPtr())); On 2015/12/04 at 22:35:29, denniskempin wrote: > On 2015/12/04 03:21:23, reveman wrote: > > why do we need to post a task for this if we're already on the correct thread? > > aura::Env is created after this class is. Unfortunately there is no hook in the browser parts that allows us to initialize stuff after aura::Env (it's initialized pre message loop run). > So we have to post a task that's executed with the message loop. hm, calling aura::Env::GetInstance() here and having this trigger initialization doesn't work? What if a window is initialized before this callback get to run? https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/DEPS File components/arc/input/DEPS (right): https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/D... components/arc/input/DEPS:2: "+ash", needed? https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/D... components/arc/input/DEPS:3: "+components/exo", you might need this in the future but is it really needed by this patch? https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... File components/arc/input/arc_input_bridge.h (right): https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge.h:27: virtual ~ArcInputBridge(){}; nit: s/(){};/() {}/ https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge.h:35: ArcInputBridge(){}; nit: s/(){};/() {}/ https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... File components/arc/input/arc_input_bridge_impl.cc (right): https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:10: #include "ash/shell.h" do you need this? https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:16: #include "components/exo/shell_surface.h" is this include needed? https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:201: if (event->type() == ui::ET_MOUSE_MOVED) { MOUSE_DRAGGED too? https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... File components/arc/input/arc_input_bridge_impl.h (right): https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.h:34: ArcInputBridgeImpl(ArcBridgeService* arc_bridge_service); nit: explicit
https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... File components/arc/input/arc_input_bridge.h (right): https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge.h:27: virtual ~ArcInputBridge(){}; On 2015/12/04 23:19:31, reveman wrote: > nit: s/(){};/() {}/ Done. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge.h:35: ArcInputBridge(){}; On 2015/12/04 23:19:32, reveman wrote: > nit: s/(){};/() {}/ Done. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... File components/arc/input/arc_input_bridge_impl.cc (right): https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:10: #include "ash/shell.h" On 2015/12/04 23:19:32, reveman wrote: > do you need this? not anymore. good catch. that'll get rid of some of the deps as well. I updated them. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:16: #include "components/exo/shell_surface.h" On 2015/12/04 23:19:32, reveman wrote: > is this include needed? ditto https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:201: if (event->type() == ui::ET_MOUSE_MOVED) { On 2015/12/04 23:19:32, reveman wrote: > MOUSE_DRAGGED too? Done. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:248: DCHECK_GE(fd.get(), 0); On 2015/12/04 22:52:08, Luis Héctor Chávez wrote: > DCHECK(fd.is_valid())? Done. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:325: close(fd[1]); // close write end. On 2015/12/04 22:52:08, Luis Héctor Chávez wrote: > Maybe it's better to create the base::ScopedFD before this block so that it will > be closed if anything goes wrong in the next few ifs. good idea. I would have missed some cases at the bottom. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:343: return base::ScopedFD(write_fd).Pass(); On 2015/12/04 22:52:07, Luis Héctor Chávez wrote: > We've moved to using std::move instead of Pass. Done. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... File components/arc/input/arc_input_bridge_impl.h (right): https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.h:34: ArcInputBridgeImpl(ArcBridgeService* arc_bridge_service); On 2015/12/04 23:19:32, reveman wrote: > nit: explicit Done. https://codereview.chromium.org/1408263006/diff/320001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.h:96: ArcBridgeService* arc_bridge_service_; On 2015/12/04 22:52:08, Luis Héctor Chávez wrote: > Just add a comment about lifetime and why it's safe to use this as-is. Done.
denniskempin@google.com changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1408263006/diff/340001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/1408263006/diff/340001/components/arc/BUILD.g... components/arc/BUILD.gn:25: "//ui", I'm surprised this compiles. //ui has no BUILD.gn, so I would have expected this to fail. As components/arc is new, I encourage you to add it to the whilelist in .gn (in the root directory). This way gn gen --check will fail if you have something wrong.
On 2015/12/05 00:16:49, sky wrote: > https://codereview.chromium.org/1408263006/diff/340001/components/arc/BUILD.gn > File components/arc/BUILD.gn (right): > > https://codereview.chromium.org/1408263006/diff/340001/components/arc/BUILD.g... > components/arc/BUILD.gn:25: "//ui", > I'm surprised this compiles. //ui has no BUILD.gn, so I would have expected this > to fail. As components/arc is new, I encourage you to add it to the whilelist in > .gn (in the root directory). This way gn gen --check will fail if you have > something wrong. Actually, it looks like arc is in components, which is checked. So, you shouldn't need to update .gn.
New DEPS LGTM
lgtm with a few nits and suggestions https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... File components/arc/input/arc_input_bridge.h (right): https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... components/arc/input/arc_input_bridge.h:27: virtual ~ArcInputBridge(){}; nit: virtual ~ArcInputBridge() {} https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... components/arc/input/arc_input_bridge.h:32: ArcBridgeService* arc_bridge_service); Not a huge fan of having a factory method when there's only one implementation. I would just rename ArcInputBridgeImpl to ArcInputBridge and use the ctor directly in ArcServiceManager. But I don't feel that strongly about this. If this somehow matches a pattern used in components/arc then fine. https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... components/arc/input/arc_input_bridge.h:35: ArcInputBridge(){}; nit: ArcInputBridge() {} https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... File components/arc/input/arc_input_bridge_impl.cc (right): https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:74: ArcInputBridgeImpl::~ArcInputBridgeImpl() { nit: aura::Env::GetInstance()->RemoveObserver(this) here https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:107: new_window->AddPreTargetHandler(this); This assumes that we outlive all windows. I would suggest that we at least DCHECK in the dtor to validate that this is the case.
https://codereview.chromium.org/1408263006/diff/340001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/1408263006/diff/340001/components/arc/BUILD.g... components/arc/BUILD.gn:25: "//ui", On 2015/12/05 00:16:49, sky wrote: > I'm surprised this compiles. //ui has no BUILD.gn, so I would have expected this > to fail. As components/arc is new, I encourage you to add it to the whilelist in > .gn (in the root directory). This way gn gen --check will fail if you have > something wrong. I updated it to //ui/aura and events. Same in DEPS. I am not sure if arc is built in gn builds. We are not using it at the moment. https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... File components/arc/input/arc_input_bridge.h (right): https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... components/arc/input/arc_input_bridge.h:27: virtual ~ArcInputBridge(){}; On 2015/12/05 00:48:45, reveman wrote: > nit: virtual ~ArcInputBridge() {} Done. https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... components/arc/input/arc_input_bridge.h:32: ArcBridgeService* arc_bridge_service); On 2015/12/05 00:48:45, reveman wrote: > Not a huge fan of having a factory method when there's only one implementation. > I would just rename ArcInputBridgeImpl to ArcInputBridge and use the ctor > directly in ArcServiceManager. But I don't feel that strongly about this. If > this somehow matches a pattern used in components/arc then fine. we would like to keep the includes from the implementation hidden from service_manager so we do not have to add DEPS there. https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... components/arc/input/arc_input_bridge.h:35: ArcInputBridge(){}; On 2015/12/05 00:48:45, reveman wrote: > nit: ArcInputBridge() {} Done. For real this time. https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... File components/arc/input/arc_input_bridge_impl.cc (right): https://codereview.chromium.org/1408263006/diff/380001/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:74: ArcInputBridgeImpl::~ArcInputBridgeImpl() { On 2015/12/05 00:48:45, reveman wrote: > nit: aura::Env::GetInstance()->RemoveObserver(this) here Done.
lgtm
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, jochen@chromium.org, lhchavez@chromium.org, sky@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1408263006/#ps420001 (title: "nit fix")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1496193002 Patch 80001). Please resolve the dependency and try again.
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, jochen@chromium.org, sky@chromium.org, lhchavez@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1408263006/#ps440001 (title: "rebase")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1496193002 Patch 140001). Please resolve the dependency and try again.
The CQ bit was checked by denniskempin@google.com
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1496193002 Patch 140001). Please resolve the dependency and try again.
The CQ bit was checked by denniskempin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408263006/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408263006/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, jochen@chromium.org, sky@chromium.org, lhchavez@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1408263006/#ps460001 (title: "added missing dependency to gn file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408263006/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408263006/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, jochen@chromium.org, sky@chromium.org, lhchavez@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1408263006/#ps480001 (title: "removed dependency on ui/events/ozone to fix generic chromium os builder")
Description was changed from ========== This CL adds the ArcBridgeService which hooks into aura::Windows that are running ARC applications to translate and transmit their ui::Events to the ARC instance. BUG=chromium:558499 TEST=Mouse, Touch and Keyboard events should be properly working on an ARC application window. ========== to ========== This CL adds the ArcInputBridge which hooks into aura::Windows that are running ARC applications to translate and transmit their ui::Events to the ARC instance. BUG=chromium:558499 TEST=Mouse, Touch and Keyboard events should be properly working on an ARC application window. ==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408263006/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408263006/480001
The CQ bit was unchecked by denniskempin@google.com
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, jochen@chromium.org, sky@chromium.org, lhchavez@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1408263006/#ps500001 (title: "rebase after mojo changes")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1441683007 Patch 60001). Please resolve the dependency and try again.
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, jochen@chromium.org, sky@chromium.org, lhchavez@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1408263006/#ps510009 (title: "removed CL dependency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408263006/510009 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408263006/510009
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1408263006/diff/510009/components/arc/input/a... File components/arc/input/arc_input_bridge_impl.cc (right): https://codereview.chromium.org/1408263006/diff/510009/components/arc/input/a... components/arc/input/arc_input_bridge_impl.cc:113: if (new_window->name() == "ExoSurface") { Fyi, as an alternative to this you can now use exo::Surface::AsSurface(aura_window) to determine if a surface is an exo surface or not. However, this is not going to be set at the time OnWindowInitialized is called so you'd have to either observe each window for property changes or just install the pre-target handler for the root window and then check at dispatch time if it's a exo surface or not. The best would be to switch over to using exo::Pointer, exo::Keyboard and exo::Touch once all the code for those classes have landed.
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, jochen@chromium.org, sky@chromium.org, lhchavez@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1408263006/#ps530001 (title: "fixed clang warnings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408263006/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408263006/530001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by denniskempin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408263006/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408263006/530001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, jochen@chromium.org, sky@chromium.org, lhchavez@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1408263006/#ps550001 (title: "only attach observer if aura::Env exists")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408263006/550001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408263006/550001
The CQ bit was unchecked by denniskempin@google.com
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, jochen@chromium.org, sky@chromium.org, lhchavez@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1408263006/#ps570001 (title: "nit fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408263006/570001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408263006/570001
Message was sent while issue was closed.
Description was changed from ========== This CL adds the ArcInputBridge which hooks into aura::Windows that are running ARC applications to translate and transmit their ui::Events to the ARC instance. BUG=chromium:558499 TEST=Mouse, Touch and Keyboard events should be properly working on an ARC application window. ========== to ========== This CL adds the ArcInputBridge which hooks into aura::Windows that are running ARC applications to translate and transmit their ui::Events to the ARC instance. BUG=chromium:558499 TEST=Mouse, Touch and Keyboard events should be properly working on an ARC application window. ==========
Message was sent while issue was closed.
Committed patchset #30 (id:570001)
Message was sent while issue was closed.
Description was changed from ========== This CL adds the ArcInputBridge which hooks into aura::Windows that are running ARC applications to translate and transmit their ui::Events to the ARC instance. BUG=chromium:558499 TEST=Mouse, Touch and Keyboard events should be properly working on an ARC application window. ========== to ========== This CL adds the ArcInputBridge which hooks into aura::Windows that are running ARC applications to translate and transmit their ui::Events to the ARC instance. BUG=chromium:558499 TEST=Mouse, Touch and Keyboard events should be properly working on an ARC application window. Committed: https://crrev.com/3b12f96225546fdcae48735e15db9124ce2add95 Cr-Commit-Position: refs/heads/master@{#363941} ==========
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/3b12f96225546fdcae48735e15db9124ce2add95 Cr-Commit-Position: refs/heads/master@{#363941} |
