|
|
Created:
3 years, 7 months ago by Saman Sami Modified:
3 years, 7 months ago CC:
chromium-reviews, cc-bugs_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnforce constant size and device scale factor for surfaces
Surfaces are expected to have constant size and device scale factor but
we don't enforce that. This CL requires a valid SurfaceInfo to be
provided at the time of constructing a surface, and all frames that don't
match the SurfaceInfo will be skipped.
TBR=sadrul@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2848223003
Cr-Commit-Position: refs/heads/master@{#474050}
Committed: https://chromium.googlesource.com/chromium/src/+/5ba3188fa05666ae8d6031706c0963532f9d1457
Patch Set 1 #
Total comments: 6
Patch Set 2 : c #
Total comments: 4
Patch Set 3 : c #Patch Set 4 : c #Patch Set 5 : c #
Total comments: 6
Patch Set 6 : c #
Total comments: 18
Patch Set 7 : c #Patch Set 8 : c #Patch Set 9 : Notify CFSSClient when a frame is rejected #
Total comments: 2
Patch Set 10 : Remove DidReject #Patch Set 11 : Fixed Android #
Total comments: 13
Patch Set 12 : Fixed dchecks #
Total comments: 2
Patch Set 13 : Fix Android #Patch Set 14 : Rebase #Patch Set 15 : Address comments #
Total comments: 4
Patch Set 16 : Address comments #Messages
Total messages: 216 (183 generated)
Description was changed from ========== Skip frames of wrong size in cc::Surface::QueueFrame ========== to ========== Skip frames of wrong size in cc::Surface::QueueFrame CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newc... cc/surfaces/surface.cc:60: device_scale_factor != surface_info_.device_scale_factor()) { Return resources and call the callback too.
https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newc... cc/surfaces/surface.cc:56: if (!frame.render_pass_list.empty()) { Not necessary any more right? https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newc... cc/surfaces/surface.cc:60: device_scale_factor != surface_info_.device_scale_factor()) { I'd love to move this test into a common place because we do it...EVERYWHERE, and we're talking about adding more things to SurfaceInfo. Maybe a CompositorFrame::Matches(...)? Or SurfaceInfo::Matches(...)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_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...
https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface.cc#... cc/surfaces/surface.cc:68: callback.Run(); Return resources too? https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface_info.h File cc/surfaces/surface_info.h (right): https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface_inf... cc/surfaces/surface_info.h:32: bool is_valid() const { return id_.is_valid(); } Why?
https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newc... cc/surfaces/surface.cc:56: if (!frame.render_pass_list.empty()) { On 2017/05/07 18:16:51, Fady Samuel wrote: > Not necessary any more right? Done. https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newc... cc/surfaces/surface.cc:60: device_scale_factor != surface_info_.device_scale_factor()) { On 2017/05/07 18:16:51, Fady Samuel wrote: > I'd love to move this test into a common place because we do it...EVERYWHERE, > and we're talking about adding more things to SurfaceInfo. > > Maybe a CompositorFrame::Matches(...)? Or SurfaceInfo::Matches(...) That'll be nice, yes. https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface.cc#... cc/surfaces/surface.cc:68: callback.Run(); On 2017/05/08 20:21:19, Fady Samuel wrote: > Return resources too? Yeah sure. https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface_info.h File cc/surfaces/surface_info.h (right): https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface_inf... cc/surfaces/surface_info.h:32: bool is_valid() const { return id_.is_valid(); } On 2017/05/08 20:21:19, Fady Samuel wrote: > Why? I need to fix some tests that clash with the original checks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: linux_chromium_asan_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: 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_...)
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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 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_...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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: 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_...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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: 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_...) 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 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: linux_chromium_chromeos_ozone_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 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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by 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: linux_chromium_chromeos_ozone_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...
Description was changed from ========== Skip frames of wrong size in cc::Surface::QueueFrame CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Enforce constant size and device scale factor for surfaces CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
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...
Description was changed from ========== Enforce constant size and device scale factor for surfaces CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and the surface will skip all frames that don't match the SurfaceInfo. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and the surface will skip all frames that don't match the SurfaceInfo. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
PTAL https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newc... cc/surfaces/surface.cc:60: device_scale_factor != surface_info_.device_scale_factor()) { On 2017/05/08 20:40:32, Saman Sami wrote: > On 2017/05/07 18:16:51, Fady Samuel wrote: > > I'd love to move this test into a common place because we do it...EVERYWHERE, > > and we're talking about adding more things to SurfaceInfo. > > > > Maybe a CompositorFrame::Matches(...)? Or SurfaceInfo::Matches(...) > > That'll be nice, yes. Actually, I'm not sure how much that will help. I know we have the comparison for new size vs old size all over the code base, but we rarely have a SurfaceInfo object, perhaps because we usually keep a LocalSurfaceId as opposed to a full SurfaceId which is necessary for SurfaceInfo.
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_...)
https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:135: current_surface_->RunDrawCallback(); Could you please add a comment why we need to do this? Do we do this whenever we replace a surface? https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.h (right): https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.h:84: float device_scale_factor_ = 0.f; Hmm, so it's starting out invalid? Maybe start it out at 1.f? https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:68: DLOG(FATAL) << "Rejected CompositorFrame that violates surface invariants."; Is this necessary? Don't we already check this in CompositorFrameSinkSupport?
https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:135: current_surface_->RunDrawCallback(); On 2017/05/17 00:40:50, Fady Samuel wrote: > Could you please add a comment why we need to do this? Do we do this whenever > we replace a surface? It's already called in the destructor. My bad. https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.h (right): https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.h:84: float device_scale_factor_ = 0.f; On 2017/05/17 00:40:50, Fady Samuel wrote: > Hmm, so it's starting out invalid? Maybe start it out at 1.f? It'll be updated from the metadata though. https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:68: DLOG(FATAL) << "Rejected CompositorFrame that violates surface invariants."; On 2017/05/17 00:40:50, Fady Samuel wrote: > Is this necessary? Don't we already check this in CompositorFrameSinkSupport? We don't. I guess you are confusing it with the check for SurfaceInfo validity?
Description was changed from ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
samans@chromium.org changed reviewers: + danakj@chromium.org
The CQ bit was checked by samans@chromium.org to run a CQ dry run
danakj, please review all files.
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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...
Non-OWNER LGTM for the approach
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:69: reject_frame = true; How will this happen in practice? Why isn't this just a DCHECK?
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 20:49:36, danakj wrote: > How will this happen in practice? Why isn't this just a DCHECK? If the client misbehaves, for example.
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 20:52:08, Saman Sami wrote: > On 2017/05/17 20:49:36, danakj wrote: > > How will this happen in practice? Why isn't this just a DCHECK? > > If the client misbehaves, for example. Can't we reject that at the IPC boundary instead of here?
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 21:03:28, danakj wrote: > On 2017/05/17 20:52:08, Saman Sami wrote: > > On 2017/05/17 20:49:36, danakj wrote: > > > How will this happen in practice? Why isn't this just a DCHECK? > > > > If the client misbehaves, for example. > > Can't we reject that at the IPC boundary instead of here? We don't know the details of the surface at IPC boundary, unfortunately.
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 21:22:50, Saman Sami wrote: > On 2017/05/17 21:03:28, danakj wrote: > > On 2017/05/17 20:52:08, Saman Sami wrote: > > > On 2017/05/17 20:49:36, danakj wrote: > > > > How will this happen in practice? Why isn't this just a DCHECK? > > > > > > If the client misbehaves, for example. > > > > Can't we reject that at the IPC boundary instead of here? > > We don't know the details of the surface at IPC boundary, unfortunately. I guess I am asking how far out we can push it then. This is called by CFS::SubmitCompositorFrameSink. It could query these and reject the frame instead of calling Queue? Repeat up the call stack, how far can it go?
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 21:25:28, danakj wrote: > On 2017/05/17 21:22:50, Saman Sami wrote: > > On 2017/05/17 21:03:28, danakj wrote: > > > On 2017/05/17 20:52:08, Saman Sami wrote: > > > > On 2017/05/17 20:49:36, danakj wrote: > > > > > How will this happen in practice? Why isn't this just a DCHECK? > > > > > > > > If the client misbehaves, for example. > > > > > > Can't we reject that at the IPC boundary instead of here? > > > > We don't know the details of the surface at IPC boundary, unfortunately. > > I guess I am asking how far out we can push it then. This is called by > CFS::SubmitCompositorFrameSink. It could query these and reject the frame > instead of calling Queue? Repeat up the call stack, how far can it go? I guess we can do the verification in CFSS but I'm not sure how much benefit that provides. We really can't push it further because there is sometimes nothing further up the call stack, such as in GpuCompositorFrameSink.
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 21:39:26, Saman Sami wrote: > On 2017/05/17 21:25:28, danakj wrote: > > On 2017/05/17 21:22:50, Saman Sami wrote: > > > On 2017/05/17 21:03:28, danakj wrote: > > > > On 2017/05/17 20:52:08, Saman Sami wrote: > > > > > On 2017/05/17 20:49:36, danakj wrote: > > > > > > How will this happen in practice? Why isn't this just a DCHECK? > > > > > > > > > > If the client misbehaves, for example. > > > > > > > > Can't we reject that at the IPC boundary instead of here? > > > > > > We don't know the details of the surface at IPC boundary, unfortunately. > > > > I guess I am asking how far out we can push it then. This is called by > > CFS::SubmitCompositorFrameSink. It could query these and reject the frame > > instead of calling Queue? Repeat up the call stack, how far can it go? > > I guess we can do the verification in CFSS but I'm not sure how much benefit > that provides. We really can't push it further because there is sometimes > nothing further up the call stack, such as in GpuCompositorFrameSink. +1 There isn't a whole lot up the stack in some cases and moving this to DelegatedFrameHost doesn't really help us in Mus/Mushrome/Mus+Ash...
LGTM https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:133: DLOG(ERROR) << "Cannot create a surface with invalid SurfaceInfo."; Can you make this a trace event instead so we can see it when debugging there? https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:352: std::unique_ptr<Surface> surface = nit: just return here instead of using a variable https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:68: DLOG(FATAL) << "Rejected CompositorFrame that violates surface invariants."; can this be a trace event also? https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_fra... File cc/test/compositor_frame_helpers.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_fra... cc/test/compositor_frame_helpers.cc:25: frame.metadata.begin_frame_ack.has_damage = true; Why is this needed? Any why not needed in the other functions?
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
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...
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:133: DLOG(ERROR) << "Cannot create a surface with invalid SurfaceInfo."; On 2017/05/18 15:15:53, danakj wrote: > Can you make this a trace event instead so we can see it when debugging there? Done. https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:352: std::unique_ptr<Surface> surface = On 2017/05/18 15:15:54, danakj wrote: > nit: just return here instead of using a variable Done. https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc... cc/surfaces/surface.cc:68: DLOG(FATAL) << "Rejected CompositorFrame that violates surface invariants."; On 2017/05/18 15:15:54, danakj wrote: > can this be a trace event also? Done. https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_fra... File cc/test/compositor_frame_helpers.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_fra... cc/test/compositor_frame_helpers.cc:25: frame.metadata.begin_frame_ack.has_damage = true; On 2017/05/18 15:15:54, danakj wrote: > Why is this needed? Any why not needed in the other functions? The other methods call this method internally. CFSS::SubmitCompositorFrame dchecks that has_damage is true.
https://codereview.chromium.org/2848223003/diff/580001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (left): https://codereview.chromium.org/2848223003/diff/580001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:139: std::move(frame), Shouldn't we be returning resources here if this fails and calling DidReceiveCompositorFrameAck?
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_...)
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...
Patchset #31 (id:600001) has been deleted
Patchset #31 (id:620001) has been deleted
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
Patchset #31 (id:640001) has been deleted
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...
Patchset #32 (id:680001) has been deleted
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
https://codereview.chromium.org/2848223003/diff/580001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (left): https://codereview.chromium.org/2848223003/diff/580001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:139: std::move(frame), On 2017/05/18 20:51:32, Fady Samuel wrote: > Shouldn't we be returning resources here if this fails and calling > DidReceiveCompositorFrameAck? If the client misbehaves then not really.
Patchset #8 (id:140001) has been deleted
Patchset #10 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:100001) has been deleted
Patchset #11 (id:240001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #14 (id:360001) has been deleted
Patchset #11 (id:300001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
Patchset #5 (id:80001) has been deleted
Patchset #8 (id:260001) has been deleted
https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_fra... File cc/test/compositor_frame_helpers.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_fra... cc/test/compositor_frame_helpers.cc:25: frame.metadata.begin_frame_ack.has_damage = true; On 2017/05/18 20:44:37, Saman Sami wrote: > On 2017/05/18 15:15:54, danakj wrote: > > Why is this needed? Any why not needed in the other functions? > > The other methods call this method internally. CFSS::SubmitCompositorFrame > dchecks that has_damage is true. It seems that CFSS is setting has_damage, not dchecking it? // |has_damage| is not transmitted. frame.metadata.begin_frame_ack.has_damage = true; https://codereview.chromium.org/2848223003/diff/700001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2848223003/diff/700001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:110: DCHECK(support_->SubmitCompositorFrame(local_surface_id_, std::move(frame))); DCHECKs are compiled out in release, so SubmitCompositorFrame will not be run at all. You want to DCHECK a variable that it returns. Please make sure you don't do this elsewhere too :) https://codereview.chromium.org/2848223003/diff/700001/cc/test/test_composito... File cc/test/test_compositor_frame_sink.cc (right): https://codereview.chromium.org/2848223003/diff/700001/cc/test/test_composito... cc/test/test_compositor_frame_sink.cc:143: DCHECK(support_->SubmitCompositorFrame(local_surface_id_, std::move(frame))); here's another https://codereview.chromium.org/2848223003/diff/700001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2848223003/diff/700001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:371: DCHECK(support_->SubmitCompositorFrame(local_surface_id, std::move(frame))); and here https://codereview.chromium.org/2848223003/diff/700001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2848223003/diff/700001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:459: DCHECK(support_->SubmitCompositorFrame(local_surface_id, std::move(frame))); and here https://codereview.chromium.org/2848223003/diff/700001/content/renderer/andro... File content/renderer/android/synchronous_compositor_frame_sink.cc (right): https://codereview.chromium.org/2848223003/diff/700001/content/renderer/andro... content/renderer/android/synchronous_compositor_frame_sink.cc:310: DCHECK(child_support_->SubmitCompositorFrame(child_local_surface_id_, and here https://codereview.chromium.org/2848223003/diff/700001/ui/android/delegated_f... File ui/android/delegated_frame_host_android.cc (right): https://codereview.chromium.org/2848223003/diff/700001/ui/android/delegated_f... ui/android/delegated_frame_host_android.cc:88: DCHECK(support_->SubmitCompositorFrame(local_surface_id, std::move(frame))); and here :) https://codereview.chromium.org/2848223003/diff/700001/ui/aura/local/composit... File ui/aura/local/compositor_frame_sink_local.cc (right): https://codereview.chromium.org/2848223003/diff/700001/ui/aura/local/composit... ui/aura/local/compositor_frame_sink_local.cc:74: DCHECK(support_->SubmitCompositorFrame(local_surface_id_, std::move(frame))); and here
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/2848223003/diff/520001/cc/test/compositor_fra... File cc/test/compositor_frame_helpers.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_fra... cc/test/compositor_frame_helpers.cc:25: frame.metadata.begin_frame_ack.has_damage = true; On 2017/05/19 19:00:59, danakj wrote: > On 2017/05/18 20:44:37, Saman Sami wrote: > > On 2017/05/18 15:15:54, danakj wrote: > > > Why is this needed? Any why not needed in the other functions? > > > > The other methods call this method internally. CFSS::SubmitCompositorFrame > > dchecks that has_damage is true. > > It seems that CFSS is setting has_damage, not dchecking it? > > // |has_damage| is not transmitted. > frame.metadata.begin_frame_ack.has_damage = true; Oops. It was DirectCFS that dchecked. I used this method in DirectCFS tests and I added this line so it doesn't fail. https://codereview.chromium.org/2848223003/diff/700001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2848223003/diff/700001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:110: DCHECK(support_->SubmitCompositorFrame(local_surface_id_, std::move(frame))); On 2017/05/19 19:00:59, danakj wrote: > DCHECKs are compiled out in release, so SubmitCompositorFrame will not be run at > all. You want to DCHECK a variable that it returns. Please make sure you don't > do this elsewhere too :) Oops... https://codereview.chromium.org/2848223003/diff/700001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2848223003/diff/700001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:371: DCHECK(support_->SubmitCompositorFrame(local_surface_id, std::move(frame))); On 2017/05/19 19:00:59, danakj wrote: > and here Done. https://codereview.chromium.org/2848223003/diff/700001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2848223003/diff/700001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:459: DCHECK(support_->SubmitCompositorFrame(local_surface_id, std::move(frame))); On 2017/05/19 19:00:59, danakj wrote: > and here Done. https://codereview.chromium.org/2848223003/diff/700001/content/renderer/andro... File content/renderer/android/synchronous_compositor_frame_sink.cc (right): https://codereview.chromium.org/2848223003/diff/700001/content/renderer/andro... content/renderer/android/synchronous_compositor_frame_sink.cc:310: DCHECK(child_support_->SubmitCompositorFrame(child_local_surface_id_, On 2017/05/19 19:00:59, danakj wrote: > and here Done. https://codereview.chromium.org/2848223003/diff/700001/ui/android/delegated_f... File ui/android/delegated_frame_host_android.cc (right): https://codereview.chromium.org/2848223003/diff/700001/ui/android/delegated_f... ui/android/delegated_frame_host_android.cc:88: DCHECK(support_->SubmitCompositorFrame(local_surface_id, std::move(frame))); On 2017/05/19 19:00:59, danakj wrote: > and here :) Done. https://codereview.chromium.org/2848223003/diff/700001/ui/aura/local/composit... File ui/aura/local/compositor_frame_sink_local.cc (right): https://codereview.chromium.org/2848223003/diff/700001/ui/aura/local/composit... ui/aura/local/compositor_frame_sink_local.cc:74: DCHECK(support_->SubmitCompositorFrame(local_surface_id_, std::move(frame))); On 2017/05/19 19:00:59, danakj wrote: > and here Done.
LGTM https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_fra... File cc/test/compositor_frame_helpers.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_fra... cc/test/compositor_frame_helpers.cc:25: frame.metadata.begin_frame_ack.has_damage = true; On 2017/05/19 19:26:46, Saman Sami wrote: > On 2017/05/19 19:00:59, danakj wrote: > > On 2017/05/18 20:44:37, Saman Sami wrote: > > > On 2017/05/18 15:15:54, danakj wrote: > > > > Why is this needed? Any why not needed in the other functions? > > > > > > The other methods call this method internally. CFSS::SubmitCompositorFrame > > > dchecks that has_damage is true. > > > > It seems that CFSS is setting has_damage, not dchecking it? > > > > // |has_damage| is not transmitted. > > frame.metadata.begin_frame_ack.has_damage = true; > > Oops. It was DirectCFS that dchecked. I used this method in DirectCFS tests and > I added this line so it doesn't fail. I see, thanks. https://codereview.chromium.org/2848223003/diff/720001/components/viz/frame_s... File components/viz/frame_sinks/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2848223003/diff/720001/components/viz/frame_s... components/viz/frame_sinks/gpu_compositor_frame_sink.cc:50: bool result = it's okay to keep this without the bool var, I think I'd prefer that https://codereview.chromium.org/2848223003/diff/720001/components/viz/frame_s... File components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc (right): https://codereview.chromium.org/2848223003/diff/720001/components/viz/frame_s... components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc:89: bool result = same here, the name result isnt adding anything
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_...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_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...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #8 (id:340001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #5 (id:220001) has been deleted
Patchset #5 (id:320001) has been deleted
Patchset #5 (id:380001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #5 (id:440001) has been deleted
Patchset #4 (id:420001) has been deleted
Patchset #3 (id:400001) has been deleted
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Description was changed from ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
samans@chromium.org changed reviewers: + boliu@chromium.org, piman@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
boliu, please review android. piman, please review content.
https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:148: return true; Why are we returning true here?
lgtm
https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:148: return true; On 2017/05/23 16:40:47, Fady Samuel wrote: > Why are we returning true here? We only return false when the frame doesn't belong to the surface. When we are asked to create a surface of size 0, we simply don't do it. I did this to make exo happy.
On 2017/05/23 16:48:11, Saman Sami wrote: > https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor... > File cc/surfaces/compositor_frame_sink_support.cc (right): > > https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor... > cc/surfaces/compositor_frame_sink_support.cc:148: return true; > On 2017/05/23 16:40:47, Fady Samuel wrote: > > Why are we returning true here? > > We only return false when the frame doesn't belong to the surface. When we are > asked to create a surface of size 0, we simply don't do it. I did this to make > exo happy. OK thanks for the clarification.
https://codereview.chromium.org/2848223003/diff/780001/content/renderer/andro... File content/renderer/android/synchronous_compositor_frame_sink.h (right): https://codereview.chromium.org/2848223003/diff/780001/content/renderer/andro... content/renderer/android/synchronous_compositor_frame_sink.h:161: float device_scale_factor_; new POD field needs new initializer
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
samans@chromium.org changed reviewers: + sadrul@chromium.org
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 ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
samans@chromium.org changed reviewers: - sadrul@chromium.org
https://codereview.chromium.org/2848223003/diff/780001/content/renderer/andro... File content/renderer/android/synchronous_compositor_frame_sink.h (right): https://codereview.chromium.org/2848223003/diff/780001/content/renderer/andro... content/renderer/android/synchronous_compositor_frame_sink.h:161: float device_scale_factor_; On 2017/05/23 18:03:17, piman wrote: > new POD field needs new initializer Done.
Description was changed from ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
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
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, danakj@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2848223003/#ps800001 (title: "Address comments")
The CQ bit was unchecked by samans@chromium.org
Description was changed from ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
samans@chromium.org changed reviewers: + sadrul@chromium.org
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, danakj@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2848223003/#ps800001 (title: "Address comments")
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": 800001, "attempt_start_ts": 1495573298747660, "parent_rev": "82bf805a06b1dede93dd10d8d4746ebfc03be0ac", "commit_rev": "5ba3188fa05666ae8d6031706c0963532f9d1457"}
Message was sent while issue was closed.
Description was changed from ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2848223003 Cr-Commit-Position: refs/heads/master@{#474050} Committed: https://chromium.googlesource.com/chromium/src/+/5ba3188fa05666ae8d6031706c09... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:800001) as https://chromium.googlesource.com/chromium/src/+/5ba3188fa05666ae8d6031706c09...
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:800001) has been created in https://codereview.chromium.org/2905523003/ by nednguyen@google.com. The reason for reverting is: Suspect this breaks SurfaceAggregatorPerfTest BUG=725840. |