Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2136193003:
Move ChannelMojo bindings to a custom multiplex router (Closed)
|
Created:
4 years, 5 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 5 months ago Reviewers:
yzshen1 CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove ChannelMojo bindings to a custom multiplex router
Changes MojoBootstrap - the implementation of master endpoints for
IPC::ChannelMojo - to use custom message routing logic in lieu of
the Mojo public Binding and InterfacePtr primitives and the underlying
MultiplexRouter implementation.
This is part a series of CLs to support Channel-associated interfaces.
BUG=612500
Committed: https://crrev.com/02b8e188466c60a084959c554c0d598303b621c3
Cr-Commit-Position: refs/heads/master@{#405254}
Patch Set 1 : . #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : . #Patch Set 4 #
Total comments: 10
Patch Set 5 : . #
Total comments: 2
Patch Set 6 : . #Messages
Total messages: 26 (18 generated)
The CQ bit was checked by rockot@chromium.org 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
rockot@chromium.org changed reviewers: + yzshen@chromium.org
Please take a look! Note that most of the logic here closely duplicates the internals of MultiplexRouter. Unfortunately some of the necessary differences seem quite difficult to extract (to a delegate interface for example), and I suspect that would require more work than is worth doing. The implementation here covers all the necessary functionality for ChannelMojo today, which is just simple interface routing on the IO thread. I've stubbed out the pieces that will support off-thread dispatch and sync messages, but plan to implement those in future CLs. WDYT?
Description was changed from ========== Move ChannelMojo bindings to a custom multiplex router Changes MojoBootstrap - the implementation of master endpoints for IPC::ChannelMojo - to use custom message routing logic in lieu of the Mojo public Binding and InterfacePtr primtivies and the underlying MultiplexRouter implementation. This is part 1 of a series of CLs to support Channel-associated interfaces. BUG=612500 ========== to ========== Move ChannelMojo bindings to a custom multiplex router Changes MojoBootstrap - the implementation of master endpoints for IPC::ChannelMojo - to use custom message routing logic in lieu of the Mojo public Binding and InterfacePtr primtivies and the underlying MultiplexRouter implementation. This is part a series of CLs to support Channel-associated interfaces: 1. This CL 2. https://codereview.chromium.org/2137353002 3. TBD... 4. TBD... BUG=612500 ==========
Description was changed from ========== Move ChannelMojo bindings to a custom multiplex router Changes MojoBootstrap - the implementation of master endpoints for IPC::ChannelMojo - to use custom message routing logic in lieu of the Mojo public Binding and InterfacePtr primtivies and the underlying MultiplexRouter implementation. This is part a series of CLs to support Channel-associated interfaces: 1. This CL 2. https://codereview.chromium.org/2137353002 3. TBD... 4. TBD... BUG=612500 ========== to ========== Move ChannelMojo bindings to a custom multiplex router Changes MojoBootstrap - the implementation of master endpoints for IPC::ChannelMojo - to use custom message routing logic in lieu of the Mojo public Binding and InterfacePtr primitives and the underlying MultiplexRouter implementation. This is part a series of CLs to support Channel-associated interfaces: 1. This CL 2. https://codereview.chromium.org/2137353002 3. TBD... 4. TBD... BUG=612500 ==========
Only a few nits. Thanks! https://codereview.chromium.org/2136193003/diff/60001/ipc/ipc_mojo_bootstrap.cc File ipc/ipc_mojo_bootstrap.cc (right): https://codereview.chromium.org/2136193003/diff/60001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:246: // It's not legal to make sync calls from the master endpoint's thread, This method is used for the implementation side of sync methods. Is it possible that some code may want to handle an incoming sync call on the IO thread? In that case, there shouldn't be anyone trying to make a sync call from the IO thread, so there is no need to do anything to allow being woken up. But it shouldn't trigger DCHECK. WDYT? https://codereview.chromium.org/2136193003/diff/60001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:520: class BootstrapMasterProxy { optional: does it make sense to move this into it own files? https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap.cc File ipc/ipc_mojo_bootstrap.cc (right): https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:46: ChannelAssociatedGroupController(bool for_proxy, nit: I feel that |for_proxy| may not be very accurate: one end of a pipe may have both client and impl objects associated with it. Besides, it is easier to compare the code if it uses the same name as MultiplexRouter. https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:372: bool MarkClosedAndMaybeRemove(Endpoint* endpoint) { The return value is never used. https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:382: bool MarkPeerClosedAndMaybeRemove(Endpoint* endpoint) { ditto. https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:442: CHECK(false); Remove this CHECK? https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:499: Endpoint* endpoint = FindOrInsertEndpoint(id, nullptr); Is a locker missing here?
The CQ bit was checked by rockot@chromium.org 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/2136193003/diff/60001/ipc/ipc_mojo_bootstrap.cc File ipc/ipc_mojo_bootstrap.cc (right): https://codereview.chromium.org/2136193003/diff/60001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:246: // It's not legal to make sync calls from the master endpoint's thread, On 2016/07/13 at 16:46:45, yzshen1 wrote: > This method is used for the implementation side of sync methods. Is it possible that some code may want to handle an incoming sync call on the IO thread? In that case, there shouldn't be anyone trying to make a sync call from the IO thread, so there is no need to do anything to allow being woken up. But it shouldn't trigger DCHECK. > > WDYT? SGTM DCHECKs removed https://codereview.chromium.org/2136193003/diff/60001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:520: class BootstrapMasterProxy { On 2016/07/13 at 16:46:45, yzshen1 wrote: > optional: does it make sense to move this into it own files? I considered it, but this I like the idea of keeping all this stuff isolated for now. WDYT? https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap.cc File ipc/ipc_mojo_bootstrap.cc (right): https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:46: ChannelAssociatedGroupController(bool for_proxy, On 2016/07/13 at 16:46:45, yzshen1 wrote: > nit: I feel that |for_proxy| may not be very accurate: one end of a pipe may have both client and impl objects associated with it. Besides, it is easier to compare the code if it uses the same name as MultiplexRouter. done https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:372: bool MarkClosedAndMaybeRemove(Endpoint* endpoint) { On 2016/07/13 at 16:46:45, yzshen1 wrote: > The return value is never used. Oops, they were used at one point. Removed :) https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:382: bool MarkPeerClosedAndMaybeRemove(Endpoint* endpoint) { On 2016/07/13 at 16:46:45, yzshen1 wrote: > ditto. done https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:442: CHECK(false); On 2016/07/13 at 16:46:45, yzshen1 wrote: > Remove this CHECK? Done https://codereview.chromium.org/2136193003/diff/100001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:499: Endpoint* endpoint = FindOrInsertEndpoint(id, nullptr); On 2016/07/13 at 16:46:45, yzshen1 wrote: > Is a locker missing here? Yes! Thanks, fixed
LGTM with one more comment https://codereview.chromium.org/2136193003/diff/120001/ipc/ipc_mojo_bootstrap.cc File ipc/ipc_mojo_bootstrap.cc (right): https://codereview.chromium.org/2136193003/diff/120001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:295: MarkClosedAndMaybeRemove(endpoint); I think it should be MarkPeerClosed, right?
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
https://codereview.chromium.org/2136193003/diff/120001/ipc/ipc_mojo_bootstrap.cc File ipc/ipc_mojo_bootstrap.cc (right): https://codereview.chromium.org/2136193003/diff/120001/ipc/ipc_mojo_bootstrap... ipc/ipc_mojo_bootstrap.cc:295: MarkClosedAndMaybeRemove(endpoint); On 2016/07/13 at 17:53:27, yzshen1 wrote: > I think it should be MarkPeerClosed, right? Gah! Fixed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move ChannelMojo bindings to a custom multiplex router Changes MojoBootstrap - the implementation of master endpoints for IPC::ChannelMojo - to use custom message routing logic in lieu of the Mojo public Binding and InterfacePtr primitives and the underlying MultiplexRouter implementation. This is part a series of CLs to support Channel-associated interfaces: 1. This CL 2. https://codereview.chromium.org/2137353002 3. TBD... 4. TBD... BUG=612500 ========== to ========== Move ChannelMojo bindings to a custom multiplex router Changes MojoBootstrap - the implementation of master endpoints for IPC::ChannelMojo - to use custom message routing logic in lieu of the Mojo public Binding and InterfacePtr primitives and the underlying MultiplexRouter implementation. This is part a series of CLs to support Channel-associated interfaces. BUG=612500 ==========
The CQ bit was unchecked by rockot@chromium.org
The CQ bit was checked by rockot@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2136193003/#ps140001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Move ChannelMojo bindings to a custom multiplex router Changes MojoBootstrap - the implementation of master endpoints for IPC::ChannelMojo - to use custom message routing logic in lieu of the Mojo public Binding and InterfacePtr primitives and the underlying MultiplexRouter implementation. This is part a series of CLs to support Channel-associated interfaces. BUG=612500 ========== to ========== Move ChannelMojo bindings to a custom multiplex router Changes MojoBootstrap - the implementation of master endpoints for IPC::ChannelMojo - to use custom message routing logic in lieu of the Mojo public Binding and InterfacePtr primitives and the underlying MultiplexRouter implementation. This is part a series of CLs to support Channel-associated interfaces. BUG=612500 Committed: https://crrev.com/02b8e188466c60a084959c554c0d598303b621c3 Cr-Commit-Position: refs/heads/master@{#405254} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/02b8e188466c60a084959c554c0d598303b621c3 Cr-Commit-Position: refs/heads/master@{#405254} |
