Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2109593006:
Add ChannelHandle::mojo_handle to mus' ChannelHandle mojom. (Closed)
|
Created:
4 years, 5 months ago by Anand Mistry (off Chromium) Modified:
4 years, 5 months ago CC:
chromium-reviews, rjkroege, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ChannelHandle::mojo_handle to mus' ChannelHandle mojom.
...and add support to the type converter.
BUG=604282
Committed: https://crrev.com/78fdffc1e7db6029385056589517fee14185828a
Cr-Commit-Position: refs/heads/master@{#404051}
Patch Set 1 #Patch Set 2 : Make mojo_handle optional, which it is. #
Total comments: 8
Patch Set 3 : rebase #Patch Set 4 : Rebase update. #
Dependent Patchsets: Messages
Total messages: 26 (9 generated)
amistry@chromium.org changed reviewers: + msw@chromium.org
msw@chromium.org changed reviewers: + sky@chromium.org
Sorry, I'm not actually the best reviewer here; +Scott. (perhaps we ought to revise components/mus/OWNERS?)
Description was changed from ========== Add ChannelHandle::mojo_handle to mus' ChannelHandle mojom. ...and add support to the type converter. BUG=604282 ========== to ========== Add ChannelHandle::mojo_handle to mus' ChannelHandle mojom. ...and add support to the type converter. BUG=604282 ==========
sky@chromium.org changed reviewers: + penghuang@chromium.org
+penghuang (sorry to keep passing the buck) Completely agree on updating owners.
On 2016/06/30 17:05:49, sky wrote: > +penghuang (sorry to keep passing the buck) Completely agree on updating owners. lgtm
Rubber stamp LGTM
amistry@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for security
https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... File components/mus/common/gpu_type_converters.cc (right): https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... components/mus/common/gpu_type_converters.cc:34: result->mojo_handle.reset(handle.mojo_handle); It's hard for me to tell where the type converter is used. What are the chances of getting rid of this type converter sooner than later? It's not at all clear to me that the DCHECKs are OK to have here (they generally aren't). https://codereview.chromium.org/2109593006/diff/20001/components/mus/public/i... File components/mus/public/interfaces/channel_handle.mojom (right): https://codereview.chromium.org/2109593006/diff/20001/components/mus/public/i... components/mus/public/interfaces/channel_handle.mojom:8: struct ChannelHandle { Would it be possible to document this interface? For example, why does mus need to do this? Naively, I would have imagined the underlying IPC stuff was Good Enough.
https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... File components/mus/common/gpu_type_converters.cc (right): https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... components/mus/common/gpu_type_converters.cc:34: result->mojo_handle.reset(handle.mojo_handle); On 2016/07/01 01:31:43, dcheng wrote: > It's hard for me to tell where the type converter is used. What are the chances > of getting rid of this type converter sooner than later? It's not at all clear > to me that the DCHECKs are OK to have here (they generally aren't). So, I'm making this change in prep for switching GPU-renderer channel to ChannelMojo. I think switching this type converter to StructTraits<> is outside the scoped of this CL. https://codereview.chromium.org/2109593006/diff/20001/components/mus/public/i... File components/mus/public/interfaces/channel_handle.mojom (right): https://codereview.chromium.org/2109593006/diff/20001/components/mus/public/i... components/mus/public/interfaces/channel_handle.mojom:8: struct ChannelHandle { On 2016/07/01 01:31:43, dcheng wrote: > Would it be possible to document this interface? For example, why does mus need > to do this? Naively, I would have imagined the underlying IPC stuff was Good > Enough. I assume this is needed for GPU channels, which still use legacy IPC channels.
https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... File components/mus/common/gpu_type_converters.cc (right): https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... components/mus/common/gpu_type_converters.cc:34: result->mojo_handle.reset(handle.mojo_handle); On 2016/07/01 05:52:54, Anand Mistry wrote: > On 2016/07/01 01:31:43, dcheng wrote: > > It's hard for me to tell where the type converter is used. What are the > chances > > of getting rid of this type converter sooner than later? It's not at all clear > > to me that the DCHECKs are OK to have here (they generally aren't). > > So, I'm making this change in prep for switching GPU-renderer channel to > ChannelMojo. I think switching this type converter to StructTraits<> is outside > the scoped of this CL. Fair enough, I've filed a followup bug and assign it to someone on the mus team. https://codereview.chromium.org/2109593006/diff/20001/components/mus/public/i... File components/mus/public/interfaces/channel_handle.mojom (right): https://codereview.chromium.org/2109593006/diff/20001/components/mus/public/i... components/mus/public/interfaces/channel_handle.mojom:8: struct ChannelHandle { On 2016/07/01 05:52:54, Anand Mistry wrote: > On 2016/07/01 01:31:43, dcheng wrote: > > Would it be possible to document this interface? For example, why does mus > need > > to do this? Naively, I would have imagined the underlying IPC stuff was Good > > Enough. > > I assume this is needed for GPU channels, which still use legacy IPC channels. mus team, can someone confirm? Is there a useful comment to add here?
https://codereview.chromium.org/2109593006/diff/20001/components/mus/public/i... File components/mus/public/interfaces/channel_handle.mojom (right): https://codereview.chromium.org/2109593006/diff/20001/components/mus/public/i... components/mus/public/interfaces/channel_handle.mojom:8: struct ChannelHandle { On 2016/07/04 07:11:59, dcheng wrote: > On 2016/07/01 05:52:54, Anand Mistry wrote: > > On 2016/07/01 01:31:43, dcheng wrote: > > > Would it be possible to document this interface? For example, why does mus > > need > > > to do this? Naively, I would have imagined the underlying IPC stuff was Good > > > Enough. > > > > I assume this is needed for GPU channels, which still use legacy IPC channels. > > mus team, can someone confirm? Is there a useful comment to add here? Mus is using the existing GPU command buffer which uses Chrome IPC, because we don't want to have and maintain two GPU command buffer implementations.
https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... File components/mus/common/gpu_type_converters.cc (right): https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... components/mus/common/gpu_type_converters.cc:34: result->mojo_handle.reset(handle.mojo_handle); On 2016/07/04 07:11:59, dcheng wrote: > On 2016/07/01 05:52:54, Anand Mistry wrote: > > On 2016/07/01 01:31:43, dcheng wrote: > > > It's hard for me to tell where the type converter is used. What are the > > chances > > > of getting rid of this type converter sooner than later? It's not at all > clear > > > to me that the DCHECKs are OK to have here (they generally aren't). > > > > So, I'm making this change in prep for switching GPU-renderer channel to > > ChannelMojo. I think switching this type converter to StructTraits<> is > outside > > the scoped of this CL. > > Fair enough, I've filed a followup bug and assign it to someone on the mus team. I think when GPU starts using ChannelMojo, we could just pass a mojo handle around instead of defining a mojo ChannelHandle structure and having converters for it.
On 2016/07/06 15:19:06, Peng wrote: > https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... > File components/mus/common/gpu_type_converters.cc (right): > > https://codereview.chromium.org/2109593006/diff/20001/components/mus/common/g... > components/mus/common/gpu_type_converters.cc:34: > result->mojo_handle.reset(handle.mojo_handle); > On 2016/07/04 07:11:59, dcheng wrote: > > On 2016/07/01 05:52:54, Anand Mistry wrote: > > > On 2016/07/01 01:31:43, dcheng wrote: > > > > It's hard for me to tell where the type converter is used. What are the > > > chances > > > > of getting rid of this type converter sooner than later? It's not at all > > clear > > > > to me that the DCHECKs are OK to have here (they generally aren't). > > > > > > So, I'm making this change in prep for switching GPU-renderer channel to > > > ChannelMojo. I think switching this type converter to StructTraits<> is > > outside > > > the scoped of this CL. > > > > Fair enough, I've filed a followup bug and assign it to someone on the mus > team. > > I think when GPU starts using ChannelMojo, And since you mentioned this, I have a CL to do exactly that (https://codereview.chromium.org/2127693002/). However, mus/mash tests fail because I need this CL. > we could just pass a mojo handle > around instead of defining a mojo ChannelHandle structure and having converters > for it.
thanks for the context. rs lgtm
The CQ bit was checked by amistry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from penghuang@chromium.org, sky@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2109593006/#ps60001 (title: "Rebase update.")
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.
Description was changed from ========== Add ChannelHandle::mojo_handle to mus' ChannelHandle mojom. ...and add support to the type converter. BUG=604282 ========== to ========== Add ChannelHandle::mojo_handle to mus' ChannelHandle mojom. ...and add support to the type converter. BUG=604282 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add ChannelHandle::mojo_handle to mus' ChannelHandle mojom. ...and add support to the type converter. BUG=604282 ========== to ========== Add ChannelHandle::mojo_handle to mus' ChannelHandle mojom. ...and add support to the type converter. BUG=604282 Committed: https://crrev.com/78fdffc1e7db6029385056589517fee14185828a Cr-Commit-Position: refs/heads/master@{#404051} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/78fdffc1e7db6029385056589517fee14185828a Cr-Commit-Position: refs/heads/master@{#404051} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
