Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1992443002:
Add Mojo IPC based input-device service. (Closed)
|
Created:
4 years, 7 months ago by kylechar Modified:
4 years, 6 months ago CC:
chromium-reviews, rjkroege, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, tdresser+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, kalyank, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@cleanup_ddm Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Mojo IPC based input-device service.
Add Mojo interfaces to pass information about input-devices between
processes.
InputDeviceServer listens for updates about input-devices from
DeviceDataManager and holds a list of out of process observers. It
forwards updates via Mojo IPC to observers.
InputDeviceClient registers as a Mojo observer with InputDeviceServer
and holds a list of in process observers. It receives updates about input-devices
from InputDeviceServer, caches the information and notifies in process
observers.
InputDeviceClient will replace the stub DeviceDataManager instance that
is created in ash and chrome.
Code is unused at this point, will be activated in a followup CL.
BUG=601981
Committed: https://crrev.com/edbac87b7a0fbe93a323840e2f143686e4065b10
Cr-Commit-Position: refs/heads/master@{#399986}
Patch Set 1 #Patch Set 2 : Clean up/move files. #
Total comments: 2
Patch Set 3 : Updated. #
Total comments: 1
Patch Set 4 : Rebase/move. #
Total comments: 4
Patch Set 5 : Rebase/update. #
Total comments: 23
Patch Set 6 : Fixes for comments. #
Total comments: 18
Patch Set 7 : Split CL and fixes. #Patch Set 8 : Rebase/comments. #Patch Set 9 : Implementation only. #
Total comments: 14
Patch Set 10 : Fix nits. #Patch Set 11 : Change namespace. #Dependent Patchsets: Messages
Total messages: 47 (12 generated)
Description was changed from ========== Introduce Mojo IPC based input-device service. WIP. BUG=601981 ========== to ========== Introduce Mojo IPC based input-device service. Add Mojo IPC interfaces and strcts necessary to pass information about input devices between processes. InputDeviceServer listens for updates about input devices and holds a list of Mojo observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. BUG=601981 ==========
kylechar@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@chromium.org changed reviewers: + ben@chromium.org
+ben@ for his thoughts on the right place for this. I was thinking perhaps this could go in //services/input_device_manager/? You should put the mojom in <location>/public/interfaces/, client-side code in <location>/public/cpp (header in cpp, impls in cpp/lib), and set security folks as owner for mojom (I think you can just copy over mus/public/interfaces/OWNERS). https://codereview.chromium.org/1992443002/diff/20001/components/input_device... File components/input_device_service/input_device_server.cc (right): https://codereview.chromium.org/1992443002/diff/20001/components/input_device... components/input_device_service/input_device_server.cc:60: DeviceDataManager::GetInstance()->AddObserver(this); What creates this DDM instance?
On 2016/05/25 20:40:08, sadrul wrote: > +ben@ for his thoughts on the right place for this. > > I was thinking perhaps this could go in //services/input_device_manager/? > > You should put the mojom in <location>/public/interfaces/, client-side code in > <location>/public/cpp (header in cpp, impls in cpp/lib), and set security folks > as owner for mojom (I think you can just copy over > mus/public/interfaces/OWNERS). > > https://codereview.chromium.org/1992443002/diff/20001/components/input_device... > File components/input_device_service/input_device_server.cc (right): > > https://codereview.chromium.org/1992443002/diff/20001/components/input_device... > components/input_device_service/input_device_server.cc:60: > DeviceDataManager::GetInstance()->AddObserver(this); > What creates this DDM instance? Is there a reason why mus shouldn't provide these interfaces?
On 2016/05/25 20:47:47, Ben Goodger (Google) wrote: > On 2016/05/25 20:40:08, sadrul wrote: > > +ben@ for his thoughts on the right place for this. > > > > I was thinking perhaps this could go in //services/input_device_manager/? > > > > You should put the mojom in <location>/public/interfaces/, client-side code in > > <location>/public/cpp (header in cpp, impls in cpp/lib), and set security > folks > > as owner for mojom (I think you can just copy over > > mus/public/interfaces/OWNERS). > > > > > https://codereview.chromium.org/1992443002/diff/20001/components/input_device... > > File components/input_device_service/input_device_server.cc (right): > > > > > https://codereview.chromium.org/1992443002/diff/20001/components/input_device... > > components/input_device_service/input_device_server.cc:60: > > DeviceDataManager::GetInstance()->AddObserver(this); > > What creates this DDM instance? > > Is there a reason why mus shouldn't provide these interfaces? The InputDeviceService mojo interface is provided by mus currently. It's implemented in the class InputDeviceServer to keep the code self contained but the interface is exposed by "mojo:mus". When I discussed with sadrul@ he was interested in keeping the service modular with the possibility of running outside mus-ws one day though, so it seemed to make more sense having everything in a separate directory. The InputDeviceClient part should be separate from mus regardless as it will be used by ash_sysui, chrome, etc.
On 2016/05/25 21:03:18, kylechar wrote: > The InputDeviceService mojo interface is provided by mus currently. It's > implemented in the class InputDeviceServer to keep the code self contained but > the interface is exposed by "mojo:mus". > > When I discussed with sadrul@ he was interested in keeping the service modular > with the possibility of running outside mus-ws one day though, so it seemed to > make more sense having everything in a separate directory. The InputDeviceClient > part should be separate from mus regardless as it will be used by ash_sysui, > chrome, etc. Seems like you can keep the code isolated within mus (by having a separate source_set, DEPS, etc.). I'm not opposed to a device service (e.g. supporting geolocation, bluetooth etc) and maybe this stuff feels like it has affinity with that... but I'm not sure... you could also make the case that it belongs with Mus. If we create a new service there's sort of an assertion that there'll ultimately be a separate process for it. Is there a case for this for these methods?
On 2016/05/25 22:01:38, Ben Goodger (Google) wrote: > On 2016/05/25 21:03:18, kylechar wrote: > > The InputDeviceService mojo interface is provided by mus currently. It's > > implemented in the class InputDeviceServer to keep the code self contained but > > the interface is exposed by "mojo:mus". > > > > When I discussed with sadrul@ he was interested in keeping the service modular > > with the possibility of running outside mus-ws one day though, so it seemed to > > make more sense having everything in a separate directory. The > InputDeviceClient > > part should be separate from mus regardless as it will be used by ash_sysui, > > chrome, etc. > > Seems like you can keep the code isolated within mus (by having a separate > source_set, DEPS, etc.). > I'm not opposed to a device service (e.g. supporting geolocation, bluetooth etc) > and maybe this stuff feels like it has affinity with that... but I'm not sure... > you could also make the case that it belongs with Mus. If we create a new > service there's sort of an assertion that there'll ultimately be a separate > process for it. Is there a case for this for these methods? blink currently receives information about the input-devices on the system from the browser (through WebPreferences). I was imagining with this new service, blink would receive that information directly from the browser instead (regardless of whether mus is used or not at the time). I was thinking geolocation/bluetooth etc. would be their own separate services, and not be bundled with this input-device service though. re separate process: I don't think a separate process is going to be necessary, although it would make sense to turn off the service/process when there are no clients interested to know about the device changes in the system.
On 2016/05/27 14:25:19, sadrul wrote: > On 2016/05/25 22:01:38, Ben Goodger (Google) wrote: > > On 2016/05/25 21:03:18, kylechar wrote: > > > The InputDeviceService mojo interface is provided by mus currently. It's > > > implemented in the class InputDeviceServer to keep the code self contained > but > > > the interface is exposed by "mojo:mus". > > > > > > When I discussed with sadrul@ he was interested in keeping the service > modular > > > with the possibility of running outside mus-ws one day though, so it seemed > to > > > make more sense having everything in a separate directory. The > > InputDeviceClient > > > part should be separate from mus regardless as it will be used by ash_sysui, > > > chrome, etc. > > > > Seems like you can keep the code isolated within mus (by having a separate > > source_set, DEPS, etc.). > > I'm not opposed to a device service (e.g. supporting geolocation, bluetooth > etc) > > and maybe this stuff feels like it has affinity with that... but I'm not > sure... > > you could also make the case that it belongs with Mus. If we create a new > > service there's sort of an assertion that there'll ultimately be a separate > > process for it. Is there a case for this for these methods? > > blink currently receives information about the input-devices on the system from > the browser (through WebPreferences). I was imagining with this new service, > blink would receive that information directly from the browser instead > (regardless of whether mus is used or not at the time). > > I was thinking geolocation/bluetooth etc. would be their own separate services, > and not be bundled with this input-device service though. > > re separate process: I don't think a separate process is going to be necessary, > although it would make sense to turn off the service/process when there are no > clients interested to know about the device changes in the system. So I think we should only be creating new subdirs under //services for things that would probably run in their own process in an unrestricted environment (e.g. cros). What you're describing is more akin to a static lib that implements a mojom. In this case, mus would depend on this lib, and its ShellClient would add the lib's interface impls to incoming connections. //content would also depend on this lib, and expose the interface to renderers. This suggests perhaps //components/input_devices... however an alternative would be to still put it in mus, but in a subdir (e.g. //components/mus/input_devices) and use DEPS to isolate it from the rest, & put its files in a separate target. That might be more consistent (fewer ultimate filemoves) with the end goal.
On 2016/05/27 15:13:05, Ben Goodger (Google) wrote: > On 2016/05/27 14:25:19, sadrul wrote: > > On 2016/05/25 22:01:38, Ben Goodger (Google) wrote: > > > On 2016/05/25 21:03:18, kylechar wrote: > > > > The InputDeviceService mojo interface is provided by mus currently. It's > > > > implemented in the class InputDeviceServer to keep the code self contained > > but > > > > the interface is exposed by "mojo:mus". > > > > > > > > When I discussed with sadrul@ he was interested in keeping the service > > modular > > > > with the possibility of running outside mus-ws one day though, so it > seemed > > to > > > > make more sense having everything in a separate directory. The > > > InputDeviceClient > > > > part should be separate from mus regardless as it will be used by > ash_sysui, > > > > chrome, etc. > > > > > > Seems like you can keep the code isolated within mus (by having a separate > > > source_set, DEPS, etc.). > > > I'm not opposed to a device service (e.g. supporting geolocation, bluetooth > > etc) > > > and maybe this stuff feels like it has affinity with that... but I'm not > > sure... > > > you could also make the case that it belongs with Mus. If we create a new > > > service there's sort of an assertion that there'll ultimately be a separate > > > process for it. Is there a case for this for these methods? > > > > blink currently receives information about the input-devices on the system > from > > the browser (through WebPreferences). I was imagining with this new service, > > blink would receive that information directly from the browser instead > > (regardless of whether mus is used or not at the time). > > > > I was thinking geolocation/bluetooth etc. would be their own separate > services, > > and not be bundled with this input-device service though. > > > > re separate process: I don't think a separate process is going to be > necessary, > > although it would make sense to turn off the service/process when there are no > > clients interested to know about the device changes in the system. > > So I think we should only be creating new subdirs under //services for things > that would probably run in their own process in an unrestricted environment > (e.g. cros). What you're describing is more akin to a static lib that implements > a mojom. > > In this case, mus would depend on this lib, and its ShellClient would add the > lib's interface impls to incoming connections. //content would also depend on > this lib, and expose the interface to renderers. > > This suggests perhaps //components/input_devices... however an alternative would > be to still put it in mus, but in a subdir (e.g. //components/mus/input_devices) > and use DEPS to isolate it from the rest, & put its files in a separate target. > That might be more consistent (fewer ultimate filemoves) with the end goal. sgtm. Let's go with //components/mus/input_devices/
On 2016/05/27 15:17:05, sadrul wrote: > On 2016/05/27 15:13:05, Ben Goodger (Google) wrote: > > On 2016/05/27 14:25:19, sadrul wrote: > > > On 2016/05/25 22:01:38, Ben Goodger (Google) wrote: > > > > On 2016/05/25 21:03:18, kylechar wrote: > > > > > The InputDeviceService mojo interface is provided by mus currently. It's > > > > > implemented in the class InputDeviceServer to keep the code self > contained > > > but > > > > > the interface is exposed by "mojo:mus". > > > > > > > > > > When I discussed with sadrul@ he was interested in keeping the service > > > modular > > > > > with the possibility of running outside mus-ws one day though, so it > > seemed > > > to > > > > > make more sense having everything in a separate directory. The > > > > InputDeviceClient > > > > > part should be separate from mus regardless as it will be used by > > ash_sysui, > > > > > chrome, etc. > > > > > > > > Seems like you can keep the code isolated within mus (by having a separate > > > > source_set, DEPS, etc.). > > > > I'm not opposed to a device service (e.g. supporting geolocation, > bluetooth > > > etc) > > > > and maybe this stuff feels like it has affinity with that... but I'm not > > > sure... > > > > you could also make the case that it belongs with Mus. If we create a new > > > > service there's sort of an assertion that there'll ultimately be a > separate > > > > process for it. Is there a case for this for these methods? > > > > > > blink currently receives information about the input-devices on the system > > from > > > the browser (through WebPreferences). I was imagining with this new service, > > > blink would receive that information directly from the browser instead > > > (regardless of whether mus is used or not at the time). > > > > > > I was thinking geolocation/bluetooth etc. would be their own separate > > services, > > > and not be bundled with this input-device service though. > > > > > > re separate process: I don't think a separate process is going to be > > necessary, > > > although it would make sense to turn off the service/process when there are > no > > > clients interested to know about the device changes in the system. > > > > So I think we should only be creating new subdirs under //services for things > > that would probably run in their own process in an unrestricted environment > > (e.g. cros). What you're describing is more akin to a static lib that > implements > > a mojom. > > > > In this case, mus would depend on this lib, and its ShellClient would add the > > lib's interface impls to incoming connections. //content would also depend on > > this lib, and expose the interface to renderers. > > > > This suggests perhaps //components/input_devices... however an alternative > would > > be to still put it in mus, but in a subdir (e.g. > //components/mus/input_devices) > > and use DEPS to isolate it from the rest, & put its files in a separate > target. > > That might be more consistent (fewer ultimate filemoves) with the end goal. > > sgtm. Let's go with //components/mus/input_devices/ Do we want directory structure within mus to match processes? By making directory structure match processes then we easily identify privileged and unprivileged code, and public APIs versus private APIs, etc.
On 2016/05/27 15:50:22, Fady Samuel wrote: > Do we want directory structure within mus to match processes? By making > directory structure match processes then we easily identify privileged and > unprivileged code, and public APIs versus private APIs, etc. I think that might be unnecessarily restrictive or at least introduce more layers of subdirs. "Privilege" is nuanced here. At the very least, perhaps the untrusted code should live in one subdir & the rest outside of it?
Okay, //components/mus/input_devices/ it is. sadrul@, the client part goes in components/mus/input_devices/public/cpp/ as I understand. What about the server part, just in components/mus/input_devices/ since it's not really public? https://codereview.chromium.org/1992443002/diff/20001/components/input_device... File components/input_device_service/input_device_server.cc (right): https://codereview.chromium.org/1992443002/diff/20001/components/input_device... components/input_device_service/input_device_server.cc:60: DeviceDataManager::GetInstance()->AddObserver(this); On 2016/05/25 20:40:08, sadrul wrote: > What creates this DDM instance? It's created as part of Ozone initialization or as part of PlatformEventSource initialization, depending on a couple different things (eg. are you running under Ozone, what Ozone platform, etc). The instance can be either a DeviceDataManager or DeviceDataManagerX11 right now. That should hopefully all go away in the future and possible InputDeviceServer could own DDM. It would also avoid the need for this method that gets called after DDM is created.
On 2016/05/27 19:52:22, kylechar wrote: > Okay, //components/mus/input_devices/ it is. sadrul@, the client part goes in > components/mus/input_devices/public/cpp/ as I understand. What about the server > part, just in components/mus/input_devices/ since it's not really public? What is the client part? I don't see that in your CL.
On 2016/05/27 19:54:05, Ben Goodger (Google) wrote: > On 2016/05/27 19:52:22, kylechar wrote: > > Okay, //components/mus/input_devices/ it is. sadrul@, the client part goes in > > components/mus/input_devices/public/cpp/ as I understand. What about the > server > > part, just in components/mus/input_devices/ since it's not really public? > > What is the client part? I don't see that in your CL. InputDeviceClient is the client bit. It starts in a process that isn't mus-ws and adds itself as an observer to InputDeviceServer (via mojo IPC). InputDeviceServer lives in the mus-ws process and listens for local input-device changes and notifies all of the observers in other processes. This CL adds starts up InputDeviceClient in the ash_sysui process but the chrome browser process (and maybe renderer processes too?) need to know about input devices.
On 2016/05/27 20:00:30, kylechar wrote: > On 2016/05/27 19:54:05, Ben Goodger (Google) wrote: > > On 2016/05/27 19:52:22, kylechar wrote: > > > Okay, //components/mus/input_devices/ it is. sadrul@, the client part goes > in > > > components/mus/input_devices/public/cpp/ as I understand. What about the > > server > > > part, just in components/mus/input_devices/ since it's not really public? > > > > What is the client part? I don't see that in your CL. > > InputDeviceClient is the client bit. It starts in a process that isn't mus-ws > and adds itself as an observer to InputDeviceServer (via mojo IPC). > InputDeviceServer lives in the mus-ws process and listens for local input-device > changes and notifies all of the observers in other processes. > > This CL adds starts up InputDeviceClient in the ash_sysui process but the chrome > browser process (and maybe renderer processes too?) need to know about input > devices. I see. In that case, I'd like the client stuff to end up somewhere under components/mus/public/cpp, though you can create a input_devices subdir under there if you want to isolate it (you should). Please don't use the suffix "service" in class names, file names or paths. I'm trying to assert more specific meaning of it than just some interface/impl. Just call it InputDevices.
Description was changed from ========== Introduce Mojo IPC based input-device service. Add Mojo IPC interfaces and strcts necessary to pass information about input devices between processes. InputDeviceServer listens for updates about input devices and holds a list of Mojo observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. BUG=601981 ========== to ========== Introduce Mojo IPC based input-device service. Add Mojo IPC interfaces and structs necessary to pass information about input devices between processes. InputDeviceServer listens for updates about input devices and holds a list of Mojo observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. BUG=601981 ==========
I've updated the CL. It's got a bit of debug and other junk in it still (to be removed before submission), plus it has some big changes it depends on that are unlanded, but InputDeviceServer/InputDeviceClient/mojoms/StructTraits could benefit from a once over from someone with more Mojo experience.
https://codereview.chromium.org/1992443002/diff/40001/components/mus/input_de... File components/mus/input_devices/public/interfaces/input_devices.mojom (right): https://codereview.chromium.org/1992443002/diff/40001/components/mus/input_de... components/mus/input_devices/public/interfaces/input_devices.mojom:5: module input_device.mojom; this dir should be in: components/mus/public/interfaces/input_devices
On 2016/06/02 18:06:44, Ben Goodger (Google) wrote: > https://codereview.chromium.org/1992443002/diff/40001/components/mus/input_de... > File components/mus/input_devices/public/interfaces/input_devices.mojom (right): > > https://codereview.chromium.org/1992443002/diff/40001/components/mus/input_de... > components/mus/input_devices/public/interfaces/input_devices.mojom:5: module > input_device.mojom; > this dir should be in: > > components/mus/public/interfaces/input_devices Whoops, misread that originally. Moved to components/mus/public/cpp/input_devices/ and components/mus/public/interfaces/input_devices/ respectively.
https://codereview.chromium.org/1992443002/diff/60001/components/mus/public/c... File components/mus/public/cpp/input_devices/lib/input_device_client.cc (right): https://codereview.chromium.org/1992443002/diff/60001/components/mus/public/c... components/mus/public/cpp/input_devices/lib/input_device_client.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. we're going to get rid of lib subdirs... so just put the .cc alongside the .h https://codereview.chromium.org/1992443002/diff/60001/components/mus/public/c... components/mus/public/cpp/input_devices/lib/input_device_client.cc:39: void InputDeviceClient::Connect(shell::Connector* connector) { rather than passing in a connector, can you require the client of this lib perform this connection & just pass in the InputDeviceServicePtr?
https://codereview.chromium.org/1992443002/diff/60001/components/mus/public/c... File components/mus/public/cpp/input_devices/lib/input_device_client.cc (right): https://codereview.chromium.org/1992443002/diff/60001/components/mus/public/c... components/mus/public/cpp/input_devices/lib/input_device_client.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/02 19:35:18, Ben Goodger (Google) wrote: > we're going to get rid of lib subdirs... so just put the .cc alongside the .h Done. Will upload a new patch after rebasing later today. https://codereview.chromium.org/1992443002/diff/60001/components/mus/public/c... components/mus/public/cpp/input_devices/lib/input_device_client.cc:39: void InputDeviceClient::Connect(shell::Connector* connector) { On 2016/06/02 19:35:18, Ben Goodger (Google) wrote: > rather than passing in a connector, can you require the client of this lib > perform this connection & just pass in the InputDeviceServicePtr? Yeah, that's absolutely possible. I'm just getting familiar with Mojo in general, is there an advantage to that way? I can see a small disadvantage, as clients of this lib needs to include the InputDeviceServer mojom and connect to mus. Is the advantage that we can connect via other means, not just the shell:Connector?
On 2016/06/03 14:35:58, kylechar wrote: https://codereview.chromium.org/1992443002/diff/60001/components/mus/public/c... > components/mus/public/cpp/input_devices/lib/input_device_client.cc:39: void > InputDeviceClient::Connect(shell::Connector* connector) { > On 2016/06/02 19:35:18, Ben Goodger (Google) wrote: > > rather than passing in a connector, can you require the client of this lib > > perform this connection & just pass in the InputDeviceServicePtr? > > Yeah, that's absolutely possible. I'm just getting familiar with Mojo in > general, is there an advantage to that way? I can see a small disadvantage, as > clients of this lib needs to include the InputDeviceServer mojom and connect to > mus. Is the advantage that we can connect via other means, not just the > shell:Connector? The remote services & interfaces code can connect & bind to are determined by the capability spec of the application that the code runs in. By burying the connect in the client library, it means the user of the client library must somehow know what services/interfaces the client lib connects to & add it to their capability spec. By making the user do this instead it becomes explicit to the client what they need to list.
On 2016/06/03 15:04:33, Ben Goodger (Google) wrote: > On 2016/06/03 14:35:58, kylechar wrote: > https://codereview.chromium.org/1992443002/diff/60001/components/mus/public/c... > > components/mus/public/cpp/input_devices/lib/input_device_client.cc:39: void > > InputDeviceClient::Connect(shell::Connector* connector) { > > On 2016/06/02 19:35:18, Ben Goodger (Google) wrote: > > > rather than passing in a connector, can you require the client of this lib > > > perform this connection & just pass in the InputDeviceServicePtr? > > > > Yeah, that's absolutely possible. I'm just getting familiar with Mojo in > > general, is there an advantage to that way? I can see a small disadvantage, as > > clients of this lib needs to include the InputDeviceServer mojom and connect > to > > mus. Is the advantage that we can connect via other means, not just the > > shell:Connector? > > The remote services & interfaces code can connect & bind to are determined by > the capability spec of the application that the code runs in. By burying the > connect in the client library, it means the user of the client library must > somehow know what services/interfaces the client lib connects to & add it to > their capability spec. By making the user do this instead it becomes explicit to > the client what they need to list. That makes sense. I've fixed the issues brought up and rebased/updated the CL.
https://codereview.chromium.org/1992443002/diff/80001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/1992443002/diff/80001/ash/shell.cc#newcode1112 ash/shell.cc:1112: touch_transformer_controller_.reset(new TouchTransformerController()); This could be a separate CL https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... File components/mus/input_devices/input_device.typemap (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... components/mus/input_devices/input_device.typemap:6: public_headers = [ "//ui/events/devices/input_device.h" ] I think this file should go in //ui/events/mojo (or maybe //ui/events/devices/mojo) https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... File components/mus/input_devices/input_device_server.h (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... components/mus/input_devices/input_device_server.h:21: // Mojo IPC. The mus-ws process should will own this. s/should//? https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... File components/mus/input_devices/input_device_struct_traits.h (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... components/mus/input_devices/input_device_struct_traits.h:32: static std::string name(const ui::InputDevice& device) { return device.name; } Can the return type be const-ref in struct-traits? https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... components/mus/input_devices/input_device_struct_traits.h:60: default: Remove default case? https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/c... File components/mus/public/cpp/input_devices/input_device_client.cc (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/c... components/mus/public/cpp/input_devices/input_device_client.cc:25: LOG(ERROR) << "InputDeviceClient failed to bind observer pipe."; DLOG https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/c... File components/mus/public/cpp/input_devices/input_device_client.h (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/c... components/mus/public/cpp/input_devices/input_device_client.h:43: // mojom::InputDeviceObserverMojo: Make these overrides private (or protected) to clarify that these are not part of the API https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... File components/mus/public/interfaces/input_devices/input_devices.mojom (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... components/mus/public/interfaces/input_devices/input_devices.mojom:29: InputDevice input_device; Hm, do we do subclassing like this in other places too? I was thinking mojom.InputDevice would have an optional TouchscreenInfo{size, touch_points} field, which would be null for non-touchscreen devices. (that's how do this for mojom.Event, for example) https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... components/mus/public/interfaces/input_devices/input_devices.mojom:33: }; The above should move into //ui/events/mojo/ (or //ui/events/devices/mojo/) https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... components/mus/public/interfaces/input_devices/input_devices.mojom:37: OnKeyboardDeviceConfigurationChanged(array<InputDevice> devices); Can you document these? For example, when an observer connects, and the InputDeviceServer has already scanned all the devices by that time, does the observer receive OnKeyboardDeviceConfigurationChanged() etc. calls once immediately after connecting? https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... components/mus/public/interfaces/input_devices/input_devices.mojom:41: OnDeviceListsComplete(); I am also wondering if OnDevicesListsComplete() is necessary. Ideally, the server would respond to the client only once the list is complete, and so the client can decide whether it has a valid list of devices depending on whether it received a response from the server or not.
Have fixed most of the issues. See my replies for the things that haven't been fixed. https://codereview.chromium.org/1992443002/diff/80001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/1992443002/diff/80001/ash/shell.cc#newcode1112 ash/shell.cc:1112: touch_transformer_controller_.reset(new TouchTransformerController()); On 2016/06/07 03:39:25, sadrul wrote: > This could be a separate CL Done. https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... File components/mus/input_devices/input_device.typemap (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... components/mus/input_devices/input_device.typemap:6: public_headers = [ "//ui/events/devices/input_device.h" ] On 2016/06/07 03:39:25, sadrul wrote: > I think this file should go in //ui/events/mojo (or maybe > //ui/events/devices/mojo) Done. https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... File components/mus/input_devices/input_device_server.h (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... components/mus/input_devices/input_device_server.h:21: // Mojo IPC. The mus-ws process should will own this. On 2016/06/07 03:39:25, sadrul wrote: > s/should//? Done. https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... File components/mus/input_devices/input_device_struct_traits.h (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... components/mus/input_devices/input_device_struct_traits.h:32: static std::string name(const ui::InputDevice& device) { return device.name; } On 2016/06/07 03:39:25, sadrul wrote: > Can the return type be const-ref in struct-traits? I don't see why not. Done. https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... components/mus/input_devices/input_device_struct_traits.h:60: default: On 2016/06/07 03:39:25, sadrul wrote: > Remove default case? Do we want that? We can't necessarily control what data we receive to deserialize, so it's possible an invalid enum value might be there? I'm not totally sure what the best practice is here. https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/c... File components/mus/public/cpp/input_devices/input_device_client.cc (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/c... components/mus/public/cpp/input_devices/input_device_client.cc:25: LOG(ERROR) << "InputDeviceClient failed to bind observer pipe."; On 2016/06/07 03:39:26, sadrul wrote: > DLOG Done. https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/c... File components/mus/public/cpp/input_devices/input_device_client.h (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/c... components/mus/public/cpp/input_devices/input_device_client.h:43: // mojom::InputDeviceObserverMojo: On 2016/06/07 03:39:26, sadrul wrote: > Make these overrides private (or protected) to clarify that these are not part > of the API Done. https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... File components/mus/public/interfaces/input_devices/input_devices.mojom (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... components/mus/public/interfaces/input_devices/input_devices.mojom:29: InputDevice input_device; On 2016/06/07 03:39:26, sadrul wrote: > Hm, do we do subclassing like this in other places too? > > I was thinking mojom.InputDevice would have an optional TouchscreenInfo{size, > touch_points} field, which would be null for non-touchscreen devices. (that's > how do this for mojom.Event, for example) Oh, so looking at mojom.Event it's definitely done differently. I'm not sure we do it this way anywhere else yet. I'm also not sure we want to do this like mojom.Event though. With that approach we'd get back a std::unique_ptr<ui::InputEvent> for both ui::InputEvent and ui::TouchscreenEvent. We don't really want that here though, so we want to get back the class we serialized. It would require an extra step to copy from std::vector<std::unique_ptr<ui::InputEvent>> to a std::vector<ui::InputEvent> or std::vector<ui::Touchscreenevent>. https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... components/mus/public/interfaces/input_devices/input_devices.mojom:33: }; On 2016/06/07 03:39:26, sadrul wrote: > The above should move into //ui/events/mojo/ (or //ui/events/devices/mojo/) Done. https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... components/mus/public/interfaces/input_devices/input_devices.mojom:37: OnKeyboardDeviceConfigurationChanged(array<InputDevice> devices); On 2016/06/07 03:39:26, sadrul wrote: > Can you document these? For example, when an observer connects, and the > InputDeviceServer has already scanned all the devices by that time, does the > observer receive OnKeyboardDeviceConfigurationChanged() etc. calls once > immediately after connecting? I added better documentation. Most of it is under InputDeviceServer, just to avoid duplicating the documentation (and having it get out of sync when things eventually change). https://codereview.chromium.org/1992443002/diff/80001/components/mus/public/i... components/mus/public/interfaces/input_devices/input_devices.mojom:41: OnDeviceListsComplete(); On 2016/06/07 03:39:26, sadrul wrote: > I am also wondering if OnDevicesListsComplete() is necessary. Ideally, the > server would respond to the client only once the list is complete, and so the > client can decide whether it has a valid list of devices depending on whether it > received a response from the server or not. I believe it's the easiest/best way to do this. By the time all the Mojo setup happens, DeviceDataManager has probably received the OnDeviceListsComplete call. However, we can't guarantee that. Regardless, we don't send IPCs for empty device lists so the client doesn't know if it hasn't received the device list yet or if the list is actually empty until it gets OnDevicesListsComplete(). We could send an IPC for each device type even if it's empty but that would require more IPC calls. It would also require more complicated logic in InputDeviceClient.
https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... File components/mus/input_devices/input_device_struct_traits.h (right): https://codereview.chromium.org/1992443002/diff/80001/components/mus/input_de... components/mus/input_devices/input_device_struct_traits.h:60: default: On 2016/06/07 16:36:41, kylechar wrote: > On 2016/06/07 03:39:25, sadrul wrote: > > Remove default case? > > Do we want that? We can't necessarily control what data we receive to > deserialize, so it's possible an invalid enum value might be there? I'm not > totally sure what the best practice is here. Hm, good point. I was going for the 'omit default: so if there are new enums, compiler tells us to update the code' benefit, but you raise a good point. Perhaps tsepez@ can suggest if this is OK? https://codereview.chromium.org/1992443002/diff/100001/components/mus/input_d... File components/mus/input_devices/input_device_server.h (right): https://codereview.chromium.org/1992443002/diff/100001/components/mus/input_d... components/mus/input_devices/input_device_server.h:31: void RegisterAsObserver(); This doesn't seem to be called from anywhere yet? https://codereview.chromium.org/1992443002/diff/100001/components/mus/input_d... components/mus/input_devices/input_device_server.h:39: void Create(shell::Connection* connection, This could be private too I think? https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... File components/mus/public/cpp/input_devices/input_device_client.cc (right): https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.cc:25: DLOG(ERROR) << "InputDeviceClient failed to bind observer pipe."; Note: the failure can happen after bind succeeds. https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... File components/mus/public/cpp/input_devices/input_device_client.h (right): https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.h:43: // ui::InputDeviceManager: Make these privates too? https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... File components/mus/public/interfaces/input_devices/input_devices.mojom (right): https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... components/mus/public/interfaces/input_devices/input_devices.mojom:21: OnDeviceListsComplete(); OK. So, it sounds like once the observer registers itself, it's can get upto 5 IPC messages (4 for the device-type lists, and then this OnDeviceListsComplete()). Can these be merged into one, e.g. can OnDeviceLists() include the various lists? https://codereview.chromium.org/1992443002/diff/100001/mojo/public/tools/bind... File mojo/public/tools/bindings/chromium_bindings_configuration.gni (right): https://codereview.chromium.org/1992443002/diff/100001/mojo/public/tools/bind... mojo/public/tools/bindings/chromium_bindings_configuration.gni:14: "//ui/events/devices/mojo/typemaps.gni", sort https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... File ui/events/devices/mojo/OWNERS (right): https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... ui/events/devices/mojo/OWNERS:1: kylechar@chromium.org security folks should also be mojo OWNERS. I think you can copy over one of the existing owner files (e.g. ui/gfx/mojo/OWNERS) https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... File ui/events/devices/mojo/touchscreen_device_struct_traits.h (right): https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... ui/events/devices/mojo/touchscreen_device_struct_traits.h:24: static gfx::Size size(const ui::TouchscreenDevice& device) { const & https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... ui/events/devices/mojo/touchscreen_device_struct_traits.h:45: This could maybe go in the input_device_struct_traits.h too? (same for .typemap)
(btw: you could split up the struct-traits into its own CL)
I've moved the Mojo structs and StructTraits to a new CL and made the requested fixes (or at least replied with some more context). https://codereview.chromium.org/1992443002/diff/100001/components/mus/input_d... File components/mus/input_devices/input_device_server.h (right): https://codereview.chromium.org/1992443002/diff/100001/components/mus/input_d... components/mus/input_devices/input_device_server.h:31: void RegisterAsObserver(); On 2016/06/08 16:31:09, sadrul wrote: > This doesn't seem to be called from anywhere yet? Rebase problem. It's needed. https://codereview.chromium.org/1992443002/diff/100001/components/mus/input_d... components/mus/input_devices/input_device_server.h:39: void Create(shell::Connection* connection, On 2016/06/08 16:31:09, sadrul wrote: > This could be private too I think? Done. https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... File components/mus/public/cpp/input_devices/input_device_client.cc (right): https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.cc:25: DLOG(ERROR) << "InputDeviceClient failed to bind observer pipe."; On 2016/06/08 16:31:09, sadrul wrote: > Note: the failure can happen after bind succeeds. What's the idiomatic way to log connection errors then? Can we drop the error handler on successful connection or..? https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... File components/mus/public/cpp/input_devices/input_device_client.h (right): https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.h:43: // ui::InputDeviceManager: On 2016/06/08 16:31:09, sadrul wrote: > Make these privates too? They seem more appropriate as public. The class exists to provide those public methods. It doesn't make a difference, since they're public on the interface but this signals they are the important part of this particular class? Thoughts? https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... File components/mus/public/interfaces/input_devices/input_devices.mojom (right): https://codereview.chromium.org/1992443002/diff/100001/components/mus/public/... components/mus/public/interfaces/input_devices/input_devices.mojom:21: OnDeviceListsComplete(); On 2016/06/08 16:31:09, sadrul wrote: > OK. So, it sounds like once the observer registers itself, it's can get upto 5 > IPC messages (4 for the device-type lists, and then this > OnDeviceListsComplete()). Can these be merged into one, e.g. can OnDeviceLists() > include the various lists? Yeah, that's definitely possible. Not sure it will make a difference in practice but I've made it such that OnDeviceListsComplete() includes all of the lists and no updates are sent before it. https://codereview.chromium.org/1992443002/diff/100001/mojo/public/tools/bind... File mojo/public/tools/bindings/chromium_bindings_configuration.gni (right): https://codereview.chromium.org/1992443002/diff/100001/mojo/public/tools/bind... mojo/public/tools/bindings/chromium_bindings_configuration.gni:14: "//ui/events/devices/mojo/typemaps.gni", On 2016/06/08 16:31:09, sadrul wrote: > sort Done. https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... File ui/events/devices/mojo/OWNERS (right): https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... ui/events/devices/mojo/OWNERS:1: kylechar@chromium.org On 2016/06/08 16:31:09, sadrul wrote: > security folks should also be mojo OWNERS. I think you can copy over one of the > existing owner files (e.g. ui/gfx/mojo/OWNERS) Done. https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... File ui/events/devices/mojo/touchscreen_device_struct_traits.h (right): https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... ui/events/devices/mojo/touchscreen_device_struct_traits.h:24: static gfx::Size size(const ui::TouchscreenDevice& device) { On 2016/06/08 16:31:09, sadrul wrote: > const & Done. https://codereview.chromium.org/1992443002/diff/100001/ui/events/devices/mojo... ui/events/devices/mojo/touchscreen_device_struct_traits.h:45: On 2016/06/08 16:31:09, sadrul wrote: > This could maybe go in the input_device_struct_traits.h too? (same for .typemap) Sure, done.
I just noticed I need to update some more comments based on the latest changes, will make sure to go over them for the next patch.
Description was changed from ========== Introduce Mojo IPC based input-device service. Add Mojo IPC interfaces and structs necessary to pass information about input devices between processes. InputDeviceServer listens for updates about input devices and holds a list of Mojo observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. BUG=601981 ========== to ========== Add Mojo IPC based input-device service. Add Mojo interfaces to pass information about input-devices between processes. InputDeviceServer listens for updates about input-devices from DeviceDataManager and holds a list of out of process observers. It forwards updates via Mojo IPC to observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. It receives updates about input-devices from InputDeviceServer, caches the information and notifies in process observers. InputDeviceClient will replace the stub DeviceDataManager instance that is created in ash and chrome. BUG=601981 ==========
Description was changed from ========== Add Mojo IPC based input-device service. Add Mojo interfaces to pass information about input-devices between processes. InputDeviceServer listens for updates about input-devices from DeviceDataManager and holds a list of out of process observers. It forwards updates via Mojo IPC to observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. It receives updates about input-devices from InputDeviceServer, caches the information and notifies in process observers. InputDeviceClient will replace the stub DeviceDataManager instance that is created in ash and chrome. BUG=601981 ========== to ========== Add Mojo IPC based input-device service. Add Mojo interfaces to pass information about input-devices between processes. InputDeviceServer listens for updates about input-devices from DeviceDataManager and holds a list of out of process observers. It forwards updates via Mojo IPC to observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. It receives updates about input-devices from InputDeviceServer, caches the information and notifies in process observers. InputDeviceClient will replace the stub DeviceDataManager instance that is created in ash and chrome. Code is unused at this point, will be activated in a followup CL. BUG=601981 ==========
PTAL. I've rebased and simplified the CL. There is a followup CL that actually enables this code at crrev.com/2058853002
Description was changed from ========== Add Mojo IPC based input-device service. Add Mojo interfaces to pass information about input-devices between processes. InputDeviceServer listens for updates about input-devices from DeviceDataManager and holds a list of out of process observers. It forwards updates via Mojo IPC to observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. It receives updates about input-devices from InputDeviceServer, caches the information and notifies in process observers. InputDeviceClient will replace the stub DeviceDataManager instance that is created in ash and chrome. Code is unused at this point, will be activated in a followup CL. BUG=601981 ========== to ========== Add Mojo IPC based input-device service. Add Mojo interfaces to pass information about input-devices between processes. InputDeviceServer listens for updates about input-devices from DeviceDataManager and holds a list of out of process observers. It forwards updates via Mojo IPC to observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. It receives updates about input-devices from InputDeviceServer, caches the information and notifies in process observers. InputDeviceClient will replace the stub DeviceDataManager instance that is created in ash and chrome. Code is unused at this point, will be activated in a followup CL. BUG=601981 ==========
nits. lgtm https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... File components/mus/input_devices/input_device_server.cc (right): https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... components/mus/input_devices/input_device_server.cc:37: void InputDeviceServer::Create(shell::Connection* connection, This should be at the end (order in .h should match order in .cc) https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... components/mus/input_devices/input_device_server.cc:52: if (manager_->AreDeviceListsComplete()) { Early return if not-complete instead in these functions. https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... components/mus/input_devices/input_device_server.cc:106: mojo::Array<ui::InputDevice>::From(keyboard_devices), Do you need to explicitly do this? i.e. are struct-traits not smart enough to take care of this? https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... File components/mus/input_devices/input_device_server.h (right): https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... components/mus/input_devices/input_device_server.h:54: void SendDeviceListsComplete(mojom::InputDeviceObserverMojo* observer); non-overrides before overrides https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... File components/mus/public/cpp/input_devices/input_device_client.cc (right): https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.cc:22: // Add this class as an observer via Mojo IPC. The comment isn't necessary https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.cc:55: mojo::Array<ui::InputDevice> keyboard_devices, It may be nice to have struct-traits take care of this so we can use std::vector<> here. https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... File components/mus/public/cpp/input_devices/input_device_client.h (right): https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.h:29: // needs input-device information but doesn't have DeviceDataManager. The Mojo each *client But I don't think this line is adding anything over what's already explained above.
Patchset #10 (id:180001) has been deleted
Fixed nits. Can you take another look for OWNERS approval ben@? https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... File components/mus/input_devices/input_device_server.cc (right): https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... components/mus/input_devices/input_device_server.cc:37: void InputDeviceServer::Create(shell::Connection* connection, On 2016/06/10 18:01:49, sadrul wrote: > This should be at the end (order in .h should match order in .cc) Done. https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... components/mus/input_devices/input_device_server.cc:52: if (manager_->AreDeviceListsComplete()) { On 2016/06/10 18:01:49, sadrul wrote: > Early return if not-complete instead in these functions. Done. https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... components/mus/input_devices/input_device_server.cc:106: mojo::Array<ui::InputDevice>::From(keyboard_devices), On 2016/06/10 18:01:49, sadrul wrote: > Do you need to explicitly do this? i.e. are struct-traits not smart enough to > take care of this? Ah they are smart enough. Fixed here and above. https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... File components/mus/input_devices/input_device_server.h (right): https://codereview.chromium.org/1992443002/diff/160001/components/mus/input_d... components/mus/input_devices/input_device_server.h:54: void SendDeviceListsComplete(mojom::InputDeviceObserverMojo* observer); On 2016/06/10 18:01:49, sadrul wrote: > non-overrides before overrides Done. https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... File components/mus/public/cpp/input_devices/input_device_client.cc (right): https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.cc:22: // Add this class as an observer via Mojo IPC. On 2016/06/10 18:01:49, sadrul wrote: > The comment isn't necessary Done. https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.cc:55: mojo::Array<ui::InputDevice> keyboard_devices, On 2016/06/10 18:01:49, sadrul wrote: > It may be nice to have struct-traits take care of this so we can use > std::vector<> here. That would be ideal but I don't think Mojo supports that yet? I can't find any examples at least. https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... File components/mus/public/cpp/input_devices/input_device_client.h (right): https://codereview.chromium.org/1992443002/diff/160001/components/mus/public/... components/mus/public/cpp/input_devices/input_device_client.h:29: // needs input-device information but doesn't have DeviceDataManager. The Mojo On 2016/06/10 18:01:49, sadrul wrote: > each *client > > But I don't think this line is adding anything over what's already explained > above. Deleted that part of the comment.
lgtm
The CQ bit was checked by kylechar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, ben@chromium.org Link to the patchset: https://codereview.chromium.org/1992443002/#ps220001 (title: "Change namespace.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1992443002/220001
Message was sent while issue was closed.
Description was changed from ========== Add Mojo IPC based input-device service. Add Mojo interfaces to pass information about input-devices between processes. InputDeviceServer listens for updates about input-devices from DeviceDataManager and holds a list of out of process observers. It forwards updates via Mojo IPC to observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. It receives updates about input-devices from InputDeviceServer, caches the information and notifies in process observers. InputDeviceClient will replace the stub DeviceDataManager instance that is created in ash and chrome. Code is unused at this point, will be activated in a followup CL. BUG=601981 ========== to ========== Add Mojo IPC based input-device service. Add Mojo interfaces to pass information about input-devices between processes. InputDeviceServer listens for updates about input-devices from DeviceDataManager and holds a list of out of process observers. It forwards updates via Mojo IPC to observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. It receives updates about input-devices from InputDeviceServer, caches the information and notifies in process observers. InputDeviceClient will replace the stub DeviceDataManager instance that is created in ash and chrome. Code is unused at this point, will be activated in a followup CL. BUG=601981 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Add Mojo IPC based input-device service. Add Mojo interfaces to pass information about input-devices between processes. InputDeviceServer listens for updates about input-devices from DeviceDataManager and holds a list of out of process observers. It forwards updates via Mojo IPC to observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. It receives updates about input-devices from InputDeviceServer, caches the information and notifies in process observers. InputDeviceClient will replace the stub DeviceDataManager instance that is created in ash and chrome. Code is unused at this point, will be activated in a followup CL. BUG=601981 ========== to ========== Add Mojo IPC based input-device service. Add Mojo interfaces to pass information about input-devices between processes. InputDeviceServer listens for updates about input-devices from DeviceDataManager and holds a list of out of process observers. It forwards updates via Mojo IPC to observers. InputDeviceClient registers as a Mojo observer with InputDeviceServer and holds a list of in process observers. It receives updates about input-devices from InputDeviceServer, caches the information and notifies in process observers. InputDeviceClient will replace the stub DeviceDataManager instance that is created in ash and chrome. Code is unused at this point, will be activated in a followup CL. BUG=601981 Committed: https://crrev.com/edbac87b7a0fbe93a323840e2f143686e4065b10 Cr-Commit-Position: refs/heads/master@{#399986} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/edbac87b7a0fbe93a323840e2f143686e4065b10 Cr-Commit-Position: refs/heads/master@{#399986} |
