Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686833003:
CopyOutputResult must have a working release callback when received over mojo (Closed)
|
Created:
3 years, 10 months ago by Saman Sami Modified:
3 years, 10 months ago CC:
Aaron Boodman, abarth-chromium, cc-bugs_chromium.org, chromium-reviews, darin (slow to review), miu+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, xjz+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCopyOutputResult must have a working release callback when received over mojo
Right now, CopyOutputResult when received over mojo has an empty release
callback. This CL introduces TextureMailboxReleaser, which is a mojo
interface that has a Release method with the same signature as
ReleaseCallback. On the sender side, the ReleaseCallback of the
CopyOutputResult is retained in a TextureMailboxReleaserImpl and
a TextureMailboxReleaserPtr will be sent to the receiver. When
the receiver calls the Release method, the sender calls the release
callback.
BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2686833003
Cr-Commit-Position: refs/heads/master@{#450558}
Committed: https://chromium.googlesource.com/chromium/src/+/05612a0cb0feafb73ec78c2aebef4c1af797380f
Patch Set 1 #Patch Set 2 : c #Patch Set 3 : c #Patch Set 4 : c #Patch Set 5 : c #Patch Set 6 : c #Patch Set 7 : c #
Total comments: 10
Patch Set 8 : c #
Total comments: 3
Patch Set 9 : c #Patch Set 10 : c #Patch Set 11 : c #Patch Set 12 : c #Patch Set 13 : c #
Total comments: 2
Patch Set 14 : c #Patch Set 15 : c #
Total comments: 22
Patch Set 16 : c #
Total comments: 2
Patch Set 17 : d #
Total comments: 3
Patch Set 18 : c #Patch Set 19 : c #
Total comments: 11
Patch Set 20 : c #
Total comments: 5
Patch Set 21 : Using constrcutor in read #Patch Set 22 : c #Patch Set 23 : c #Patch Set 24 : comment #Patch Set 25 : c #
Total comments: 2
Patch Set 26 : c #Patch Set 27 : c #
Messages
Total messages: 124 (87 generated)
Description was changed from ========== SingleReleaseCallback should work accorss process boundaries BUG=672071 ========== to ========== SingleReleaseCallback should work accorss process boundaries BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@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 checked by samans@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 checked by samans@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 checked by samans@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...
Description was changed from ========== SingleReleaseCallback should work accorss process boundaries BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== SingleReleaseCallback should work across process boundaries BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_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 samans@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 checked by samans@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 checked by samans@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...
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... File cc/resources/single_release_callback.cc (right): https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.cc:23: DCHECK(callback_.is_null() && !ptr_holder_) split a DCHECK with && into 2 DCHECKs so you can tell which failed https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.cc:29: DCHECK(!callback_.is_null() || ptr_holder_) I think this can be written DCHECK(callback_ || ptr_holder_) now which is nicer without the double negatives https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... File cc/resources/single_release_callback.h (right): https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.h:36: class PtrHolderImpl : public PtrHolder { If this is implemented right here, I'm not sure what the point of the virtuals is. Can this be implemented inside StructTraits<cc::mojom::SingleReleaseCallbackDataView, std::unique_ptr<cc::SingleReleaseCallback>>? Then no friend relationships are needed either. https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.h:54: template <class T> Why template and not pass a unique_ptr<PtrHolder>?
The CQ bit was checked by samans@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...
PTAL https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... File cc/resources/single_release_callback.cc (right): https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.cc:23: DCHECK(callback_.is_null() && !ptr_holder_) On 2017/02/08 20:53:31, danakj (slow) wrote: > split a DCHECK with && into 2 DCHECKs so you can tell which failed Done. https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.cc:29: DCHECK(!callback_.is_null() || ptr_holder_) On 2017/02/08 20:53:31, danakj (slow) wrote: > I think this can be written DCHECK(callback_ || ptr_holder_) now which is nicer > without the double negatives Done. https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... File cc/resources/single_release_callback.h (right): https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.h:36: class PtrHolderImpl : public PtrHolder { On 2017/02/08 20:53:31, danakj (slow) wrote: > If this is implemented right here, I'm not sure what the point of the virtuals > is. Can this be implemented inside > StructTraits<cc::mojom::SingleReleaseCallbackDataView, > std::unique_ptr<cc::SingleReleaseCallback>>? Then no friend relationships are > needed either. We use the virtuals because this template cannot be instantiated here due to dependency problems. A class similar to this has to be implemented in not only struct traits but also struct traits unit tests and GpuCompositorFrameSink (in a future CL). Having this template here avoids duplicate code. https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.h:54: template <class T> On 2017/02/08 20:53:31, danakj (slow) wrote: > Why template and not pass a unique_ptr<PtrHolder>? I think this looks better and more intuitive than cc::SingleReleaseCallback::Create(cc::SingleReleaseCallback::PtrHolder(std::move(ptr))). We also won't have to expose the internal classes at all.
samans@chromium.org changed reviewers: + tsepez@chromium.org
tsepez@: Please review mojo.
Description was changed from ========== SingleReleaseCallback should work across process boundaries BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== SingleReleaseCallback should work across process boundaries We need the SingleReleaseCallback to work when CopyOutputResult is sent back from the gpu process. This CL enables us to store a TextureMailboxReleaserPtr in SingleReleaseCallback instead of a ReleaseCallback so that it can be passed to another process. Note that SingleReleaseCallback will either call its ReleaseCallback or use its TextureMailboxReleaserPtr, depending on which one it has. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... File cc/resources/single_release_callback.h (right): https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.h:36: class PtrHolderImpl : public PtrHolder { On 2017/02/08 21:44:21, Saman Sami wrote: > On 2017/02/08 20:53:31, danakj (slow) wrote: > > If this is implemented right here, I'm not sure what the point of the virtuals > > is. Can this be implemented inside > > StructTraits<cc::mojom::SingleReleaseCallbackDataView, > > std::unique_ptr<cc::SingleReleaseCallback>>? Then no friend relationships are > > needed either. > > We use the virtuals because this template cannot be instantiated here due to > dependency problems. A class similar to this has to be implemented in not only > struct traits but also struct traits unit tests and GpuCompositorFrameSink (in a > future CL). Having this template here avoids duplicate code. It's fine to have a test impl and a production impl. I don't think we should use a template to avoid that. It also helps avoid test requirements leaking into production. https://codereview.chromium.org/2686833003/diff/140001/cc/ipc/single_release_... File cc/ipc/single_release_callback_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/140001/cc/ipc/single_release_... cc/ipc/single_release_callback_struct_traits.cc:15: auto result = callback->ptr_holder_->TakePtr(); It seems like we're being tied to the c++ types too strongly here. When we serialize we've already made a mojo type and hid it inside the SingleReleaseCallback just so that we have the right type here. Maybe we can loosen these requirements, and use a different type than SingleReleaseCallback here as a member of the mojo'd CopyOutputResult. Or maybe we need a different mojo CopyOutputResult type or something. But I think at the end we shouldn't have SingleResultCallback knowing about mojo, we should have a distinct type entirely, or it has an interface that happens to have a mojo-based impl without it knowing. Which means TakePtr() here doesn't work. So that says to me the serialization code shouldn't be making a mojo-backed SingleReleaseCallback, only the reciever side should be doing that. And the sender should be sending something else..
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
samans@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... File cc/resources/single_release_callback.h (right): https://codereview.chromium.org/2686833003/diff/120001/cc/resources/single_re... cc/resources/single_release_callback.h:36: class PtrHolderImpl : public PtrHolder { On 2017/02/08 23:24:17, danakj (slow) wrote: > On 2017/02/08 21:44:21, Saman Sami wrote: > > On 2017/02/08 20:53:31, danakj (slow) wrote: > > > If this is implemented right here, I'm not sure what the point of the > virtuals > > > is. Can this be implemented inside > > > StructTraits<cc::mojom::SingleReleaseCallbackDataView, > > > std::unique_ptr<cc::SingleReleaseCallback>>? Then no friend relationships > are > > > needed either. > > > > We use the virtuals because this template cannot be instantiated here due to > > dependency problems. A class similar to this has to be implemented in not only > > struct traits but also struct traits unit tests and GpuCompositorFrameSink (in > a > > future CL). Having this template here avoids duplicate code. > > It's fine to have a test impl and a production impl. I don't think we should use > a template to avoid that. It also helps avoid test requirements leaking into > production. OK. I can avoid the template if having duplicate code is fine. https://codereview.chromium.org/2686833003/diff/140001/cc/ipc/single_release_... File cc/ipc/single_release_callback_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/140001/cc/ipc/single_release_... cc/ipc/single_release_callback_struct_traits.cc:15: auto result = callback->ptr_holder_->TakePtr(); On 2017/02/08 23:24:17, danakj (slow) wrote: > It seems like we're being tied to the c++ types too strongly here. When we > serialize we've already made a mojo type and hid it inside the > SingleReleaseCallback just so that we have the right type here. Maybe we can > loosen these requirements, and use a different type than SingleReleaseCallback > here as a member of the mojo'd CopyOutputResult. Or maybe we need a different > mojo CopyOutputResult type or something. > > But I think at the end we shouldn't have SingleResultCallback knowing about > mojo, we should have a distinct type entirely, or it has an interface that > happens to have a mojo-based impl without it knowing. Which means TakePtr() here > doesn't work. So that says to me the serialization code shouldn't be making a > mojo-backed SingleReleaseCallback, only the reciever side should be doing that. > And the sender should be sending something else.. In struct traits, when serializing, a mojo pointer has to be somehow obtained. One way is to have already stored the pointer in the copy output result (either directly, or indirectly in SingleReleaseCallback), which you do not seem to be a huge fan of. An alternative is that CopyOutputResult can have a PtrProvider or something that has a virtual method that GpuCompositorFrameSink implements which takes in a SingleReleaseCallback and returns a mojo pointer to be used in struct traits while serializing. When deseriazing, we turn the mojo pointer into a SingleReleaseCallback using base::Bind. What do you think? This doesn't sound too terrible.
On Thu, Feb 9, 2017 at 11:30 AM, <samans@chromium.org> wrote: > > https://codereview.chromium.org/2686833003/diff/120001/cc/ > resources/single_release_callback.h > File cc/resources/single_release_callback.h (right): > > https://codereview.chromium.org/2686833003/diff/120001/cc/ > resources/single_release_callback.h#newcode36 > cc/resources/single_release_callback.h:36: class PtrHolderImpl : public > PtrHolder { > On 2017/02/08 23:24:17, danakj (slow) wrote: > > On 2017/02/08 21:44:21, Saman Sami wrote: > > > On 2017/02/08 20:53:31, danakj (slow) wrote: > > > > If this is implemented right here, I'm not sure what the point of > the > > virtuals > > > > is. Can this be implemented inside > > > > StructTraits<cc::mojom::SingleReleaseCallbackDataView, > > > > std::unique_ptr<cc::SingleReleaseCallback>>? Then no friend > relationships > > are > > > > needed either. > > > > > > We use the virtuals because this template cannot be instantiated > here due to > > > dependency problems. A class similar to this has to be implemented > in not only > > > struct traits but also struct traits unit tests and > GpuCompositorFrameSink (in > > a > > > future CL). Having this template here avoids duplicate code. > > > > It's fine to have a test impl and a production impl. I don't think we > should use > > a template to avoid that. It also helps avoid test requirements > leaking into > > production. > > OK. I can avoid the template if having duplicate code is fine. > > https://codereview.chromium.org/2686833003/diff/140001/cc/ > ipc/single_release_callback_struct_traits.cc > File cc/ipc/single_release_callback_struct_traits.cc (right): > > https://codereview.chromium.org/2686833003/diff/140001/cc/ > ipc/single_release_callback_struct_traits.cc#newcode15 > cc/ipc/single_release_callback_struct_traits.cc:15: auto result = > callback->ptr_holder_->TakePtr(); > On 2017/02/08 23:24:17, danakj (slow) wrote: > > It seems like we're being tied to the c++ types too strongly here. > When we > > serialize we've already made a mojo type and hid it inside the > > SingleReleaseCallback just so that we have the right type here. Maybe > we can > > loosen these requirements, and use a different type than > SingleReleaseCallback > > here as a member of the mojo'd CopyOutputResult. Or maybe we need a > different > > mojo CopyOutputResult type or something. > > > > But I think at the end we shouldn't have SingleResultCallback knowing > about > > mojo, we should have a distinct type entirely, or it has an interface > that > > happens to have a mojo-based impl without it knowing. Which means > TakePtr() here > > doesn't work. So that says to me the serialization code shouldn't be > making a > > mojo-backed SingleReleaseCallback, only the reciever side should be > doing that. > > And the sender should be sending something else.. > > In struct traits, when serializing, a mojo pointer has to be somehow > obtained. > Ok few questions. I think you're saying that we need to put cc::SingleReleaseCallback here right? https://cs.chromium.org/chromium/src/out/Debug/gen/cc/ipc/copy_output_result.... Can there be a mojo interface there instead? Ie TextureMailboxRelease or whatever? > One way is to have already stored the pointer in the copy > output result (either directly, or indirectly in SingleReleaseCallback), > which you do not seem to be a huge fan of. > I'm okay with the idea of it being indirectly in SingleReleaseCallback, but in this patch we're making friend relationships to mojo code and putting mojo types directly in SingleReleaseCallback. It seems like it should be a different type at that point to me. But I am not also sure how to suggest making it indirect when mojo code needs to pull out a mojo interface from it. Which again made me feel like we're putting a square in a round hole and the mojo code could ideally use a different type. > An alternative is that > CopyOutputResult can have a PtrProvider or something that has a virtual > method that GpuCompositorFrameSink implements which takes in a > SingleReleaseCallback and returns a mojo pointer to be used in struct > traits while serializing. When deseriazing, we turn the mojo pointer > into a SingleReleaseCallback using base::Bind. What do you think? This > doesn't sound too terrible. > I think that sounds good, and aligns with what I just wrong above. Please lmk if I'm wrong. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2686833003/diff/140001/cc/ipc/single_release_... File cc/ipc/single_release_callback_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/140001/cc/ipc/single_release_... cc/ipc/single_release_callback_struct_traits.cc:15: auto result = callback->ptr_holder_->TakePtr(); On 2017/02/09 16:30:01, Saman Sami wrote: > On 2017/02/08 23:24:17, danakj (slow) wrote: > > It seems like we're being tied to the c++ types too strongly here. When we > > serialize we've already made a mojo type and hid it inside the > > SingleReleaseCallback just so that we have the right type here. Maybe we can > > loosen these requirements, and use a different type than SingleReleaseCallback > > here as a member of the mojo'd CopyOutputResult. Or maybe we need a different > > mojo CopyOutputResult type or something. > > > > But I think at the end we shouldn't have SingleResultCallback knowing about > > mojo, we should have a distinct type entirely, or it has an interface that > > happens to have a mojo-based impl without it knowing. Which means TakePtr() > here > > doesn't work. So that says to me the serialization code shouldn't be making a > > mojo-backed SingleReleaseCallback, only the reciever side should be doing > that. > > And the sender should be sending something else.. > > In struct traits, when serializing, a mojo pointer has to be somehow obtained. > One way is to have already stored the pointer in the copy output result (either > directly, or indirectly in SingleReleaseCallback), which you do not seem to be a > huge fan of. An alternative is that CopyOutputResult can have a PtrProvider or > something that has a virtual method that GpuCompositorFrameSink implements which > takes in a SingleReleaseCallback and returns a mojo pointer to be used in struct > traits while serializing. When deseriazing, we turn the mojo pointer into a > SingleReleaseCallback using base::Bind. What do you think? This doesn't sound > too terrible. However, note that having a method like TakePtr that returns a TextureMailboxReleaserPtr is unavoidable. In my new proposal, the PtrProvider class will have such a method. But it would be a virtual method with no implementation in cc/, which is I guess the best we could hope for.
On Thu, Feb 9, 2017 at 11:52 AM, <samans@chromium.org> wrote: > > https://codereview.chromium.org/2686833003/diff/140001/cc/ > ipc/single_release_callback_struct_traits.cc > File cc/ipc/single_release_callback_struct_traits.cc (right): > > https://codereview.chromium.org/2686833003/diff/140001/cc/ > ipc/single_release_callback_struct_traits.cc#newcode15 > cc/ipc/single_release_callback_struct_traits.cc:15: auto result = > callback->ptr_holder_->TakePtr(); > On 2017/02/09 16:30:01, Saman Sami wrote: > > On 2017/02/08 23:24:17, danakj (slow) wrote: > > > It seems like we're being tied to the c++ types too strongly here. > When we > > > serialize we've already made a mojo type and hid it inside the > > > SingleReleaseCallback just so that we have the right type here. > Maybe we can > > > loosen these requirements, and use a different type than > SingleReleaseCallback > > > here as a member of the mojo'd CopyOutputResult. Or maybe we need a > different > > > mojo CopyOutputResult type or something. > > > > > > But I think at the end we shouldn't have SingleResultCallback > knowing about > > > mojo, we should have a distinct type entirely, or it has an > interface that > > > happens to have a mojo-based impl without it knowing. Which means > TakePtr() > > here > > > doesn't work. So that says to me the serialization code shouldn't be > making a > > > mojo-backed SingleReleaseCallback, only the reciever side should be > doing > > that. > > > And the sender should be sending something else.. > > > > In struct traits, when serializing, a mojo pointer has to be somehow > obtained. > > One way is to have already stored the pointer in the copy output > result (either > > directly, or indirectly in SingleReleaseCallback), which you do not > seem to be a > > huge fan of. An alternative is that CopyOutputResult can have a > PtrProvider or > > something that has a virtual method that GpuCompositorFrameSink > implements which > > takes in a SingleReleaseCallback and returns a mojo pointer to be used > in struct > > traits while serializing. When deseriazing, we turn the mojo pointer > into a > > SingleReleaseCallback using base::Bind. What do you think? This > doesn't sound > > too terrible. > > However, note that having a method like TakePtr that returns a > TextureMailboxReleaserPtr is unavoidable. In my new proposal, the > PtrProvider class will have such a method. But it would be a virtual > method with no implementation in cc/, which is I guess the best we could > hope for. > I've asked dcheng for some help in how to make mojo happier here, we'll see what input he might have. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by samans@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...
Description was changed from ========== SingleReleaseCallback should work across process boundaries We need the SingleReleaseCallback to work when CopyOutputResult is sent back from the gpu process. This CL enables us to store a TextureMailboxReleaserPtr in SingleReleaseCallback instead of a ReleaseCallback so that it can be passed to another process. Note that SingleReleaseCallback will either call its ReleaseCallback or use its TextureMailboxReleaserPtr, depending on which one it has. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== CopyOutputResult must have a working release callback when received over mojo BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@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: This issue passed the CQ dry run.
The CQ bit was checked by samans@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...
samans@chromium.org changed reviewers: + rockot@chromium.org
The CQ bit was checked by samans@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/2686833003/diff/240001/cc/ipc/struct_traits_u... File cc/ipc/struct_traits_unittest.cc (right): https://codereview.chromium.org/2686833003/diff/240001/cc/ipc/struct_traits_u... cc/ipc/struct_traits_unittest.cc:27: class TextureMailboxReleaserImpl : public mojom::TextureMailboxReleaser { I don't understand why you need more than this impl of TextureMailboxRelease (and therefore why you need a TextureMailboxReleaseProvider). Maybe you aren't aware of StrongBinding! You should be able to hide this impl inside the traits cc and have the serializer do: cc::mojom::TextureMailboxReleaserPtr StructTraits<cc::mojom::CopyOutputResultDataView, std::unique_ptr<cc::CopyOutputResult>>:: releaser(const std::unique_ptr<cc::CopyOutputResult>& result) { if (result->release_callback_) { cc::mojom::TextureMailboxReleaserPtr releaser; mojo::MakeStrongBinding( base::MakeUnique<ReleaserImpl>(result->release_callback_), mojo::MakeRequest(&release)); return releaser; } else { return cc::mojom::TextureMailboxReleaserPtr(); } }
https://codereview.chromium.org/2686833003/diff/240001/cc/ipc/struct_traits_u... File cc/ipc/struct_traits_unittest.cc (right): https://codereview.chromium.org/2686833003/diff/240001/cc/ipc/struct_traits_u... cc/ipc/struct_traits_unittest.cc:27: class TextureMailboxReleaserImpl : public mojom::TextureMailboxReleaser { On 2017/02/10 01:17:24, Ken Rockot wrote: > I don't understand why you need more than this impl of TextureMailboxRelease > (and therefore why you need a TextureMailboxReleaseProvider). Maybe you aren't > aware of StrongBinding! You should be able to hide this impl inside the traits > cc and have the serializer do: > > cc::mojom::TextureMailboxReleaserPtr > StructTraits<cc::mojom::CopyOutputResultDataView, > std::unique_ptr<cc::CopyOutputResult>>:: > releaser(const std::unique_ptr<cc::CopyOutputResult>& result) { > if (result->release_callback_) { > cc::mojom::TextureMailboxReleaserPtr releaser; > mojo::MakeStrongBinding( > base::MakeUnique<ReleaserImpl>(result->release_callback_), > mojo::MakeRequest(&release)); > return releaser; > } else { > return cc::mojom::TextureMailboxReleaserPtr(); > } > } That's a great advice! The whole point of the Provider class is that I thought the binding has to live somewhere! I think StrongBinding should work and make things a whole lot simpler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by samans@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...
tsepez@: Please review mojo rockot@: Please review mojo fsamuel@: Please review all files danakj@: Please review all files
Description was changed from ========== CopyOutputResult must have a working release callback when received over mojo BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== CopyOutputResult must have a working release callback when received over mojo Right now, CopyOutputResult when received over mojo has an empty release callback. This CL introduces TextureMailboxReleaser, which is a mojo interface that has a Release method with the same signature as ReleaseCallback. On the sender side, the ReleaseCallback of the CopyOutputResult is retained in a TextureMailboxReleaserImpl and a TextureMailboxReleaserPtr will be sent to the receiver. When the receiver calls the Release method, the sender calls the release callback. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Thanks this is what I was hoping for https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/BUILD.gn File cc/ipc/BUILD.gn (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/BUILD.gn#newcode42 cc/ipc/BUILD.gn:42: "texture_mailbox_releaser.mojom", why isn't this part of "interfaces" below? https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result.mojom (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result.mojom:17: TextureMailboxReleaser? releaser; If this is optional, TextureMailbox should be too, and they should both be present when one is present. Can you express that in mojo? For that matter bitmap and texture_mailbox shouldn't both be present either.. https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:40: static SkBitmap* null_bitmap = new SkBitmap(); curious why do we need this, given that bitmap() is an optional field? the return type here seems to imply its not optional https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:51: if (result->release_callback_) { Prefer early outs to if/else. Here: if (!release callback) return {}; <!-- don't have to indent the rest of this method --> https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:58: return cc::mojom::TextureMailboxReleaserPtr(); nit: you could "return {};" here https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:82: base::Bind(Release, base::Passed(&releaser))); you can bind directly to TextureMailboxReleaserPtr::Release here, you don't need the intermediate Release static method?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
drive-by https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/texture_mailbox... File cc/ipc/texture_mailbox_releaser.mojom (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/texture_mailbox... cc/ipc/texture_mailbox_releaser.mojom:9: interface TextureMailboxReleaser { Btw, this interface (and the method too perhaps?) need some sort of comment.
https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result.mojom (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result.mojom:17: TextureMailboxReleaser? releaser; On 2017/02/10 at 18:32:50, danakj wrote: > If this is optional, TextureMailbox should be too, and they should both be present when one is present. Can you express that in mojo? For that matter bitmap and texture_mailbox shouldn't both be present either.. The way to express this would be to group them: struct TextureInfo { skia.mojom.Bitmap bitmap; TextureMailbox texture_mailbox; TextureMailboxReleaser releaser; }; struct CopyOutputResult { gfx.mojom.Size size; TextureInfo? texture_info; }; But then, if there's no bitmap, mailbox, or releaser, is there really a CopyOutputResult at all? Could we just make none of the fields optional, and have messages which reply with "CopyOutputResult" instead reply with "CopyOutputResult?" ?
On Fri, Feb 10, 2017 at 1:56 PM, <rockot@chromium.org> wrote: > > https://codereview.chromium.org/2686833003/diff/280001/cc/ > ipc/copy_output_result.mojom > File cc/ipc/copy_output_result.mojom (right): > > https://codereview.chromium.org/2686833003/diff/280001/cc/ > ipc/copy_output_result.mojom#newcode17 > cc/ipc/copy_output_result.mojom:17: TextureMailboxReleaser? releaser; > On 2017/02/10 at 18:32:50, danakj wrote: > > If this is optional, TextureMailbox should be too, and they should > both be present when one is present. Can you express that in mojo? For > that matter bitmap and texture_mailbox shouldn't both be present > either.. > > The way to express this would be to group them: > > struct TextureInfo { > skia.mojom.Bitmap bitmap; > TextureMailbox texture_mailbox; > TextureMailboxReleaser releaser; > }; > > struct CopyOutputResult { > gfx.mojom.Size size; > TextureInfo? texture_info; > }; > > > But then, if there's no bitmap, mailbox, or releaser, is there really a > CopyOutputResult at all? Could we just make none of the fields optional, > and have messages which reply with "CopyOutputResult" instead reply with > "CopyOutputResult?" ? > There can be one of: - No Bitmap, Texture, or SingleReleaseCallback (empty result) - Bitmap - Texture + SingleReleaseCallback In the empty case mojo can make CopyOutputResult optional if that makes sense but the client side gets an empty CopyOutputResult eventually, cuz that is what callbacks receive. > > https://codereview.chromium.org/2686833003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #16 (id:300001) has been deleted
On Fri, Feb 10, 2017 at 11:00 AM, <danakj@chromium.org> wrote: > > > On Fri, Feb 10, 2017 at 1:56 PM, <rockot@chromium.org> wrote: > >> >> https://codereview.chromium.org/2686833003/diff/280001/cc/ip >> c/copy_output_result.mojom >> File cc/ipc/copy_output_result.mojom (right): >> >> https://codereview.chromium.org/2686833003/diff/280001/cc/ip >> c/copy_output_result.mojom#newcode17 >> cc/ipc/copy_output_result.mojom:17: TextureMailboxReleaser? releaser; >> On 2017/02/10 at 18:32:50, danakj wrote: >> > If this is optional, TextureMailbox should be too, and they should >> both be present when one is present. Can you express that in mojo? For >> that matter bitmap and texture_mailbox shouldn't both be present >> either.. >> >> The way to express this would be to group them: >> >> struct TextureInfo { >> skia.mojom.Bitmap bitmap; >> TextureMailbox texture_mailbox; >> TextureMailboxReleaser releaser; >> }; >> >> struct CopyOutputResult { >> gfx.mojom.Size size; >> TextureInfo? texture_info; >> }; >> >> >> But then, if there's no bitmap, mailbox, or releaser, is there really a >> CopyOutputResult at all? Could we just make none of the fields optional, >> and have messages which reply with "CopyOutputResult" instead reply with >> "CopyOutputResult?" ? >> > > There can be one of: > - No Bitmap, Texture, or SingleReleaseCallback (empty result) > - Bitmap > - Texture + SingleReleaseCallback > Ah, got it. So in that case grouping just TextureMailbox and TextureMailboxReleaser into a struct makes sense. This would of course mean having to define an equivalent intermediate struct to support typemapping between TextureInfo and the native cc::CopyOutputResult fields, but its definition could live directly in the traits h/cc since no user code will ever touch it. > In the empty case mojo can make CopyOutputResult optional if that makes > sense but the client side gets an empty CopyOutputResult eventually, cuz > that is what callbacks receive. > > >> >> https://codereview.chromium.org/2686833003/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by samans@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/2686833003/diff/280001/cc/ipc/BUILD.gn File cc/ipc/BUILD.gn (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/BUILD.gn#newcode42 cc/ipc/BUILD.gn:42: "texture_mailbox_releaser.mojom", On 2017/02/10 18:32:50, danakj wrote: > why isn't this part of "interfaces" below? because struct traits depending on interfaces creates a cycle. https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:40: static SkBitmap* null_bitmap = new SkBitmap(); On 2017/02/10 18:32:51, danakj wrote: > curious why do we need this, given that bitmap() is an optional field? the > return type here seems to imply its not optional SkBitmap is nullable (The struct traits has IsNull and SetToNull). That's all is needed to use the ? notation. See https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap_skbitmap_s... https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:51: if (result->release_callback_) { On 2017/02/10 18:32:51, danakj wrote: > Prefer early outs to if/else. Here: > > if (!release callback) > return {}; > > <!-- don't have to indent the rest of this method --> Done. https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:58: return cc::mojom::TextureMailboxReleaserPtr(); On 2017/02/10 18:32:51, danakj wrote: > nit: you could "return {};" here Done. https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:82: base::Bind(Release, base::Passed(&releaser))); On 2017/02/10 18:32:51, danakj wrote: > you can bind directly to TextureMailboxReleaserPtr::Release here, you don't need > the intermediate Release static method? TextureMailboxReleaserPtr does not have a Release method. It has the -> operator which returns the TextureMailboxReleaserProxy that it owns. https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/texture_mailbox... File cc/ipc/texture_mailbox_releaser.mojom (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/texture_mailbox... cc/ipc/texture_mailbox_releaser.mojom:9: interface TextureMailboxReleaser { On 2017/02/10 18:50:21, dcheng wrote: > Btw, this interface (and the method too perhaps?) need some sort of comment. Done.
On Fri, Feb 10, 2017 at 2:05 PM, Ken Rockot <rockot@chromium.org> wrote: > > > On Fri, Feb 10, 2017 at 11:00 AM, <danakj@chromium.org> wrote: > >> >> >> On Fri, Feb 10, 2017 at 1:56 PM, <rockot@chromium.org> wrote: >> >>> >>> https://codereview.chromium.org/2686833003/diff/280001/cc/ip >>> c/copy_output_result.mojom >>> File cc/ipc/copy_output_result.mojom (right): >>> >>> https://codereview.chromium.org/2686833003/diff/280001/cc/ip >>> c/copy_output_result.mojom#newcode17 >>> cc/ipc/copy_output_result.mojom:17: TextureMailboxReleaser? releaser; >>> On 2017/02/10 at 18:32:50, danakj wrote: >>> > If this is optional, TextureMailbox should be too, and they should >>> both be present when one is present. Can you express that in mojo? For >>> that matter bitmap and texture_mailbox shouldn't both be present >>> either.. >>> >>> The way to express this would be to group them: >>> >>> struct TextureInfo { >>> skia.mojom.Bitmap bitmap; >>> TextureMailbox texture_mailbox; >>> TextureMailboxReleaser releaser; >>> }; >>> >>> struct CopyOutputResult { >>> gfx.mojom.Size size; >>> TextureInfo? texture_info; >>> }; >>> >>> >>> But then, if there's no bitmap, mailbox, or releaser, is there really a >>> CopyOutputResult at all? Could we just make none of the fields optional, >>> and have messages which reply with "CopyOutputResult" instead reply with >>> "CopyOutputResult?" ? >>> >> >> There can be one of: >> - No Bitmap, Texture, or SingleReleaseCallback (empty result) >> - Bitmap >> - Texture + SingleReleaseCallback >> > > Ah, got it. So in that case grouping just TextureMailbox and > TextureMailboxReleaser into a struct makes sense. > > This would of course mean having to define an equivalent intermediate > struct to support typemapping between TextureInfo and the native > cc::CopyOutputResult fields, but its definition could live directly in the > traits h/cc since no user code will ever touch it. > OK, I'm not sure if it's worth making things more complex if you only make mojo objects out of C++ objects and those define the behaviour correctly? I wonder what approach to this is normal in mojo, does it try to be authoritative on what's a valid object? > > >> In the empty case mojo can make CopyOutputResult optional if that makes >> sense but the client side gets an empty CopyOutputResult eventually, cuz >> that is what callbacks receive. >> >> >>> >>> https://codereview.chromium.org/2686833003/ >>> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/10 19:13:47, danakj wrote: > On Fri, Feb 10, 2017 at 2:05 PM, Ken Rockot <mailto:rockot@chromium.org> wrote: > > > > > > > On Fri, Feb 10, 2017 at 11:00 AM, <mailto:danakj@chromium.org> wrote: > > > >> > >> > >> On Fri, Feb 10, 2017 at 1:56 PM, <mailto:rockot@chromium.org> wrote: > >> > >>> > >>> https://codereview.chromium.org/2686833003/diff/280001/cc/ip > >>> c/copy_output_result.mojom > >>> File cc/ipc/copy_output_result.mojom (right): > >>> > >>> https://codereview.chromium.org/2686833003/diff/280001/cc/ip > >>> c/copy_output_result.mojom#newcode17 > >>> cc/ipc/copy_output_result.mojom:17: TextureMailboxReleaser? releaser; > >>> On 2017/02/10 at 18:32:50, danakj wrote: > >>> > If this is optional, TextureMailbox should be too, and they should > >>> both be present when one is present. Can you express that in mojo? For > >>> that matter bitmap and texture_mailbox shouldn't both be present > >>> either.. > >>> > >>> The way to express this would be to group them: > >>> > >>> struct TextureInfo { > >>> skia.mojom.Bitmap bitmap; > >>> TextureMailbox texture_mailbox; > >>> TextureMailboxReleaser releaser; > >>> }; > >>> > >>> struct CopyOutputResult { > >>> gfx.mojom.Size size; > >>> TextureInfo? texture_info; > >>> }; > >>> > >>> > >>> But then, if there's no bitmap, mailbox, or releaser, is there really a > >>> CopyOutputResult at all? Could we just make none of the fields optional, > >>> and have messages which reply with "CopyOutputResult" instead reply with > >>> "CopyOutputResult?" ? > >>> > >> > >> There can be one of: > >> - No Bitmap, Texture, or SingleReleaseCallback (empty result) > >> - Bitmap > >> - Texture + SingleReleaseCallback > >> > > > > Ah, got it. So in that case grouping just TextureMailbox and > > TextureMailboxReleaser into a struct makes sense. > > > > This would of course mean having to define an equivalent intermediate > > struct to support typemapping between TextureInfo and the native > > cc::CopyOutputResult fields, but its definition could live directly in the > > traits h/cc since no user code will ever touch it. > > > > OK, I'm not sure if it's worth making things more complex if you only make > mojo objects out of C++ objects and those define the behaviour correctly? I > wonder what approach to this is normal in mojo, does it try to be > authoritative on what's a valid object? > > > > > > > >> In the empty case mojo can make CopyOutputResult optional if that makes > >> sense but the client side gets an empty CopyOutputResult eventually, cuz > >> that is what callbacks receive. > >> > >> > >>> > >>> https://codereview.chromium.org/2686833003/ > >>> > >> > >> > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. We can do some validation in struct traits instead. I can define the null CopyOutputResult, and also in the read method I can do some extra checks. But grouping TextureMailbox and TextureMailboxReleaser might be a good idea too. Actually, I've mostly seen them together in the code base. So there could even be a C++ equivalent?
For any validation beyond what can be expressed in mojom IDL (e.g. making sure non-nullable things aren't null, etc), typemapping is required, and it falls on StructTraits to validate. We exploit this to get reliable validation logic in place without yet having support for its expression in mojom directly (e.g. we enforce max URL length when deserializing a url.mojom.Url as a GURL). One can always build a (non-chromium) target which doesn't apply any given typemap and thus doesn't get the relevant StructTraits' validaiton logic. In general this is OK because we don't anticipate using anyone C++ bindings for without also using the chromium or blink typemap configurations. In this case however, I kind of think it makes sense for the grouping to be made explicit in mojom simply because it is, in fact, something which can be expressed in mojom. I'm not sure it really adds significant complexity to the traits, but I'll defer to you folks on this decision. On Fri, Feb 10, 2017 at 11:06 AM, <danakj@chromium.org> wrote: > On Fri, Feb 10, 2017 at 2:05 PM, Ken Rockot <rockot@chromium.org> wrote: > >> >> >> On Fri, Feb 10, 2017 at 11:00 AM, <danakj@chromium.org> wrote: >> >>> >>> >>> On Fri, Feb 10, 2017 at 1:56 PM, <rockot@chromium.org> wrote: >>> >>>> >>>> https://codereview.chromium.org/2686833003/diff/280001/cc/ip >>>> c/copy_output_result.mojom >>>> File cc/ipc/copy_output_result.mojom (right): >>>> >>>> https://codereview.chromium.org/2686833003/diff/280001/cc/ip >>>> c/copy_output_result.mojom#newcode17 >>>> cc/ipc/copy_output_result.mojom:17: TextureMailboxReleaser? releaser; >>>> On 2017/02/10 at 18:32:50, danakj wrote: >>>> > If this is optional, TextureMailbox should be too, and they should >>>> both be present when one is present. Can you express that in mojo? For >>>> that matter bitmap and texture_mailbox shouldn't both be present >>>> either.. >>>> >>>> The way to express this would be to group them: >>>> >>>> struct TextureInfo { >>>> skia.mojom.Bitmap bitmap; >>>> TextureMailbox texture_mailbox; >>>> TextureMailboxReleaser releaser; >>>> }; >>>> >>>> struct CopyOutputResult { >>>> gfx.mojom.Size size; >>>> TextureInfo? texture_info; >>>> }; >>>> >>>> >>>> But then, if there's no bitmap, mailbox, or releaser, is there really a >>>> CopyOutputResult at all? Could we just make none of the fields optional, >>>> and have messages which reply with "CopyOutputResult" instead reply with >>>> "CopyOutputResult?" ? >>>> >>> >>> There can be one of: >>> - No Bitmap, Texture, or SingleReleaseCallback (empty result) >>> - Bitmap >>> - Texture + SingleReleaseCallback >>> >> >> Ah, got it. So in that case grouping just TextureMailbox and >> TextureMailboxReleaser into a struct makes sense. >> >> This would of course mean having to define an equivalent intermediate >> struct to support typemapping between TextureInfo and the native >> cc::CopyOutputResult fields, but its definition could live directly in the >> traits h/cc since no user code will ever touch it. >> > > OK, I'm not sure if it's worth making things more complex if you only make > mojo objects out of C++ objects and those define the behaviour correctly? I > wonder what approach to this is normal in mojo, does it try to be > authoritative on what's a valid object? > > >> >> >>> In the empty case mojo can make CopyOutputResult optional if that makes >>> sense but the client side gets an empty CopyOutputResult eventually, cuz >>> that is what callbacks receive. >>> >>> >>>> >>>> https://codereview.chromium.org/2686833003/ >>>> >>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/BUILD.gn File cc/ipc/BUILD.gn (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/BUILD.gn#newcode42 cc/ipc/BUILD.gn:42: "texture_mailbox_releaser.mojom", On 2017/02/10 19:10:07, Saman Sami wrote: > On 2017/02/10 18:32:50, danakj wrote: > > why isn't this part of "interfaces" below? > > because struct traits depending on interfaces creates a cycle. Can you leave a comment here explaining this is a helper interface used by struct traits? And maybe put it lower in the file so people don't cargo-cult into it as easily? https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:40: static SkBitmap* null_bitmap = new SkBitmap(); On 2017/02/10 19:10:07, Saman Sami wrote: > On 2017/02/10 18:32:51, danakj wrote: > > curious why do we need this, given that bitmap() is an optional field? the > > return type here seems to imply its not optional > > SkBitmap is nullable (The struct traits has IsNull and SetToNull). That's all is > needed to use the ? notation. See > https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap_skbitmap_s... If there is IsNull() would this ever be called when result->bitmap_ is not present? Could we DCHECK instead?
On Fri, Feb 10, 2017 at 2:33 PM, Ken Rockot <rockot@chromium.org> wrote: > For any validation beyond what can be expressed in mojom IDL (e.g. making > sure non-nullable things aren't null, etc), typemapping is required, and it > falls on StructTraits to validate. We exploit this to get reliable > validation logic in place without yet having support for its expression in > mojom directly (e.g. we enforce max URL length when deserializing a > url.mojom.Url as a GURL). > > One can always build a (non-chromium) target which doesn't apply any given > typemap and thus doesn't get the relevant StructTraits' validaiton logic. > In general this is OK because we don't anticipate using anyone C++ bindings > for without also using the chromium or blink typemap configurations. > > In this case however, I kind of think it makes sense for the grouping to > be made explicit in mojom simply because it is, in fact, something which > can be expressed in mojom. I'm not sure it really adds significant > complexity to the traits, but I'll defer to you folks on this decision. > I think I'd need to see it, or a pseudocode, to judge the complexity. I'm open to it if you think it's reasonable samans. > > On Fri, Feb 10, 2017 at 11:06 AM, <danakj@chromium.org> wrote: > >> On Fri, Feb 10, 2017 at 2:05 PM, Ken Rockot <rockot@chromium.org> wrote: >> >>> >>> >>> On Fri, Feb 10, 2017 at 11:00 AM, <danakj@chromium.org> wrote: >>> >>>> >>>> >>>> On Fri, Feb 10, 2017 at 1:56 PM, <rockot@chromium.org> wrote: >>>> >>>>> >>>>> https://codereview.chromium.org/2686833003/diff/280001/cc/ip >>>>> c/copy_output_result.mojom >>>>> File cc/ipc/copy_output_result.mojom (right): >>>>> >>>>> https://codereview.chromium.org/2686833003/diff/280001/cc/ip >>>>> c/copy_output_result.mojom#newcode17 >>>>> cc/ipc/copy_output_result.mojom:17: TextureMailboxReleaser? releaser; >>>>> On 2017/02/10 at 18:32:50, danakj wrote: >>>>> > If this is optional, TextureMailbox should be too, and they should >>>>> both be present when one is present. Can you express that in mojo? For >>>>> that matter bitmap and texture_mailbox shouldn't both be present >>>>> either.. >>>>> >>>>> The way to express this would be to group them: >>>>> >>>>> struct TextureInfo { >>>>> skia.mojom.Bitmap bitmap; >>>>> TextureMailbox texture_mailbox; >>>>> TextureMailboxReleaser releaser; >>>>> }; >>>>> >>>>> struct CopyOutputResult { >>>>> gfx.mojom.Size size; >>>>> TextureInfo? texture_info; >>>>> }; >>>>> >>>>> >>>>> But then, if there's no bitmap, mailbox, or releaser, is there really a >>>>> CopyOutputResult at all? Could we just make none of the fields >>>>> optional, >>>>> and have messages which reply with "CopyOutputResult" instead reply >>>>> with >>>>> "CopyOutputResult?" ? >>>>> >>>> >>>> There can be one of: >>>> - No Bitmap, Texture, or SingleReleaseCallback (empty result) >>>> - Bitmap >>>> - Texture + SingleReleaseCallback >>>> >>> >>> Ah, got it. So in that case grouping just TextureMailbox and >>> TextureMailboxReleaser into a struct makes sense. >>> >>> This would of course mean having to define an equivalent intermediate >>> struct to support typemapping between TextureInfo and the native >>> cc::CopyOutputResult fields, but its definition could live directly in the >>> traits h/cc since no user code will ever touch it. >>> >> >> OK, I'm not sure if it's worth making things more complex if you only >> make mojo objects out of C++ objects and those define the behaviour >> correctly? I wonder what approach to this is normal in mojo, does it try to >> be authoritative on what's a valid object? >> >> >>> >>> >>>> In the empty case mojo can make CopyOutputResult optional if that makes >>>> sense but the client side gets an empty CopyOutputResult eventually, cuz >>>> that is what callbacks receive. >>>> >>>> >>>>> >>>>> https://codereview.chromium.org/2686833003/ >>>>> >>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:82: base::Bind(Release, base::Passed(&releaser))); On 2017/02/10 19:10:07, Saman Sami wrote: > On 2017/02/10 18:32:51, danakj wrote: > > you can bind directly to TextureMailboxReleaserPtr::Release here, you don't > need > > the intermediate Release static method? > > TextureMailboxReleaserPtr does not have a Release method. It has the -> operator > which returns the TextureMailboxReleaserProxy that it owns. Ohh, wild. ok :)
If others are happy with this lgtm. I love this! You've helped establish an idiom of how to pass callbacks in structs over IPC! :-) More comments though would be helpful because this is kind of subtle on first glance. https://codereview.chromium.org/2686833003/diff/320001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/320001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:11: class TextureMailboxReleaserImpl : public cc::mojom::TextureMailboxReleaser { nit: Please add a comment about where this code runs, and what its lifetime is...in the display compositor, and lives as long as a client holds on to the MessagePipe (StrongBinding), https://codereview.chromium.org/2686833003/diff/320001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:26: void Release(cc::mojom::TextureMailboxReleaserPtr ptr, nit: Please a comment about where this is used: in the client and bound to a SingleReleaseCallback.
The CQ bit was checked by samans@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2686833003/diff/340001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/340001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:60: /* this would be deleted https://codereview.chromium.org/2686833003/diff/340001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:93: auto releaser = data.TakeReleaser<cc::mojom::TextureMailboxReleaserPtr>(); This won't work right? It should be using the Read() method defined below? https://codereview.chromium.org/2686833003/diff/340001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.h (right): https://codereview.chromium.org/2686833003/diff/340001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.h:31: static bool IsNull(const std::unique_ptr<cc::CopyOutputResult>& result) { What do these have to do with having the sub type?
The CQ bit was checked by samans@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...
tsepez@: Please review mojo. danakj@: Please review all files.
https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:40: static SkBitmap* null_bitmap = new SkBitmap(); On 2017/02/10 22:51:03, danakj wrote: > On 2017/02/10 19:10:07, Saman Sami wrote: > > On 2017/02/10 18:32:51, danakj wrote: > > > curious why do we need this, given that bitmap() is an optional field? the > > > return type here seems to imply its not optional > > > > SkBitmap is nullable (The struct traits has IsNull and SetToNull). That's all > is > > needed to use the ? notation. See > > > https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap_skbitmap_s... > > If there is IsNull() would this ever be called when result->bitmap_ is not > present? Could we DCHECK instead? Still wondering this https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:26: // If the client fails to call Release, we should do it ourselves because This should be a DCHECK instead I think, the same as SingleReleaseCallback is. Or just do nothing and let the SingleReleaseCallback DCHECK. https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:34: DCHECK(release_callback_) The SingleReleaseCallback already does this why double it? https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:105: if (!!result->release_callback_ != result->texture_mailbox_.IsValid()) you could just if (!releaser) return false above right? That's the same thing?
https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:40: static SkBitmap* null_bitmap = new SkBitmap(); On 2017/02/13 20:50:16, danakj wrote: > On 2017/02/10 22:51:03, danakj wrote: > > On 2017/02/10 19:10:07, Saman Sami wrote: > > > On 2017/02/10 18:32:51, danakj wrote: > > > > curious why do we need this, given that bitmap() is an optional field? the > > > > return type here seems to imply its not optional > > > > > > SkBitmap is nullable (The struct traits has IsNull and SetToNull). That's > all > > is > > > needed to use the ? notation. See > > > > > > https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap_skbitmap_s... > > > > If there is IsNull() would this ever be called when result->bitmap_ is not > > present? Could we DCHECK instead? > > Still wondering this Sorry I missed this one. I don't think IsNull replaces the getter. After all, mojo needs to first get the element and then check if it's null or not. https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:26: // If the client fails to call Release, we should do it ourselves because On 2017/02/13 20:50:16, danakj wrote: > This should be a DCHECK instead I think, the same as SingleReleaseCallback is. > Or just do nothing and let the SingleReleaseCallback DCHECK. I'm not sure we want the display compositor to crash when some other process fails to call the SingleReleaseCallback? https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:34: DCHECK(release_callback_) On 2017/02/13 20:50:16, danakj wrote: > The SingleReleaseCallback already does this why double it? SingleReleaseCallback doesn't do anything because we reset it below. I think this DCHECK is more detailed than what unique_ptr provides. We reset the release callback because we want to know whether it's called or not because if it's not we call it in the destructor. https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:105: if (!!result->release_callback_ != result->texture_mailbox_.IsValid()) On 2017/02/13 20:50:16, danakj wrote: > you could just if (!releaser) return false above right? That's the same thing? What if the result is bitmap?
lgtm
https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:40: static SkBitmap* null_bitmap = new SkBitmap(); On 2017/02/13 21:03:01, Saman Sami wrote: > On 2017/02/13 20:50:16, danakj wrote: > > On 2017/02/10 22:51:03, danakj wrote: > > > On 2017/02/10 19:10:07, Saman Sami wrote: > > > > On 2017/02/10 18:32:51, danakj wrote: > > > > > curious why do we need this, given that bitmap() is an optional field? > the > > > > > return type here seems to imply its not optional > > > > > > > > SkBitmap is nullable (The struct traits has IsNull and SetToNull). That's > > all > > > is > > > > needed to use the ? notation. See > > > > > > > > > > https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap_skbitmap_s... > > > > > > If there is IsNull() would this ever be called when result->bitmap_ is not > > > present? Could we DCHECK instead? > > > > Still wondering this > > Sorry I missed this one. I don't think IsNull replaces the getter. After all, > mojo needs to first get the element and then check if it's null or not. Oh... it's rather unfortunate you have to deserialize before asking if the field is present. That's a lot of wasted overhead for optionally-missing fields. https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:26: // If the client fails to call Release, we should do it ourselves because On 2017/02/13 21:03:01, Saman Sami wrote: > On 2017/02/13 20:50:16, danakj wrote: > > This should be a DCHECK instead I think, the same as SingleReleaseCallback is. > > Or just do nothing and let the SingleReleaseCallback DCHECK. > > I'm not sure we want the display compositor to crash when some other process > fails to call the SingleReleaseCallback? Oh this is on the other process, okay. Ya I agree. I thought this is on the client, my bad. https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:34: DCHECK(release_callback_) On 2017/02/13 21:03:01, Saman Sami wrote: > On 2017/02/13 20:50:16, danakj wrote: > > The SingleReleaseCallback already does this why double it? > > SingleReleaseCallback doesn't do anything because we reset it below. I think > this DCHECK is more detailed than what unique_ptr provides. We reset the release > callback because we want to know whether it's called or not because if it's not > we call it in the destructor. Oh I was thinking if the reset() isnt there, but ya this is on the service side so seems good to deal with. But this would DCHECK now if the client misbehaved. We should just do nothing in this case. https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:105: if (!!result->release_callback_ != result->texture_mailbox_.IsValid()) On 2017/02/13 21:03:01, Saman Sami wrote: > On 2017/02/13 20:50:16, danakj wrote: > > you could just if (!releaser) return false above right? That's the same thing? > > What if the result is bitmap? Oh ya ok. This is a nit, but I think it'd maybe be nicer to use the bool values you have here already to check this rather than going thru result. We should also reject both bitmap & texture.
The CQ bit was checked by samans@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...
PTAL https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:40: static SkBitmap* null_bitmap = new SkBitmap(); On 2017/02/13 22:43:17, danakj wrote: > On 2017/02/13 21:03:01, Saman Sami wrote: > > On 2017/02/13 20:50:16, danakj wrote: > > > On 2017/02/10 22:51:03, danakj wrote: > > > > On 2017/02/10 19:10:07, Saman Sami wrote: > > > > > On 2017/02/10 18:32:51, danakj wrote: > > > > > > curious why do we need this, given that bitmap() is an optional field? > > the > > > > > > return type here seems to imply its not optional > > > > > > > > > > SkBitmap is nullable (The struct traits has IsNull and SetToNull). > That's > > > all > > > > is > > > > > needed to use the ? notation. See > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap_skbitmap_s... > > > > > > > > If there is IsNull() would this ever be called when result->bitmap_ is not > > > > present? Could we DCHECK instead? > > > > > > Still wondering this > > > > Sorry I missed this one. I don't think IsNull replaces the getter. After all, > > mojo needs to first get the element and then check if it's null or not. > > Oh... it's rather unfortunate you have to deserialize before asking if the field > is present. That's a lot of wasted overhead for optionally-missing fields. This method doesn't do any deserialization. It just returns the bitmap from the CopyOutputResult. (And returns a const ref actually. Pretty efficient.) https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:34: DCHECK(release_callback_) On 2017/02/13 22:43:17, danakj wrote: > On 2017/02/13 21:03:01, Saman Sami wrote: > > On 2017/02/13 20:50:16, danakj wrote: > > > The SingleReleaseCallback already does this why double it? > > > > SingleReleaseCallback doesn't do anything because we reset it below. I think > > this DCHECK is more detailed than what unique_ptr provides. We reset the > release > > callback because we want to know whether it's called or not because if it's > not > > we call it in the destructor. > > Oh I was thinking if the reset() isnt there, but ya this is on the service side > so seems good to deal with. But this would DCHECK now if the client misbehaved. > We should just do nothing in this case. That's right. I'll get rid of the DCHECK. https://codereview.chromium.org/2686833003/diff/380001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:105: if (!!result->release_callback_ != result->texture_mailbox_.IsValid()) On 2017/02/13 22:43:17, danakj wrote: > On 2017/02/13 21:03:01, Saman Sami wrote: > > On 2017/02/13 20:50:16, danakj wrote: > > > you could just if (!releaser) return false above right? That's the same > thing? > > > > What if the result is bitmap? > > Oh ya ok. This is a nit, but I think it'd maybe be nicer to use the bool values > you have here already to check this rather than going thru result. > > We should also reject both bitmap & texture. Added the check for both bitmap and texture.
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_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 samans@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/280001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:40: static SkBitmap* null_bitmap = new SkBitmap(); On 2017/02/13 23:23:43, Saman Sami wrote: > On 2017/02/13 22:43:17, danakj wrote: > > On 2017/02/13 21:03:01, Saman Sami wrote: > > > On 2017/02/13 20:50:16, danakj wrote: > > > > On 2017/02/10 22:51:03, danakj wrote: > > > > > On 2017/02/10 19:10:07, Saman Sami wrote: > > > > > > On 2017/02/10 18:32:51, danakj wrote: > > > > > > > curious why do we need this, given that bitmap() is an optional > field? > > > the > > > > > > > return type here seems to imply its not optional > > > > > > > > > > > > SkBitmap is nullable (The struct traits has IsNull and SetToNull). > > That's > > > > all > > > > > is > > > > > > needed to use the ? notation. See > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap_skbitmap_s... > > > > > > > > > > If there is IsNull() would this ever be called when result->bitmap_ is > not > > > > > present? Could we DCHECK instead? > > > > > > > > Still wondering this > > > > > > Sorry I missed this one. I don't think IsNull replaces the getter. After > all, > > > mojo needs to first get the element and then check if it's null or not. > > > > Oh... it's rather unfortunate you have to deserialize before asking if the > field > > is present. That's a lot of wasted overhead for optionally-missing fields. > > This method doesn't do any deserialization. It just returns the bitmap from the > CopyOutputResult. (And returns a const ref actually. Pretty efficient.) I guess so, I just keep looking at this malloc :p https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result.mojom (right): https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... cc/ipc/copy_output_result.mojom:14: gfx.mojom.Size size; This should be optional, it's only used for texture results: https://cs.chromium.org/chromium/src/cc/output/copy_output_result.h?rcl=c8e27... https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:83: auto result = cc::CopyOutputResult::CreateEmptyResult(); Thanks this LG. What I think gets very confusing is that this is a friend of CopyOutputResult, so it's building the private members, instead of using the constructors. If it did the latter, then it would be much more clear which fields can go together and which can not. And we'd never end up with a CopyOutputResult with HasBitmap() and HasTexture() true because there'd be no way to construct such a thing. Why did you choose to make this a friend instead of collecting the various fields locally here and then invoking the appropriate constructor?
https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result.mojom (right): https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... cc/ipc/copy_output_result.mojom:14: gfx.mojom.Size size; On 2017/02/14 17:49:10, danakj wrote: > This should be optional, it's only used for texture results: > https://cs.chromium.org/chromium/src/cc/output/copy_output_result.h?rcl=c8e27... Acknowledged. https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:83: auto result = cc::CopyOutputResult::CreateEmptyResult(); On 2017/02/14 17:49:11, danakj wrote: > Thanks this LG. What I think gets very confusing is that this is a friend of > CopyOutputResult, so it's building the private members, instead of using the > constructors. If it did the latter, then it would be much more clear which > fields can go together and which can not. And we'd never end up with a > CopyOutputResult with HasBitmap() and HasTexture() true because there'd be no > way to construct such a thing. > > Why did you choose to make this a friend instead of collecting the various > fields locally here and then invoking the appropriate constructor? It has to be a friend so we can return the private members in the getters. Using the constructors might help a little bit but not too much. Remember that the object that we receive can be totally invalid and we still need the checks below. But I'll give it a shot to see how it can improve my code.
On Tue, Feb 14, 2017 at 5:17 PM, <samans@chromium.org> wrote: > > https://codereview.chromium.org/2686833003/diff/400001/cc/ > ipc/copy_output_result.mojom > File cc/ipc/copy_output_result.mojom (right): > > https://codereview.chromium.org/2686833003/diff/400001/cc/ > ipc/copy_output_result.mojom#newcode14 > cc/ipc/copy_output_result.mojom:14: gfx.mojom.Size size; > On 2017/02/14 17:49:10, danakj wrote: > > This should be optional, it's only used for texture results: > > > https://cs.chromium.org/chromium/src/cc/output/copy_output_result.h?rcl= > c8e27a009a07233002d0397cf8c0134e09b572f5&l=38 > > Acknowledged. > > https://codereview.chromium.org/2686833003/diff/400001/cc/ > ipc/copy_output_result_struct_traits.cc > File cc/ipc/copy_output_result_struct_traits.cc (right): > > https://codereview.chromium.org/2686833003/diff/400001/cc/ > ipc/copy_output_result_struct_traits.cc#newcode83 > cc/ipc/copy_output_result_struct_traits.cc:83: auto result = > cc::CopyOutputResult::CreateEmptyResult(); > On 2017/02/14 17:49:11, danakj wrote: > > Thanks this LG. What I think gets very confusing is that this is a > friend of > > CopyOutputResult, so it's building the private members, instead of > using the > > constructors. If it did the latter, then it would be much more clear > which > > fields can go together and which can not. And we'd never end up with a > > CopyOutputResult with HasBitmap() and HasTexture() true because > there'd be no > > way to construct such a thing. > > > > Why did you choose to make this a friend instead of collecting the > various > > fields locally here and then invoking the appropriate constructor? > > It has to be a friend so we can return the private members in the > getters. Using the constructors might help a little bit but not too > much. Remember that the object that we receive can be totally invalid > and we still need the checks below. But I'll give it a shot to see how > it can improve my code. > Yeh I think that's really my idea, if you go thru constructors then it'll force us to consider what's valid. An invalid set of inputs won't compile, and we don't have to write out what's valid twice (as constructors and here). Thanks for giving it a try! > > https://codereview.chromium.org/2686833003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by samans@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 checked by samans@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 checked by samans@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 checked by samans@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 checked by samans@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...
LGTM https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... File cc/ipc/copy_output_result.mojom (right): https://codereview.chromium.org/2686833003/diff/400001/cc/ipc/copy_output_res... cc/ipc/copy_output_result.mojom:14: gfx.mojom.Size size; On 2017/02/14 22:17:04, Saman Sami wrote: > On 2017/02/14 17:49:10, danakj wrote: > > This should be optional, it's only used for texture results: > > > https://cs.chromium.org/chromium/src/cc/output/copy_output_result.h?rcl=c8e27... > > Acknowledged. Looks like this shouldn't be ? cuz ? refers to a magic null value which Size doesn't have, not to the field being optional. https://codereview.chromium.org/2686833003/diff/440002/cc/ipc/copy_output_res... File cc/ipc/copy_output_result_struct_traits.cc (right): https://codereview.chromium.org/2686833003/diff/440002/cc/ipc/copy_output_res... cc/ipc/copy_output_result_struct_traits.cc:108: // Empty result comments are sentences, needs periods. https://codereview.chromium.org/2686833003/diff/440002/cc/ipc/texture_mailbox... File cc/ipc/texture_mailbox_struct_traits.h (right): https://codereview.chromium.org/2686833003/diff/440002/cc/ipc/texture_mailbox... cc/ipc/texture_mailbox_struct_traits.h:19: return !input.IsTexture(); Uh.. so. This is where this nullable thing gets really weird. !IsTexture doesnt mean it's null. Maybe this is kinda right cuz we don't send software ones.. yet, but it's awkward.. I wish we did not use nullable mojo fields at all.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@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 checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, tsepez@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2686833003/#ps530001 (title: "c")
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": 530001, "attempt_start_ts": 1487124639347510,
"parent_rev": "b26d145864fbbe1b6990c6d9164eb3a5e4287401", "commit_rev":
"05612a0cb0feafb73ec78c2aebef4c1af797380f"}
Message was sent while issue was closed.
Description was changed from ========== CopyOutputResult must have a working release callback when received over mojo Right now, CopyOutputResult when received over mojo has an empty release callback. This CL introduces TextureMailboxReleaser, which is a mojo interface that has a Release method with the same signature as ReleaseCallback. On the sender side, the ReleaseCallback of the CopyOutputResult is retained in a TextureMailboxReleaserImpl and a TextureMailboxReleaserPtr will be sent to the receiver. When the receiver calls the Release method, the sender calls the release callback. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== CopyOutputResult must have a working release callback when received over mojo Right now, CopyOutputResult when received over mojo has an empty release callback. This CL introduces TextureMailboxReleaser, which is a mojo interface that has a Release method with the same signature as ReleaseCallback. On the sender side, the ReleaseCallback of the CopyOutputResult is retained in a TextureMailboxReleaserImpl and a TextureMailboxReleaserPtr will be sent to the receiver. When the receiver calls the Release method, the sender calls the release callback. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2686833003 Cr-Commit-Position: refs/heads/master@{#450558} Committed: https://chromium.googlesource.com/chromium/src/+/05612a0cb0feafb73ec78c2aebef... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:530001) as https://chromium.googlesource.com/chromium/src/+/05612a0cb0feafb73ec78c2aebef... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
