Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522843002:
Convert Gamepad IPC messages into mojo interface. (Closed)
|
Created:
4 years, 1 month ago by ke.he Modified:
4 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, nasko+codewatch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert Gamepad IPC messages into mojo interface.
This patch is a subtask of "Decoupling Gamepad from //content".
The Gamepad IPC messages are converted into mojo interfaces, so the
GamepadBrowserMessageFilter doesn't need to extend BrowserMessageFilter anymore
so we renamed it as GamepadMonitor.
GamepadMonitor still extend GamepadConsumer interface. Each RenderProcessHost
owns one GamepadMonitor. GamepadMonitor instance is created by "StrongBinding"
and will be registered into the "ConsumerSet" of GamepadService.
The base::SharedMemory handle are wrapped into mojo::SharedMemoryHandle and
passed as parameter of mojo interface from Browser to Renderer, Renderer Process
unwrap it to get the real shared memory contents.
Note that to obtain a new SharedMemoryHandle to wrap in a Mojo handle we simply
clone the existing one. While it's not obvious, this is functionally equivalent
to the old approach of passing the peer handle: when the renderer unwraps the
Mojo handle that it obtained into a base::SharedMemoryHandle, it fills in that
handle with its own process ID.
BUG=612330
Committed: https://crrev.com/0bb26d507b0a3ee05b01784841b850e22e58eec5
Cr-Commit-Position: refs/heads/master@{#437477}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Convert GamepadMsg_GamepadConnected/Disconnected msgs to mojo interface. #Patch Set 3 : remove IPC messages #Patch Set 4 : trybot strangely failed, code rebase. #
Total comments: 2
Patch Set 5 : Convert Gamepad IPC messages into mojo interface. #Patch Set 6 : remove useless Fuzztraits<blink::WebGamepad> #
Total comments: 7
Patch Set 7 : add GetSharedMemoryHandle() in GamepadService. #
Total comments: 6
Patch Set 8 : hide the "sharedmemory wrap" in GamepadService. #
Total comments: 1
Patch Set 9 : Convert Gamepad IPC messages into mojo interface. #
Total comments: 18
Patch Set 10 : Convert Gamepad IPC messages into mojo interface. #Patch Set 11 : Convert Gamepad IPC messages into mojo interface. #Patch Set 12 : code rebase #Messages
Total messages: 121 (77 generated)
ke.he@intel.com changed reviewers: + leon.han@intel.com
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ke.he@intel.com changed reviewers: + blundell@chromium.org
blundell@, PTAL. Thanks!
https://codereview.chromium.org/2522843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/gamepad_browser_message_filter.cc (right): https://codereview.chromium.org/2522843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gamepad_browser_message_filter.cc:39: interface_provider_->GetInterface(mojo::GetProxy(&gamepad_observer_)); Hi, blundell@, we have a concern: here |interface_provider_| is being accessed on IO thread, is it correct? I think this |interface_provider_| should only be accessed on its bound thread(UI thread). But the strange thing is that we've confirmed locally that these codes run well... Any idea about this? Thanks.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Could you also remove the IPC messages that you're transitioning away from and make sure the bots are green on that? Then I'll do a full review.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/23 12:13:13, blundell wrote: > Thanks! Could you also remove the IPC messages that you're transitioning away > from and make sure the bots are green on that? Then I'll do a full review. blundell@, IPC messages removed, PTAL. Thanks!
Thanks! https://codereview.chromium.org/2522843002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gamepad_browser_message_filter.cc (right): https://codereview.chromium.org/2522843002/diff/80001/content/browser/rendere... content/browser/renderer_host/gamepad_browser_message_filter.cc:39: interface_provider_->GetInterface(mojo::GetProxy(&gamepad_observer_)); You're planning on following up by converting the renderer->browser messages to Mojo as well, correct? (e.g., StartPolling()). Had you looked yet at how to replace the call to PeerHandle() that's made in line 64 below? I think that we should start there, which will involve creating a GamepadManager interface (note that offhand I'm not exactly sure what the best way is to manage that transition, which might mean we need to have a design discussion first). Once we've figured out how that's going to work (e.g., can GamepadService implement it directly and multiplex the incoming connections, or will we will still need two separate classes?), it will be easy to return to this CL and change it to just add a SetObserver() (or possibly AddObserver()) method to the GamepadManager interface rather than exposing an interface from the renderer to the browser. Does that make sense?
https://codereview.chromium.org/2522843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/gamepad_browser_message_filter.cc (right): https://codereview.chromium.org/2522843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gamepad_browser_message_filter.cc:39: interface_provider_->GetInterface(mojo::GetProxy(&gamepad_observer_)); On 2016/11/22 09:43:44, leonhsl wrote: > Hi, blundell@, we have a concern: here |interface_provider_| is being accessed > on IO thread, is it correct? I think this |interface_provider_| should only be > accessed on its bound thread(UI thread). > But the strange thing is that we've confirmed locally that these codes run > well... Any idea about this? Thanks. This is a good question, and I had to dig into the code to figure out why this is working. The magic is here: https://cs.chromium.org/chromium/src/content/common/service_manager/child_con... Basically, the InterfaceProvider that RenderProcessHostImpl uses forwards all calls to GetInterface() to get processed on the IO thread, regardless of which thread they come in on.
Description was changed from ========== Convert GamepadMsg_GamepadConnected/Disconnected msgs to mojo interface. This patch is a subtask of "Decoupling Gamepad from //content". In next patch we'll continue convert the "GamepadHostMsg_StartPolling" messages into mojo interface. BUG=612330 ========== to ========== Convert Gamepad IPC messages into mojo interface. This patch is a subtask of "Decoupling Gamepad from //content". The Gamepad IPC messages are converted into mojo interfaces, so the GamepadBrowserMessageFilter doesn't need to extend BrowserMessageFilter anymore so we renamed it as GamepadMonitor. GamepadMonitor still extend GamepadConsumer interface. Each RenderProcessHost owns one GamepadMonitor. GamepadMonitor instance is created by "StrongBinding" and will be registered into the "ConsumerSet" of GamepadService. The base::SharedMemory handle are wrapped into mojo::SharedMemoryHandle and passed as parameter of mojo interface from Browser to Renderer, Renderer Process unwrap it to get the real shared memory contents. In next patch we'll let GamepadService directly implement the GamepadMonitor interface(will rename it as GamepadManager) and multiplex incoming connections. BUG=612330 ==========
https://codereview.chromium.org/2522843002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gamepad_browser_message_filter.cc (right): https://codereview.chromium.org/2522843002/diff/80001/content/browser/rendere... content/browser/renderer_host/gamepad_browser_message_filter.cc:39: interface_provider_->GetInterface(mojo::GetProxy(&gamepad_observer_)); On 2016/11/28 14:45:34, blundell wrote: > You're planning on following up by converting the renderer->browser messages to > Mojo as well, correct? (e.g., StartPolling()). Had you looked yet at how to > replace the call to PeerHandle() that's made in line 64 below? I think that we > should start there, which will involve creating a GamepadManager interface (note > that offhand I'm not exactly sure what the best way is to manage that > transition, which might mean we need to have a design discussion first). Once > we've figured out how that's going to work (e.g., can GamepadService implement > it directly and multiplex the incoming connections, or will we will still need > two separate classes?), it will be easy to return to this CL and change it to > just add a SetObserver() (or possibly AddObserver()) method to the > GamepadManager interface rather than exposing an interface from the renderer to > the browser. > > Does that make sense? blundell@, Thanks very much! Yes, it makes sense. This patch is to convert "browser->renderer" messages and I have a another patch to convert "renderer->browser" messages. Now I noticed It is my mistake to split them into 2 CLs, and I should explain more on the CL description too. Really Sorry for this. I have updated my CL(merged the two CLs into one) You mentioned there are 2 possible choices: choice(1)"GamepadService implement it directly and multiplex the incoming connections". Or choice(2)"Still use two separate classes". I found my current implementation(updated CL) is in the latter way. From your suggestion I just noticed the choice(1) makes sense too. I guess I understand what you said about choice(1). The original implementation is GamepadService owns a set of GamepadConsumer. While in choice(1) the GamepadService will own a mojo gamepad-observer-ptr set instead. Render process will call AddObserver() through mojo to register itself into GameService. I think maybe in this patch we can just keep focus on the straight forword IPC-to-mojo conversion work. In next patch I'll focus on implementing choice(1). Do you think it's OK? just to avoid the patch too complex. Thanks again! CL updated. PTAL.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Change the title and description.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! I didn't have time to do a full review, but I did look at the PeerHandle() transformation, which for me is the most crucial aspect right now. I agree that we can isolate the transformation away from Chrome IPC first and then see if it would be better to fold the host-side mojom impl into GamepadService, especially because the usage of PeerHandle() means that this transformation isn't totally straightforward. https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... File content/browser/renderer_host/gamepad_monitor.cc (right): https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:52: service->GetSharedMemoryHandleForProcess(base::GetCurrentProcessHandle()); I don't think that this is the same as what the code was doing before, was it? This is returning the handle for *this process* (i.e., the browser process), whereas BrowserMessageFilter::PeerHandle() is returning a handle for the renderer process associated with that message filter. Unless I'm misunderstanding how one or both of those methods work?
https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... File content/browser/renderer_host/gamepad_monitor.cc (right): https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:52: service->GetSharedMemoryHandleForProcess(base::GetCurrentProcessHandle()); On 2016/11/29 15:36:24, blundell wrote: > I don't think that this is the same as what the code was doing before, was it? > This is returning the handle for *this process* (i.e., the browser process), > whereas BrowserMessageFilter::PeerHandle() is returning a handle for the > renderer process associated with that message filter. Unless I'm > misunderstanding how one or both of those methods work? The logic is here on purpose. In the original base::sharedMemory mechanism, we need to set the PeerHandle to specify the target process we want to share memory with. But in mojo's sharedMemory mechanism, we don't need to specify the target process, because the shared memory handle is passed through mojo channel, mojo already knows who is the right target process. The most straight forward way here is to add a new memory function "GetSharedMemoryHandle()" in GameService, the "GetSharedMemoryHandle()" is similar to "GetSharedMemoryHandleForProcess(peerHandle)" but the former doesn't set PeerHandle. Then we wrap the base::sharedMemoryHandle into a mojo::ScopedSharedMemoryHandle, and pass it to render process by mojo call. But I didn't add a new "GetSharedMemoryHandle()" since we found if we pass "current process handle" to GetSharedMemoryHandleForProcess(), the effect is almost the same. I feel it make sense for GetSharedMemoryHandleForProcess() that "set current process as peerHandle" means "doesn't specify the target process". What do you think about this? Thanks very much.
On 2016/11/30 05:45:11, ke.he wrote: > https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... > File content/browser/renderer_host/gamepad_monitor.cc (right): > > https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... > content/browser/renderer_host/gamepad_monitor.cc:52: > service->GetSharedMemoryHandleForProcess(base::GetCurrentProcessHandle()); > On 2016/11/29 15:36:24, blundell wrote: > > I don't think that this is the same as what the code was doing before, was it? > > This is returning the handle for *this process* (i.e., the browser process), > > whereas BrowserMessageFilter::PeerHandle() is returning a handle for the > > renderer process associated with that message filter. Unless I'm > > misunderstanding how one or both of those methods work? > > The logic is here on purpose. > In the original base::sharedMemory mechanism, we need to set the PeerHandle to > specify the target process we want to share memory with. But in mojo's > sharedMemory mechanism, we don't need to specify the target process, because the > shared memory handle is passed through mojo channel, mojo already knows who is > the right target process. > The most straight forward way here is to add a new memory function > "GetSharedMemoryHandle()" in GameService, the "GetSharedMemoryHandle()" is > similar to "GetSharedMemoryHandleForProcess(peerHandle)" but the former doesn't > set PeerHandle. Then we wrap the base::sharedMemoryHandle into a > mojo::ScopedSharedMemoryHandle, and pass it to render process by mojo call. > But I didn't add a new "GetSharedMemoryHandle()" since we found if we pass > "current process handle" to GetSharedMemoryHandleForProcess(), the effect is > almost the same. I feel it make sense for GetSharedMemoryHandleForProcess() that > "set current process as peerHandle" means "doesn't specify the target process". > What do you think about this? Thanks very much. This is useful, thanks! I unfortunately likely won't be able to get to this today but I'll review it tomorrow.
On 2016/11/29 06:40:06, ke.he wrote: > https://codereview.chromium.org/2522843002/diff/80001/content/browser/rendere... > File content/browser/renderer_host/gamepad_browser_message_filter.cc (right): > > https://codereview.chromium.org/2522843002/diff/80001/content/browser/rendere... > content/browser/renderer_host/gamepad_browser_message_filter.cc:39: > interface_provider_->GetInterface(mojo::GetProxy(&gamepad_observer_)); > On 2016/11/28 14:45:34, blundell wrote: > > You're planning on following up by converting the renderer->browser messages > to > > Mojo as well, correct? (e.g., StartPolling()). Had you looked yet at how to > > replace the call to PeerHandle() that's made in line 64 below? I think that we > > should start there, which will involve creating a GamepadManager interface > (note > > that offhand I'm not exactly sure what the best way is to manage that > > transition, which might mean we need to have a design discussion first). Once > > we've figured out how that's going to work (e.g., can GamepadService implement > > it directly and multiplex the incoming connections, or will we will still need > > two separate classes?), it will be easy to return to this CL and change it to > > just add a SetObserver() (or possibly AddObserver()) method to the > > GamepadManager interface rather than exposing an interface from the renderer > to > > the browser. > > > > Does that make sense? > > blundell@, Thanks very much! > Yes, it makes sense. This patch is to convert "browser->renderer" messages > and I have a another patch to convert "renderer->browser" messages. Now I > noticed It is my mistake to split them into 2 CLs, and I should explain more on > the CL description too. Really Sorry for this. I have updated my CL(merged the > two CLs into one) > > You mentioned there are 2 possible choices: choice(1)"GamepadService > implement it directly and multiplex the incoming connections". Or > choice(2)"Still use two separate classes". I found my current > implementation(updated CL) is in the latter way. > > From your suggestion I just noticed the choice(1) makes sense too. I guess I > understand what you said about choice(1). The original implementation is > GamepadService owns a set of GamepadConsumer. While in choice(1) the > GamepadService will own a mojo gamepad-observer-ptr set instead. Render process > will call AddObserver() through mojo to register itself into GameService. > > I think maybe in this patch we can just keep focus on the straight forword > IPC-to-mojo conversion work. In next patch I'll focus on implementing choice(1). > Do you think it's OK? just to avoid the patch too complex. > > Thanks again! CL updated. PTAL. blundell@, I found it is Not good to implement the choice(1) by now. The reason is: GamepadService owns a "ConsumerSet" as |consumers_|, the Set member might be not only GamepadMonitor(renamed from GamepadBrowserMessageFilter), but also PepperGamepadHost. If we implement choice(1), we have to add another Set in GamepadService to hold the mojo-observer-ptr. Then GamepadService need to take care both Sets and find itself difficult to know the real status of its "consumers". See https://cs.chromium.org/chromium/src/content/browser/gamepad/gamepad_service.... you know what I mean. I think only if someone finished the mojoification of PepperGamepadHost we can start consider to fold the host-side mojom impl into GamepadService (by replacing the ConsumerSet completely with mojo-observer-ptr Set). Unfortunately the IPC messages in PepperGamepadHost are not straightforward. By the way, seems the Pepper will be deprecated?? So in current phase, I prefer the choice(2)"Still use two separate classes". What's your opinion about this? I'll change this CL's description if you think it is OK. Thanks!
Thanks! https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... File content/browser/renderer_host/gamepad_monitor.cc (right): https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:52: service->GetSharedMemoryHandleForProcess(base::GetCurrentProcessHandle()); On 2016/11/30 05:45:11, ke.he wrote: > On 2016/11/29 15:36:24, blundell wrote: > > I don't think that this is the same as what the code was doing before, was it? > > This is returning the handle for *this process* (i.e., the browser process), > > whereas BrowserMessageFilter::PeerHandle() is returning a handle for the > > renderer process associated with that message filter. Unless I'm > > misunderstanding how one or both of those methods work? > > The logic is here on purpose. > In the original base::sharedMemory mechanism, we need to set the PeerHandle to > specify the target process we want to share memory with. But in mojo's > sharedMemory mechanism, we don't need to specify the target process, because the > shared memory handle is passed through mojo channel, mojo already knows who is > the right target process. > The most straight forward way here is to add a new memory function > "GetSharedMemoryHandle()" in GameService, the "GetSharedMemoryHandle()" is > similar to "GetSharedMemoryHandleForProcess(peerHandle)" but the former doesn't > set PeerHandle. Then we wrap the base::sharedMemoryHandle into a > mojo::ScopedSharedMemoryHandle, and pass it to render process by mojo call. > But I didn't add a new "GetSharedMemoryHandle()" since we found if we pass > "current process handle" to GetSharedMemoryHandleForProcess(), the effect is > almost the same. I feel it make sense for GetSharedMemoryHandleForProcess() that > "set current process as peerHandle" means "doesn't specify the target process". > What do you think about this? Thanks very much. > OK, this makes sense, thanks. The key is that mojo::UnwrapSharedMemoryHandle() creates a new PlatformHandle with the target process proc ID (this is what you were getting at in your above explanation). To make the fact that we don't care about the target process more explicit, I would do the following: - Change the naming to GamepadService::DuplicateHandle() (and similarly for the other methods with the same name) - Change the impl to call shared_memory()->DuplicateHandle(shared_memory()->handle()). This should have the same effect while avoiding the oddness of looking like we're asking to share to the browser process :), instead being explicit that we only care about giving the client a handle to the shared memory. I would also file a TODO() and create a bug for moving GamepadSharedBufferImpl to using a Mojo buffer rather than base::SharedMemory. Then we could hide all of this manipulation entirely. https://codereview.chromium.org/2522843002/diff/120001/device/gamepad/public/... File device/gamepad/public/interfaces/gamepad.mojom (right): https://codereview.chromium.org/2522843002/diff/120001/device/gamepad/public/... device/gamepad/public/interfaces/gamepad.mojom:65: GamepadStopPolling(); Did you reason about whether this one should be sync as well (as it was in the old impl)?
https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... File content/browser/renderer_host/gamepad_monitor.cc (right): https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:52: service->GetSharedMemoryHandleForProcess(base::GetCurrentProcessHandle()); On 2016/12/01 16:01:26, blundell wrote: > On 2016/11/30 05:45:11, ke.he wrote: > > On 2016/11/29 15:36:24, blundell wrote: > > > I don't think that this is the same as what the code was doing before, was > it? > > > This is returning the handle for *this process* (i.e., the browser process), > > > whereas BrowserMessageFilter::PeerHandle() is returning a handle for the > > > renderer process associated with that message filter. Unless I'm > > > misunderstanding how one or both of those methods work? > > > > The logic is here on purpose. > > In the original base::sharedMemory mechanism, we need to set the PeerHandle to > > specify the target process we want to share memory with. But in mojo's > > sharedMemory mechanism, we don't need to specify the target process, because > the > > shared memory handle is passed through mojo channel, mojo already knows who is > > the right target process. > > The most straight forward way here is to add a new memory function > > "GetSharedMemoryHandle()" in GameService, the "GetSharedMemoryHandle()" is > > similar to "GetSharedMemoryHandleForProcess(peerHandle)" but the former > doesn't > > set PeerHandle. Then we wrap the base::sharedMemoryHandle into a > > mojo::ScopedSharedMemoryHandle, and pass it to render process by mojo call. > > But I didn't add a new "GetSharedMemoryHandle()" since we found if we pass > > "current process handle" to GetSharedMemoryHandleForProcess(), the effect is > > almost the same. I feel it make sense for GetSharedMemoryHandleForProcess() > that > > "set current process as peerHandle" means "doesn't specify the target > process". > > What do you think about this? Thanks very much. > > > > OK, this makes sense, thanks. The key is that mojo::UnwrapSharedMemoryHandle() > creates a new PlatformHandle with the target process proc ID (this is what you > were getting at in your above explanation). > > To make the fact that we don't care about the target process more explicit, I > would do the following: > > - Change the naming to GamepadService::DuplicateHandle() (and similarly for the > other methods with the same name) > - Change the impl to call > shared_memory()->DuplicateHandle(shared_memory()->handle()). > > This should have the same effect while avoiding the oddness of looking like > we're asking to share to the browser process :), instead being explicit that we > only care about giving the client a handle to the shared memory. > > I would also file a TODO() and create a bug for moving GamepadSharedBufferImpl > to using a Mojo buffer rather than base::SharedMemory. Then we could hide all of > this manipulation entirely. Sorry, the GamepadService method name should probably be something like GetSharedMemoryHandle() instead; DuplicateHandle() is too low-level of a name. This is more-or-less what you were suggesting as an option I think. I think it's the way to go.
blundell@, CL updated. PTAL. Big Thanks:) https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... File content/browser/renderer_host/gamepad_monitor.cc (right): https://codereview.chromium.org/2522843002/diff/120001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:52: service->GetSharedMemoryHandleForProcess(base::GetCurrentProcessHandle()); On 2016/12/01 16:04:39, blundell wrote: > On 2016/12/01 16:01:26, blundell wrote: > > On 2016/11/30 05:45:11, ke.he wrote: > > > On 2016/11/29 15:36:24, blundell wrote: > > > > I don't think that this is the same as what the code was doing before, was > > it? > > > > This is returning the handle for *this process* (i.e., the browser > process), > > > > whereas BrowserMessageFilter::PeerHandle() is returning a handle for the > > > > renderer process associated with that message filter. Unless I'm > > > > misunderstanding how one or both of those methods work? > > > > > > The logic is here on purpose. > > > In the original base::sharedMemory mechanism, we need to set the PeerHandle > to > > > specify the target process we want to share memory with. But in mojo's > > > sharedMemory mechanism, we don't need to specify the target process, because > > the > > > shared memory handle is passed through mojo channel, mojo already knows who > is > > > the right target process. > > > The most straight forward way here is to add a new memory function > > > "GetSharedMemoryHandle()" in GameService, the "GetSharedMemoryHandle()" is > > > similar to "GetSharedMemoryHandleForProcess(peerHandle)" but the former > > doesn't > > > set PeerHandle. Then we wrap the base::sharedMemoryHandle into a > > > mojo::ScopedSharedMemoryHandle, and pass it to render process by mojo call. > > > But I didn't add a new "GetSharedMemoryHandle()" since we found if we pass > > > "current process handle" to GetSharedMemoryHandleForProcess(), the effect is > > > almost the same. I feel it make sense for GetSharedMemoryHandleForProcess() > > that > > > "set current process as peerHandle" means "doesn't specify the target > > process". > > > What do you think about this? Thanks very much. > > > > > > > OK, this makes sense, thanks. The key is that mojo::UnwrapSharedMemoryHandle() > > creates a new PlatformHandle with the target process proc ID (this is what you > > were getting at in your above explanation). > > > > To make the fact that we don't care about the target process more explicit, I > > would do the following: > > > > - Change the naming to GamepadService::DuplicateHandle() (and similarly for > the > > other methods with the same name) > > - Change the impl to call > > shared_memory()->DuplicateHandle(shared_memory()->handle()). > > > > This should have the same effect while avoiding the oddness of looking like > > we're asking to share to the browser process :), instead being explicit that > we > > only care about giving the client a handle to the shared memory. > > > > I would also file a TODO() and create a bug for moving GamepadSharedBufferImpl > > to using a Mojo buffer rather than base::SharedMemory. Then we could hide all > of > > this manipulation entirely. > > Sorry, the GamepadService method name should probably be something like > GetSharedMemoryHandle() instead; DuplicateHandle() is too low-level of a name. > This is more-or-less what you were suggesting as an option I think. I think it's > the way to go. blundell@, Thanks! 1) Add a new GetSharedMemoryHandle() alongside the GetSharedMemoryHandleForProcess(). --->done. 2) Create bug and add TODO() for "Using a Mojo buffer rather than base::SharedMemory". ---> I create an issue https://bugs.chromium.org/p/chromium/issues/detail?id=670655 for this. By the way, I don't have the permission of editing issues and trigger DryRun. May I apply? Thanks. https://codereview.chromium.org/2522843002/diff/120001/device/gamepad/public/... File device/gamepad/public/interfaces/gamepad.mojom (right): https://codereview.chromium.org/2522843002/diff/120001/device/gamepad/public/... device/gamepad/public/interfaces/gamepad.mojom:65: GamepadStopPolling(); On 2016/12/01 16:01:26, blundell wrote: > Did you reason about whether this one should be sync as well (as it was in the > old impl)? It should be Sync as it was in the old impl. Thanks! done.
Description was changed from ========== Convert Gamepad IPC messages into mojo interface. This patch is a subtask of "Decoupling Gamepad from //content". The Gamepad IPC messages are converted into mojo interfaces, so the GamepadBrowserMessageFilter doesn't need to extend BrowserMessageFilter anymore so we renamed it as GamepadMonitor. GamepadMonitor still extend GamepadConsumer interface. Each RenderProcessHost owns one GamepadMonitor. GamepadMonitor instance is created by "StrongBinding" and will be registered into the "ConsumerSet" of GamepadService. The base::SharedMemory handle are wrapped into mojo::SharedMemoryHandle and passed as parameter of mojo interface from Browser to Renderer, Renderer Process unwrap it to get the real shared memory contents. In next patch we'll let GamepadService directly implement the GamepadMonitor interface(will rename it as GamepadManager) and multiplex incoming connections. BUG=612330 ========== to ========== Convert Gamepad IPC messages into mojo interface. This patch is a subtask of "Decoupling Gamepad from //content". The Gamepad IPC messages are converted into mojo interfaces, so the GamepadBrowserMessageFilter doesn't need to extend BrowserMessageFilter anymore so we renamed it as GamepadMonitor. GamepadMonitor still extend GamepadConsumer interface. Each RenderProcessHost owns one GamepadMonitor. GamepadMonitor instance is created by "StrongBinding" and will be registered into the "ConsumerSet" of GamepadService. The base::SharedMemory handle are wrapped into mojo::SharedMemoryHandle and passed as parameter of mojo interface from Browser to Renderer, Renderer Process unwrap it to get the real shared memory contents. BUG=612330 ==========
On 2016/12/01 03:23:14, ke.he wrote: > On 2016/11/29 06:40:06, ke.he wrote: > > > https://codereview.chromium.org/2522843002/diff/80001/content/browser/rendere... > > File content/browser/renderer_host/gamepad_browser_message_filter.cc (right): > > > > > https://codereview.chromium.org/2522843002/diff/80001/content/browser/rendere... > > content/browser/renderer_host/gamepad_browser_message_filter.cc:39: > > interface_provider_->GetInterface(mojo::GetProxy(&gamepad_observer_)); > > On 2016/11/28 14:45:34, blundell wrote: > > > You're planning on following up by converting the renderer->browser messages > > to > > > Mojo as well, correct? (e.g., StartPolling()). Had you looked yet at how to > > > replace the call to PeerHandle() that's made in line 64 below? I think that > we > > > should start there, which will involve creating a GamepadManager interface > > (note > > > that offhand I'm not exactly sure what the best way is to manage that > > > transition, which might mean we need to have a design discussion first). > Once > > > we've figured out how that's going to work (e.g., can GamepadService > implement > > > it directly and multiplex the incoming connections, or will we will still > need > > > two separate classes?), it will be easy to return to this CL and change it > to > > > just add a SetObserver() (or possibly AddObserver()) method to the > > > GamepadManager interface rather than exposing an interface from the renderer > > to > > > the browser. > > > > > > Does that make sense? > > > > blundell@, Thanks very much! > > Yes, it makes sense. This patch is to convert "browser->renderer" messages > > and I have a another patch to convert "renderer->browser" messages. Now I > > noticed It is my mistake to split them into 2 CLs, and I should explain more > on > > the CL description too. Really Sorry for this. I have updated my CL(merged the > > two CLs into one) > > > > You mentioned there are 2 possible choices: choice(1)"GamepadService > > implement it directly and multiplex the incoming connections". Or > > choice(2)"Still use two separate classes". I found my current > > implementation(updated CL) is in the latter way. > > > > From your suggestion I just noticed the choice(1) makes sense too. I guess > I > > understand what you said about choice(1). The original implementation is > > GamepadService owns a set of GamepadConsumer. While in choice(1) the > > GamepadService will own a mojo gamepad-observer-ptr set instead. Render > process > > will call AddObserver() through mojo to register itself into GameService. > > > > I think maybe in this patch we can just keep focus on the straight forword > > IPC-to-mojo conversion work. In next patch I'll focus on implementing > choice(1). > > Do you think it's OK? just to avoid the patch too complex. > > > > Thanks again! CL updated. PTAL. > > blundell@, I found it is Not good to implement the choice(1) by now. The reason > is: > GamepadService owns a "ConsumerSet" as |consumers_|, the Set member might be not > only GamepadMonitor(renamed from GamepadBrowserMessageFilter), but also > PepperGamepadHost. > If we implement choice(1), we have to add another Set in GamepadService to hold > the mojo-observer-ptr. Then GamepadService need to take care both Sets and find > itself difficult to know the real status of its "consumers". See > https://cs.chromium.org/chromium/src/content/browser/gamepad/gamepad_service.... > you know what I mean. > I think only if someone finished the mojoification of PepperGamepadHost we can > start consider to fold the host-side mojom impl into GamepadService (by > replacing the ConsumerSet completely with mojo-observer-ptr Set). Unfortunately > the IPC messages in PepperGamepadHost are not straightforward. By the way, seems > the Pepper will be deprecated?? > So in current phase, I prefer the choice(2)"Still use two separate classes". > What's your opinion about this? I'll change this CL's description if you think > it is OK. Thanks! As above, I explained why it is not good time to fold the host-side mojom impl into GamepadService. what's your opinion about this? Just kindly make sure you didn't miss it:) Thanks very much!
This looks good, thanks! I agree with your reasoning on why it's not a good idea to have GamepadService be the GamepadMonitor impl until the Pepper client is converted (my understanding is that that is a goal, but a long-term goal). https://codereview.chromium.org/2522843002/diff/140001/content/browser/gamepa... File content/browser/gamepad/gamepad_service.h (right): https://codereview.chromium.org/2522843002/diff/140001/content/browser/gamepa... content/browser/gamepad/gamepad_service.h:78: base::SharedMemoryHandle GetSharedMemoryHandle(); Ah, I just thought of something even better: This method should just return the handle as a Mojo handle rather than a base::SharedMemoryHandle. So, GetSharedBufferHandle(). Then I would put the TODO and bug reference about moving everything to Mojo in the comment on this method. https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... File content/browser/renderer_host/gamepad_monitor.cc (right): https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:46: if (is_started_) You shouldn't handle a condition that you've DCHECK'd doesn't occur, so just eliminate lines 46-47 (I see this is just copied code, but while you're here...). https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:61: if (!is_started_) same comment as above.
On 2016/12/05 15:22:34, blundell wrote: > This looks good, thanks! I agree with your reasoning on why it's not a good idea > to have GamepadService be the GamepadMonitor impl until the Pepper client is > converted (my understanding is that that is a goal, but a long-term goal). > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/gamepa... > File content/browser/gamepad/gamepad_service.h (right): > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/gamepa... > content/browser/gamepad/gamepad_service.h:78: base::SharedMemoryHandle > GetSharedMemoryHandle(); > Ah, I just thought of something even better: This method should just return the > handle as a Mojo handle rather than a base::SharedMemoryHandle. So, > GetSharedBufferHandle(). Then I would put the TODO and bug reference about > moving everything to Mojo in the comment on this method. > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... > File content/browser/renderer_host/gamepad_monitor.cc (right): > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... > content/browser/renderer_host/gamepad_monitor.cc:46: if (is_started_) > You shouldn't handle a condition that you've DCHECK'd doesn't occur, so just > eliminate lines 46-47 (I see this is just copied code, but while you're > here...). > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... > content/browser/renderer_host/gamepad_monitor.cc:61: if (!is_started_) > same comment as above. I sent a request for edit-bugs access for you, will let you know when it goes through.
On 2016/12/05 15:24:17, blundell wrote: > On 2016/12/05 15:22:34, blundell wrote: > > This looks good, thanks! I agree with your reasoning on why it's not a good > idea > > to have GamepadService be the GamepadMonitor impl until the Pepper client is > > converted (my understanding is that that is a goal, but a long-term goal). > > > > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/gamepa... > > File content/browser/gamepad/gamepad_service.h (right): > > > > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/gamepa... > > content/browser/gamepad/gamepad_service.h:78: base::SharedMemoryHandle > > GetSharedMemoryHandle(); > > Ah, I just thought of something even better: This method should just return > the > > handle as a Mojo handle rather than a base::SharedMemoryHandle. So, > > GetSharedBufferHandle(). Then I would put the TODO and bug reference about > > moving everything to Mojo in the comment on this method. > > > > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... > > File content/browser/renderer_host/gamepad_monitor.cc (right): > > > > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... > > content/browser/renderer_host/gamepad_monitor.cc:46: if (is_started_) > > You shouldn't handle a condition that you've DCHECK'd doesn't occur, so just > > eliminate lines 46-47 (I see this is just copied code, but while you're > > here...). > > > > > https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... > > content/browser/renderer_host/gamepad_monitor.cc:61: if (!is_started_) > > same comment as above. > > I sent a request for edit-bugs access for you, will let you know when it goes > through. Hi, blundell@, would you please also help to apply trybot dryrun access permission for He Ke? So he can work easier without my help to run it. Thanks:)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:180001) has been deleted
Hi, blundell@, CL updated. PTAL. Thanks~ https://codereview.chromium.org/2522843002/diff/140001/content/browser/gamepa... File content/browser/gamepad/gamepad_service.h (right): https://codereview.chromium.org/2522843002/diff/140001/content/browser/gamepa... content/browser/gamepad/gamepad_service.h:78: base::SharedMemoryHandle GetSharedMemoryHandle(); On 2016/12/05 15:22:34, blundell wrote: > Ah, I just thought of something even better: This method should just return the > handle as a Mojo handle rather than a base::SharedMemoryHandle. So, > GetSharedBufferHandle(). Then I would put the TODO and bug reference about > moving everything to Mojo in the comment on this method. Done. https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... File content/browser/renderer_host/gamepad_monitor.cc (right): https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:46: if (is_started_) On 2016/12/05 15:22:34, blundell wrote: > You shouldn't handle a condition that you've DCHECK'd doesn't occur, so just > eliminate lines 46-47 (I see this is just copied code, but while you're > here...). Done. https://codereview.chromium.org/2522843002/diff/140001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:61: if (!is_started_) On 2016/12/05 15:22:34, blundell wrote: > same comment as above. Done. https://codereview.chromium.org/2522843002/diff/160001/content/browser/gamepa... File content/browser/gamepad/gamepad_service.cc (right): https://codereview.chromium.org/2522843002/diff/160001/content/browser/gamepa... content/browser/gamepad/gamepad_service.cc:162: return mojo::WrapSharedMemoryHandle(provider_->GetSharedMemoryHandle(), We add the "mojo::WrapSharedMemoryHandle(...)" here instead of adding it in GamepadProvider because GamepadProvider is located in //device/ and cannot "#include content/common/gamepad_hardware_buffer.h"
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks! Ke He, filed request for you to have tryjobs access. You should now have editbugs access. https://codereview.chromium.org/2522843002/diff/200001/content/browser/gamepa... File content/browser/gamepad/gamepad_service.cc (right): https://codereview.chromium.org/2522843002/diff/200001/content/browser/gamepa... content/browser/gamepad/gamepad_service.cc:161: // TODO(heke): Use mojo::SharedBuffer rather than base::Sharedmemory in nit: base::SharedMemory https://codereview.chromium.org/2522843002/diff/200001/content/browser/gamepa... File content/browser/gamepad/gamepad_service.h (right): https://codereview.chromium.org/2522843002/diff/200001/content/browser/gamepa... content/browser/gamepad/gamepad_service.h:78: // Returns the mojo::ScopedSharedBuffer handle of the gamepad data. nit: s/Returns the/Returns a new https://codereview.chromium.org/2522843002/diff/200001/content/browser/render... File content/browser/renderer_host/gamepad_monitor.cc (right): https://codereview.chromium.org/2522843002/diff/200001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:43: GamepadService* service = GamepadService::GetInstance(); nit: I would reorganize this code as follows: DCHECK(!is_started_); is_started_ = true; GamepadService* service = ... ... https://codereview.chromium.org/2522843002/diff/200001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:55: is_started_ = false; nit: similar to above, I would put blank line after this line rather than before it. https://codereview.chromium.org/2522843002/diff/200001/content/renderer/gamep... File content/renderer/gamepad_shared_memory_reader.cc (right): https://codereview.chromium.org/2522843002/diff/200001/content/renderer/gamep... content/renderer/gamepad_shared_memory_reader.cc:35: MojoResult result = mojo::UnwrapSharedMemoryHandle( Can you add a TODO and bug for moving the renderer to just using the Mojo shared buffer directly? That we should be able to do independently of the browser-side change, right? https://codereview.chromium.org/2522843002/diff/200001/content/renderer/gamep... content/renderer/gamepad_shared_memory_reader.cc:38: DCHECK_EQ(MOJO_RESULT_OK, result); This should be CHECK_EQ to keep the intent of line 25 from the curcrent impl. https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/gamepad... File device/gamepad/gamepad_provider.h (right): https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/gamepad... device/gamepad/gamepad_provider.h:61: // Returns the shared memory handle of the gamepad data. Could you add a TODO and bug to follow up by moving gamepad_hardware_buffer.h into //device/gamepad and then change this method to talk Mojo instead of base::SharedMemoryHandle?
blundell@chromium.org changed reviewers: + bajones@chromium.org, tsepez@chromium.org
bajones@: Can you review the whole CL as a gamepad OWNER? tsepez@: Can you review //device/gamepad/public/interfaces and //tools/ipc_fuzzer? Is there any way to do fuzzing for Mojo interfaces?
Ke He, could you extend the CL description with why the change to how the SharedMemoryHandle is obtained is OK? Something like: "Note that to obtain a new SharedMemoryHandle to wrap in a Mojo handle we simply clone the existing one. While it's not obvious, this is functionally equivalent to the old approach of passing the peer handle: when the renderer unwraps the Mojo handle that it obtained into a base::SharedMemoryHandle, it fills in that handle with its own process ID."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with blundell@'s comments addressed. Thanks for taking on this work!
mojom/messages LGTM, please build target ipc_fuzzer_all locally as a double-check on your work. Thanks. https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/public/... File device/gamepad/public/interfaces/gamepad.mojom (right): https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/public/... device/gamepad/public/interfaces/gamepad.mojom:59: // handles that will hold the data from the hardware. See nit: handle that https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/public/... device/gamepad/public/interfaces/gamepad.mojom:61: // handled. The number of Starts should match the number of Stops. nit: or what happens if they don't? Does the browser poll forever? Do we clean up on child termination?
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2522843002/diff/200001/content/browser/gamepa... File content/browser/gamepad/gamepad_service.cc (right): https://codereview.chromium.org/2522843002/diff/200001/content/browser/gamepa... content/browser/gamepad/gamepad_service.cc:161: // TODO(heke): Use mojo::SharedBuffer rather than base::Sharedmemory in On 2016/12/06 12:26:50, blundell wrote: > nit: base::SharedMemory Done. https://codereview.chromium.org/2522843002/diff/200001/content/browser/gamepa... File content/browser/gamepad/gamepad_service.h (right): https://codereview.chromium.org/2522843002/diff/200001/content/browser/gamepa... content/browser/gamepad/gamepad_service.h:78: // Returns the mojo::ScopedSharedBuffer handle of the gamepad data. On 2016/12/06 12:26:50, blundell wrote: > nit: s/Returns the/Returns a new Done. https://codereview.chromium.org/2522843002/diff/200001/content/browser/render... File content/browser/renderer_host/gamepad_monitor.cc (right): https://codereview.chromium.org/2522843002/diff/200001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:43: GamepadService* service = GamepadService::GetInstance(); On 2016/12/06 12:26:50, blundell wrote: > nit: I would reorganize this code as follows: > > DCHECK(!is_started_); > is_started_ = true; > > GamepadService* service = ... > ... Done. https://codereview.chromium.org/2522843002/diff/200001/content/browser/render... content/browser/renderer_host/gamepad_monitor.cc:55: is_started_ = false; On 2016/12/06 12:26:50, blundell wrote: > nit: similar to above, I would put blank line after this line rather than before > it. Done. https://codereview.chromium.org/2522843002/diff/200001/content/renderer/gamep... File content/renderer/gamepad_shared_memory_reader.cc (right): https://codereview.chromium.org/2522843002/diff/200001/content/renderer/gamep... content/renderer/gamepad_shared_memory_reader.cc:35: MojoResult result = mojo::UnwrapSharedMemoryHandle( On 2016/12/06 12:26:50, blundell wrote: > Can you add a TODO and bug for moving the renderer to just using the Mojo shared > buffer directly? That we should be able to do independently of the browser-side > change, right? Yes. we can do it independently in GamepadSharedMemoryReader. I added comments in the bug 670655 and also add TODO() in code. I think I can start this part as soon as this CL got landed. https://codereview.chromium.org/2522843002/diff/200001/content/renderer/gamep... content/renderer/gamepad_shared_memory_reader.cc:38: DCHECK_EQ(MOJO_RESULT_OK, result); On 2016/12/06 12:26:50, blundell wrote: > This should be CHECK_EQ to keep the intent of line 25 from the curcrent impl. Done. https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/gamepad... File device/gamepad/gamepad_provider.h (right): https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/gamepad... device/gamepad/gamepad_provider.h:61: // Returns the shared memory handle of the gamepad data. On 2016/12/06 12:26:50, blundell wrote: > Could you add a TODO and bug to follow up by moving gamepad_hardware_buffer.h > into //device/gamepad and then change this method to talk Mojo instead of > base::SharedMemoryHandle? Done. https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/public/... File device/gamepad/public/interfaces/gamepad.mojom (right): https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/public/... device/gamepad/public/interfaces/gamepad.mojom:59: // handles that will hold the data from the hardware. See On 2016/12/06 17:53:40, Tom Sepez wrote: > nit: handle that Done. https://codereview.chromium.org/2522843002/diff/200001/device/gamepad/public/... device/gamepad/public/interfaces/gamepad.mojom:61: // handled. The number of Starts should match the number of Stops. On 2016/12/06 17:53:40, Tom Sepez wrote: > nit: or what happens if they don't? Does the browser poll forever? Do we clean > up on child termination? Hi, Tom Sepez, Thanks very much for considering this. The GamepadService owns a std::Set of consumers. The renderer does register/unregister consumer in GamepadService by calling StartPolling/StopPolling. If GamepadService finds the std::Set<consumers> are not empty, it will ask GamepadProvider to do Polling in the polling thread, to notify consumers if any updates. So if a renderer calls Starts, and doesn't call Stops, the polling thread will keep polling, it means that renderer wants to received updates before it calls Stops. We do cleanup on child termination. Each render process "StrongBinds" a GamepadMonitor(on behalf a consumer) instance in browser side, GamepadMonitor will un-register in its destructor to avoid cause unnecessary polling. And, mismatch of Starts and Stops may cause DCHECK() complains too. Actually mojoifiction didn't change the logic of the register/unregister and the polling. Comments here are copied from the removed "content/common/gamepad_messages.h".
Description was changed from ========== Convert Gamepad IPC messages into mojo interface. This patch is a subtask of "Decoupling Gamepad from //content". The Gamepad IPC messages are converted into mojo interfaces, so the GamepadBrowserMessageFilter doesn't need to extend BrowserMessageFilter anymore so we renamed it as GamepadMonitor. GamepadMonitor still extend GamepadConsumer interface. Each RenderProcessHost owns one GamepadMonitor. GamepadMonitor instance is created by "StrongBinding" and will be registered into the "ConsumerSet" of GamepadService. The base::SharedMemory handle are wrapped into mojo::SharedMemoryHandle and passed as parameter of mojo interface from Browser to Renderer, Renderer Process unwrap it to get the real shared memory contents. BUG=612330 ========== to ========== Convert Gamepad IPC messages into mojo interface. This patch is a subtask of "Decoupling Gamepad from //content". The Gamepad IPC messages are converted into mojo interfaces, so the GamepadBrowserMessageFilter doesn't need to extend BrowserMessageFilter anymore so we renamed it as GamepadMonitor. GamepadMonitor still extend GamepadConsumer interface. Each RenderProcessHost owns one GamepadMonitor. GamepadMonitor instance is created by "StrongBinding" and will be registered into the "ConsumerSet" of GamepadService. The base::SharedMemory handle are wrapped into mojo::SharedMemoryHandle and passed as parameter of mojo interface from Browser to Renderer, Renderer Process unwrap it to get the real shared memory contents. Note that to obtain a new SharedMemoryHandle to wrap in a Mojo handle we simply clone the existing one. While it's not obvious, this is functionally equivalent to the old approach of passing the peer handle: when the renderer unwraps the Mojo handle that it obtained into a base::SharedMemoryHandle, it fills in that handle with its own process ID. BUG=612330 ==========
On 2016/12/06 12:31:56, blundell wrote: > Ke He, could you extend the CL description with why the change to how the > SharedMemoryHandle is obtained is OK? Something like: > > "Note that to obtain a new SharedMemoryHandle to wrap in a Mojo handle we simply > clone the existing one. While it's not obvious, this is functionally equivalent > to the old approach of passing the peer handle: when the renderer unwraps the > Mojo handle that it obtained into a base::SharedMemoryHandle, it fills in that > handle with its own process ID." done.
The CQ bit was unchecked by ke.he@intel.com
The CQ bit was checked by ke.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, bajones@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2522843002/#ps220001 (title: "Convert Gamepad IPC messages into mojo interface.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by ke.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, bajones@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2522843002/#ps240001 (title: "Convert Gamepad IPC messages into mojo interface.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ke.he@intel.com changed reviewers: + jam@chromium.org
jam@, Could you please PTAL on this? Thanks very much.
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ke.he@intel.com
The CQ bit was checked by ke.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ke He, you should specify which files (or directories) you need John's review on (you should basically always do this, especially if you're adding someone to a CL where there have been a bunch of reviews already). Thanks!
On 2016/12/08 09:28:02, blundell wrote:
> Ke He, you should specify which files (or directories) you need John's review
on
> (you should basically always do this, especially if you're adding someone to a
> CL where there have been a bunch of reviews already). Thanks!
blundell@, Thanks for your kindly advice.
I found the presubmit check failed, the log shows as below. That's why I invite
jam@ to review.
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/browser/BUILD.gn
content/browser/renderer_host/gamepad_browser_message_filter.cc
content/browser/renderer_host/gamepad_browser_message_filter.h
content/browser/renderer_host/gamepad_monitor.cc
content/browser/renderer_host/gamepad_monitor.h
content/browser/renderer_host/render_process_host_impl.cc
content/common/BUILD.gn
content/common/content_message_generator.h
content/renderer/BUILD.gn
content/renderer/DEPS
content/renderer/gamepad_shared_memory_reader.cc
content/renderer/gamepad_shared_memory_reader.h
jam@, Could you PTAL on the changed files in //content? Thanks very much.
lgtm
The CQ bit was checked by ke.he@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for content/public/app/mojo/content_browser_manifest.json:
While running git apply --index -p1;
error: patch failed: content/public/app/mojo/content_browser_manifest.json:29
error: content/public/app/mojo/content_browser_manifest.json: patch does not
apply
Patch: content/public/app/mojo/content_browser_manifest.json
Index: content/public/app/mojo/content_browser_manifest.json
diff --git a/content/public/app/mojo/content_browser_manifest.json
b/content/public/app/mojo/content_browser_manifest.json
index
7de9d2802ba2b875a1ead7e0f32be900a38e2f68..c1f10eede2b588853b0704b615f5b53955cc0be6
100644
--- a/content/public/app/mojo/content_browser_manifest.json
+++ b/content/public/app/mojo/content_browser_manifest.json
@@ -29,6 +29,7 @@
"content::mojom::URLLoaderFactory",
"content::mojom::VideoCaptureHost",
"device::BatteryMonitor",
+ "device::mojom::GamepadMonitor",
"device::mojom::LightSensor",
"device::mojom::MotionSensor",
"device::mojom::OrientationSensor",
The CQ bit was checked by ke.he@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for content/public/app/mojo/content_browser_manifest.json:
While running git apply --index -p1;
error: patch failed: content/public/app/mojo/content_browser_manifest.json:29
error: content/public/app/mojo/content_browser_manifest.json: patch does not
apply
Patch: content/public/app/mojo/content_browser_manifest.json
Index: content/public/app/mojo/content_browser_manifest.json
diff --git a/content/public/app/mojo/content_browser_manifest.json
b/content/public/app/mojo/content_browser_manifest.json
index
7de9d2802ba2b875a1ead7e0f32be900a38e2f68..c1f10eede2b588853b0704b615f5b53955cc0be6
100644
--- a/content/public/app/mojo/content_browser_manifest.json
+++ b/content/public/app/mojo/content_browser_manifest.json
@@ -29,6 +29,7 @@
"content::mojom::URLLoaderFactory",
"content::mojom::VideoCaptureHost",
"device::BatteryMonitor",
+ "device::mojom::GamepadMonitor",
"device::mojom::LightSensor",
"device::mojom::MotionSensor",
"device::mojom::OrientationSensor",
The CQ bit was checked by ke.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, bajones@chromium.org, jam@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2522843002/#ps260001 (title: "code rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1481256236986060,
"parent_rev": "0f0ec69694109b49e8f5b7c83d3453e4354c4177", "commit_rev":
"7e38ac3d8acb884960d3665a29ff12157d35ce3d"}
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Convert Gamepad IPC messages into mojo interface. This patch is a subtask of "Decoupling Gamepad from //content". The Gamepad IPC messages are converted into mojo interfaces, so the GamepadBrowserMessageFilter doesn't need to extend BrowserMessageFilter anymore so we renamed it as GamepadMonitor. GamepadMonitor still extend GamepadConsumer interface. Each RenderProcessHost owns one GamepadMonitor. GamepadMonitor instance is created by "StrongBinding" and will be registered into the "ConsumerSet" of GamepadService. The base::SharedMemory handle are wrapped into mojo::SharedMemoryHandle and passed as parameter of mojo interface from Browser to Renderer, Renderer Process unwrap it to get the real shared memory contents. Note that to obtain a new SharedMemoryHandle to wrap in a Mojo handle we simply clone the existing one. While it's not obvious, this is functionally equivalent to the old approach of passing the peer handle: when the renderer unwraps the Mojo handle that it obtained into a base::SharedMemoryHandle, it fills in that handle with its own process ID. BUG=612330 ========== to ========== Convert Gamepad IPC messages into mojo interface. This patch is a subtask of "Decoupling Gamepad from //content". The Gamepad IPC messages are converted into mojo interfaces, so the GamepadBrowserMessageFilter doesn't need to extend BrowserMessageFilter anymore so we renamed it as GamepadMonitor. GamepadMonitor still extend GamepadConsumer interface. Each RenderProcessHost owns one GamepadMonitor. GamepadMonitor instance is created by "StrongBinding" and will be registered into the "ConsumerSet" of GamepadService. The base::SharedMemory handle are wrapped into mojo::SharedMemoryHandle and passed as parameter of mojo interface from Browser to Renderer, Renderer Process unwrap it to get the real shared memory contents. Note that to obtain a new SharedMemoryHandle to wrap in a Mojo handle we simply clone the existing one. While it's not obvious, this is functionally equivalent to the old approach of passing the peer handle: when the renderer unwraps the Mojo handle that it obtained into a base::SharedMemoryHandle, it fills in that handle with its own process ID. BUG=612330 Committed: https://crrev.com/0bb26d507b0a3ee05b01784841b850e22e58eec5 Cr-Commit-Position: refs/heads/master@{#437477} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/0bb26d507b0a3ee05b01784841b850e22e58eec5 Cr-Commit-Position: refs/heads/master@{#437477} |
