Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2676353002:
MojoCompositorFrameSinkPrivate should support copy requests (Closed)
|
Created:
3 years, 10 months ago by Saman Sami Modified:
3 years, 10 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, xjz+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, cc-bugs_chromium.org, darin (slow to review), miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMojoCompositorFrameSinkPrivate should support copy requests
Copy requests must be supported over mojo because some clients of
SurfaceFactory use them and SurfaceFactory will move to a separate
process. This CL adds methods for sending copy requests to
MojoCompositorFrameSinkPrivate.
BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2676353002
Cr-Commit-Position: refs/heads/master@{#451237}
Committed: https://chromium.googlesource.com/chromium/src/+/b4abfddd91ae9f4088f1544ec7ca8e95afbe7229
Patch Set 1 #
Total comments: 7
Patch Set 2 : c #Patch Set 3 : c #Patch Set 4 : c #
Total comments: 14
Patch Set 5 : Get rid of ReleaseCopyOfSurface #Patch Set 6 : Cleanup #Patch Set 7 : Cleanup #
Total comments: 2
Patch Set 8 : Remove callback #
Messages
Total messages: 77 (46 generated)
Description was changed from ========== MojoCompositorFrameSink should support copy requests BUG=672071 ========== to ========== MojoCompositorFrameSink should support copy requests 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...
Description was changed from ========== MojoCompositorFrameSink should support copy requests BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== MojoCompositorFrameSink should support copy requests BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
samans@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... File cc/ipc/mojo_compositor_frame_sink.mojom (left): https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... cc/ipc/mojo_compositor_frame_sink.mojom:50: // TODO(fsamuel): ReadbackBitmap API would be useful here. Delete this? https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... cc/ipc/mojo_compositor_frame_sink.mojom:57: DeleteMailbox(gpu.mojom.Mailbox mailbox, gpu.mojom.SyncToken sync_token, bool is_lost); This is way too generic, in my opinion. Why do we need this?
samans@chromium.org changed reviewers: + danakj@chromium.org
danakj@: Please review all files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... cc/ipc/mojo_compositor_frame_sink.mojom:57: DeleteMailbox(gpu.mojom.Mailbox mailbox, gpu.mojom.SyncToken sync_token, bool is_lost); On 2017/02/06 17:13:48, Fady Samuel wrote: > This is way too generic, in my opinion. Why do we need this? Same, CFS can transport resources but it doesn't manage lifetimes. Textures are deleted/mailboxes are dereffed thru a GL context.
https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... cc/ipc/mojo_compositor_frame_sink.mojom:57: DeleteMailbox(gpu.mojom.Mailbox mailbox, gpu.mojom.SyncToken sync_token, bool is_lost); On 2017/02/06 19:18:51, danakj (slow) wrote: > On 2017/02/06 17:13:48, Fady Samuel wrote: > > This is way too generic, in my opinion. Why do we need this? > > Same, CFS can transport resources but it doesn't manage lifetimes. Textures are > deleted/mailboxes are dereffed thru a GL context. This sync token indicates when it's safe to delete this mailbox, right? Can The display compositor allocate the synctoken instead and pass it to the client in CopyOutputResult?
https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... cc/ipc/mojo_compositor_frame_sink.mojom:57: DeleteMailbox(gpu.mojom.Mailbox mailbox, gpu.mojom.SyncToken sync_token, bool is_lost); On 2017/02/06 19:18:51, danakj (slow) wrote: > On 2017/02/06 17:13:48, Fady Samuel wrote: > > This is way too generic, in my opinion. Why do we need this? > > Same, CFS can transport resources but it doesn't manage lifetimes. Textures are > deleted/mailboxes are dereffed thru a GL context. Can you explain how it should be? I'm basically trying to mimic how copy requests work at the moment. The requester has to run a callback to delete the mailbox (called release_callback_ in CopyOutputResult). Since the callback would be tricky with mojo, I added a method to the mojo interface which acts like that callback.
https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... cc/ipc/mojo_compositor_frame_sink.mojom:57: DeleteMailbox(gpu.mojom.Mailbox mailbox, gpu.mojom.SyncToken sync_token, bool is_lost); On 2017/02/06 20:04:00, Saman Sami wrote: > On 2017/02/06 19:18:51, danakj (slow) wrote: > > On 2017/02/06 17:13:48, Fady Samuel wrote: > > > This is way too generic, in my opinion. Why do we need this? > > > > Same, CFS can transport resources but it doesn't manage lifetimes. Textures > are > > deleted/mailboxes are dereffed thru a GL context. > > Can you explain how it should be? I'm basically trying to mimic how copy > requests work at the moment. The requester has to run a callback to delete the > mailbox (called release_callback_ in CopyOutputResult). Since the callback would > be tricky with mojo, I added a method to the mojo interface which acts like that > callback. I see, just call this ReleaseMailbox then. It doesn't delete, it tells the service that it is done with it and is releasing ownership of its contents. Delete implies something else altogether and may be what the service does, or not.
https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/1/cc/ipc/mojo_compositor_fram... cc/ipc/mojo_compositor_frame_sink.mojom:57: DeleteMailbox(gpu.mojom.Mailbox mailbox, gpu.mojom.SyncToken sync_token, bool is_lost); On 2017/02/06 20:09:15, danakj (slow) wrote: > On 2017/02/06 20:04:00, Saman Sami wrote: > > On 2017/02/06 19:18:51, danakj (slow) wrote: > > > On 2017/02/06 17:13:48, Fady Samuel wrote: > > > > This is way too generic, in my opinion. Why do we need this? > > > > > > Same, CFS can transport resources but it doesn't manage lifetimes. Textures > > are > > > deleted/mailboxes are dereffed thru a GL context. > > > > Can you explain how it should be? I'm basically trying to mimic how copy > > requests work at the moment. The requester has to run a callback to delete the > > mailbox (called release_callback_ in CopyOutputResult). Since the callback > would > > be tricky with mojo, I added a method to the mojo interface which acts like > that > > callback. > > I see, just call this ReleaseMailbox then. It doesn't delete, it tells the > service that it is done with it and is releasing ownership of its contents. > Delete implies something else altogether and may be what the service does, or > not. ReleaseMailboxForCopyOutputResult might work too if that is too general as this is a widely used API. Or can the cc.mojom.CopyOutputResult provide this somehow to namespace it?
samans@chromium.org changed reviewers: + tsepez@chromium.org
tsepez@: Please review mojo
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...
Can someone walk me through how we ensure that we are only reading back pixels that we are entitled to see, and not from some other frame?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/06 21:36:51, Tom Sepez wrote: > Can someone walk me through how we ensure that we are only reading back pixels > that we are entitled to see, and not from some other frame? The CompositorFrameSink will send back a copy of its own surface only, to which the client has submitted frames. So bascically the client can only see copies of the frames it has submitted.
On 2017/02/06 21:36:51, Tom Sepez wrote: > Can someone walk me through how we ensure that we are only reading back pixels > that we are entitled to see, and not from some other frame? The CompositorFrameSink will send back a copy of its own surface only, to which the client has submitted frames. So bascically the client can only see copies of the frames it has submitted.
mojom lgtm
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 danakj@: Please review all files. I moved the methods to MojoCompositorFrameSinkPrivate for security concerns.
Description was changed from ========== MojoCompositorFrameSink should support copy requests BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== MojoCompositorFrameSinkPrivate should support copy requests Copy requests must be supported over mojo because some clients of SurfaceFactory use them and SurfaceFactory will move to a separate process. This CL adds method for sending copy requests to MojoCompositorFrameSinkPrivate. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== MojoCompositorFrameSinkPrivate should support copy requests Copy requests must be supported over mojo because some clients of SurfaceFactory use them and SurfaceFactory will move to a separate process. This CL adds method for sending copy requests to MojoCompositorFrameSinkPrivate. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== MojoCompositorFrameSinkPrivate should support copy requests Copy requests must be supported over mojo because some clients of SurfaceFactory use them and SurfaceFactory will move to a separate process. This CL adds methods for sending copy requests to MojoCompositorFrameSinkPrivate. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
lgtm
https://codereview.chromium.org/2676353002/diff/60001/cc/ipc/mojo_compositor_... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/60001/cc/ipc/mojo_compositor_... cc/ipc/mojo_compositor_frame_sink.mojom:89: // If the copy returned by RequestCopyOfSurface is a texture, this method This might be off-topic but what if it's a bitmap, do we use shared memory or do we now ship a lot of pixels over ipc? What does https://cs.chromium.org/chromium/src/cc/ipc/copy_output_result.mojom?rcl=5fdb... do? Should that be a SharedBitmap? https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... File cc/output/copy_output_request.h (right): https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... cc/output/copy_output_request.h:86: void set_result_callback(CopyOutputRequestCallback result_callback) { This looks like what CreateRelayRequest is for, to make a new CopyOutputRequest with a new callback? https://codereview.chromium.org/2676353002/diff/60001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2676353002/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:209: callback.Run(std::move(result)); The result here should have a release callback to calls to ReleaseCopyOfSurface. It looks like you want a CreateRelayResult similar to CreateRelayRequest? It can move the contents of the original from it store a new release callback. It's super not valid to give back a result without the release callback.
https://codereview.chromium.org/2676353002/diff/60001/cc/ipc/mojo_compositor_... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/60001/cc/ipc/mojo_compositor_... cc/ipc/mojo_compositor_frame_sink.mojom:89: // If the copy returned by RequestCopyOfSurface is a texture, this method On 2017/02/06 23:02:12, danakj (slow) wrote: > This might be off-topic but what if it's a bitmap, do we use shared memory or do > we now ship a lot of pixels over ipc? What does > https://cs.chromium.org/chromium/src/cc/ipc/copy_output_result.mojom?rcl=5fdb... > do? Should that be a SharedBitmap? We serialize the bitmap and send it over IPC. I'm not sure how to use shared bitmap. CopyOutputResult only has a SkBitmap. That might need to change I guess? https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... File cc/output/copy_output_request.h (right): https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... cc/output/copy_output_request.h:86: void set_result_callback(CopyOutputRequestCallback result_callback) { On 2017/02/06 23:02:12, danakj (slow) wrote: > This looks like what CreateRelayRequest is for, to make a new CopyOutputRequest > with a new callback? Yeah, you're right. I can fix that, but do we have to make an unnecessary copy? https://codereview.chromium.org/2676353002/diff/60001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2676353002/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:209: callback.Run(std::move(result)); On 2017/02/06 23:02:12, danakj (slow) wrote: > The result here should have a release callback to calls to ReleaseCopyOfSurface. > It looks like you want a CreateRelayResult similar to CreateRelayRequest? It can > move the contents of the original from it store a new release callback. > > It's super not valid to give back a result without the release callback. I can't set the release callback here. Remember that it will be lost over mojo, so there is no point. The client of MojoCompositorFrameSinkPrivate is required to set the release callback appropriately.
https://codereview.chromium.org/2676353002/diff/60001/cc/ipc/mojo_compositor_... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/60001/cc/ipc/mojo_compositor_... cc/ipc/mojo_compositor_frame_sink.mojom:89: // If the copy returned by RequestCopyOfSurface is a texture, this method On 2017/02/06 23:09:54, Saman Sami wrote: > On 2017/02/06 23:02:12, danakj (slow) wrote: > > This might be off-topic but what if it's a bitmap, do we use shared memory or > do > > we now ship a lot of pixels over ipc? What does > > > https://cs.chromium.org/chromium/src/cc/ipc/copy_output_result.mojom?rcl=5fdb... > > do? Should that be a SharedBitmap? > > We serialize the bitmap and send it over IPC. I'm not sure how to use shared > bitmap. CopyOutputResult only has a SkBitmap. That might need to change I guess? Yeh it would have to. We should do that cuz copying pixels is slow. https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... File cc/output/copy_output_request.h (right): https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... cc/output/copy_output_request.h:86: void set_result_callback(CopyOutputRequestCallback result_callback) { On 2017/02/06 23:09:54, Saman Sami wrote: > On 2017/02/06 23:02:12, danakj (slow) wrote: > > This looks like what CreateRelayRequest is for, to make a new > CopyOutputRequest > > with a new callback? > > Yeah, you're right. I can fix that, but do we have to make an unnecessary copy? It's not a copy of much, what are you concerned for? And it keeps the internals private. https://codereview.chromium.org/2676353002/diff/60001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2676353002/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:209: callback.Run(std::move(result)); On 2017/02/06 23:09:54, Saman Sami wrote: > On 2017/02/06 23:02:12, danakj (slow) wrote: > > The result here should have a release callback to calls to > ReleaseCopyOfSurface. > > It looks like you want a CreateRelayResult similar to CreateRelayRequest? It > can > > move the contents of the original from it store a new release callback. > > > > It's super not valid to give back a result without the release callback. > > I can't set the release callback here. Remember that it will be lost over mojo, > so there is no point. The client of MojoCompositorFrameSinkPrivate is required > to set the release callback appropriately. There should be a release callback made when it comes out of mojo too.
https://codereview.chromium.org/2676353002/diff/60001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2676353002/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:209: callback.Run(std::move(result)); On 2017/02/06 23:24:08, danakj (slow) wrote: > On 2017/02/06 23:09:54, Saman Sami wrote: > > On 2017/02/06 23:02:12, danakj (slow) wrote: > > > The result here should have a release callback to calls to > > ReleaseCopyOfSurface. > > > It looks like you want a CreateRelayResult similar to CreateRelayRequest? It > > can > > > move the contents of the original from it store a new release callback. > > > > > > It's super not valid to give back a result without the release callback. > > > > I can't set the release callback here. Remember that it will be lost over > mojo, > > so there is no point. The client of MojoCompositorFrameSinkPrivate is required > > to set the release callback appropriately. > > There should be a release callback made when it comes out of mojo too. And if you don't do this here, how does the texture ever get returned in the current patch? Clients will try to use that callback.
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 to run a CQ dry run
PTAL Note that https://codereview.chromium.org/2686833003/ lands first. https://codereview.chromium.org/2676353002/diff/60001/cc/ipc/mojo_compositor_... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2676353002/diff/60001/cc/ipc/mojo_compositor_... cc/ipc/mojo_compositor_frame_sink.mojom:89: // If the copy returned by RequestCopyOfSurface is a texture, this method On 2017/02/06 23:24:07, danakj wrote: > On 2017/02/06 23:09:54, Saman Sami wrote: > > On 2017/02/06 23:02:12, danakj (slow) wrote: > > > This might be off-topic but what if it's a bitmap, do we use shared memory > or > > do > > > we now ship a lot of pixels over ipc? What does > > > > > > https://cs.chromium.org/chromium/src/cc/ipc/copy_output_result.mojom?rcl=5fdb... > > > do? Should that be a SharedBitmap? > > > > We serialize the bitmap and send it over IPC. I'm not sure how to use shared > > bitmap. CopyOutputResult only has a SkBitmap. That might need to change I > guess? > > Yeh it would have to. We should do that cuz copying pixels is slow. I agree. I created a new bug to track this issue. https://crbug.com/691070 https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... File cc/output/copy_output_request.h (right): https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... cc/output/copy_output_request.h:86: void set_result_callback(CopyOutputRequestCallback result_callback) { On 2017/02/06 23:24:08, danakj wrote: > On 2017/02/06 23:09:54, Saman Sami wrote: > > On 2017/02/06 23:02:12, danakj (slow) wrote: > > > This looks like what CreateRelayRequest is for, to make a new > > CopyOutputRequest > > > with a new callback? > > > > Yeah, you're right. I can fix that, but do we have to make an unnecessary > copy? > > It's not a copy of much, what are you concerned for? And it keeps the internals > private. Pretty much every private member has a setter though?
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/2676353002/diff/60001/cc/output/copy_output_r... File cc/output/copy_output_request.h (right): https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... cc/output/copy_output_request.h:86: void set_result_callback(CopyOutputRequestCallback result_callback) { On 2017/02/13 23:07:37, Saman Sami wrote: > On 2017/02/06 23:24:08, danakj wrote: > > On 2017/02/06 23:09:54, Saman Sami wrote: > > > On 2017/02/06 23:02:12, danakj (slow) wrote: > > > > This looks like what CreateRelayRequest is for, to make a new > > > CopyOutputRequest > > > > with a new callback? > > > > > > Yeah, you're right. I can fix that, but do we have to make an unnecessary > > copy? > > > > It's not a copy of much, what are you concerned for? And it keeps the > internals > > private. > > Pretty much every private member has a setter though? Yes but this the thing that ensures the callback is run, it's passed in the constructor unlike the other things. If you replace the callback now errors and what is getting replied to gets much more complicated. I think the relationship that a CopyOutputRequest is tied to a fixed callback is valuable and this would allow much more difficult to understand things to happen, even if in this case it's not bad. I don't want to see these methods added here for those reasons.
https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... File cc/output/copy_output_request.h (right): https://codereview.chromium.org/2676353002/diff/60001/cc/output/copy_output_r... cc/output/copy_output_request.h:86: void set_result_callback(CopyOutputRequestCallback result_callback) { On 2017/02/13 23:11:15, danakj wrote: > On 2017/02/13 23:07:37, Saman Sami wrote: > > On 2017/02/06 23:24:08, danakj wrote: > > > On 2017/02/06 23:09:54, Saman Sami wrote: > > > > On 2017/02/06 23:02:12, danakj (slow) wrote: > > > > > This looks like what CreateRelayRequest is for, to make a new > > > > CopyOutputRequest > > > > > with a new callback? > > > > > > > > Yeah, you're right. I can fix that, but do we have to make an unnecessary > > > copy? > > > > > > It's not a copy of much, what are you concerned for? And it keeps the > > internals > > > private. > > > > Pretty much every private member has a setter though? > > Yes but this the thing that ensures the callback is run, it's passed in the > constructor unlike the other things. If you replace the callback now errors and > what is getting replied to gets much more complicated. I think the relationship > that a CopyOutputRequest is tied to a fixed callback is valuable and this would > allow much more difficult to understand things to happen, even if in this case > it's not bad. I don't want to see these methods added here for those reasons. This along with the fact there is already a way to do this, with a RelayRequest. This adds new APIs that are not needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2676353002/diff/120001/cc/output/copy_output_... File cc/output/copy_output_request.h (right): https://codereview.chromium.org/2676353002/diff/120001/cc/output/copy_output_... cc/output/copy_output_request.h:77: void set_result_callback(CopyOutputRequestCallback result_callback) { Is this still necessary? Thanks!
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 Note that https://codereview.chromium.org/2700533002/ lands first. https://codereview.chromium.org/2676353002/diff/120001/cc/output/copy_output_... File cc/output/copy_output_request.h (right): https://codereview.chromium.org/2676353002/diff/120001/cc/output/copy_output_... cc/output/copy_output_request.h:77: void set_result_callback(CopyOutputRequestCallback result_callback) { On 2017/02/15 13:32:20, Fady Samuel wrote: > Is this still necessary? Thanks! Not after my other CL lands. I'll get rid of it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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.
LGTM!
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2676353002/#ps140001 (title: "Remove callback")
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 samans@chromium.org
The CQ bit was checked by samans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by samans@chromium.org
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 samans@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 unchecked by samans@chromium.org
The CQ bit was checked by samans@chromium.org
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": 140001, "attempt_start_ts": 1487305643987590,
"parent_rev": "e95bae2c160909c52b01b269f90c63f46ab9039a", "commit_rev":
"b4abfddd91ae9f4088f1544ec7ca8e95afbe7229"}
Message was sent while issue was closed.
Description was changed from ========== MojoCompositorFrameSinkPrivate should support copy requests Copy requests must be supported over mojo because some clients of SurfaceFactory use them and SurfaceFactory will move to a separate process. This CL adds methods for sending copy requests to MojoCompositorFrameSinkPrivate. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== MojoCompositorFrameSinkPrivate should support copy requests Copy requests must be supported over mojo because some clients of SurfaceFactory use them and SurfaceFactory will move to a separate process. This CL adds methods for sending copy requests to MojoCompositorFrameSinkPrivate. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2676353002 Cr-Commit-Position: refs/heads/master@{#451237} Committed: https://chromium.googlesource.com/chromium/src/+/b4abfddd91ae9f4088f1544ec7ca... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/b4abfddd91ae9f4088f1544ec7ca... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
