Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2612083002:
DirectCompositorFrameSink Uses CompositorFrameSinkSupport (Closed)
DescriptionDirectCompositorFrameSink Uses CompositorFrameSinkSupport
CompositorFrameSinkSupport is the common service-side implementation of
CompositorFrameSink. In an effort to evaluate mojo performance
independently of refactoring existing code, CompositorFrameSinkSupport
provides the same public interface as MojoCompositorFrameSink but does
not depend on mojo.
This CL takes us in the direction of splitting DirectCompositorFrameSink
into two pieces: a client side component and a service side component.
BUG=673543
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2612083002
Cr-Original-Commit-Position: refs/heads/master@{#448731}
Committed: https://chromium.googlesource.com/chromium/src/+/c405416053b99aa4d6f98f3126608c660773f6b4
Review-Url: https://codereview.chromium.org/2612083002
Cr-Commit-Position: refs/heads/master@{#451462}
Committed: https://chromium.googlesource.com/chromium/src/+/9d4e3df6e621c6bd80dfaee7008b3e33d0c4fe79
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed comments #
Total comments: 10
Patch Set 3 : DirectCompositorFrameSink implements ExternalBeginFrameSouceClient; Addressed comments #
Total comments: 4
Patch Set 4 : Check if we have a display before setting visibility #Patch Set 5 : Calling SetLocalFrameId() before SubmitCompositorFrame() in CompositorFrameSinkSupport::SubmitCompo… #
Total comments: 2
Patch Set 6 : DirectCompositorFrameSink resets client's BeginFrameSource before detaching #Patch Set 7 : minor fixes #Patch Set 8 : rebase #Patch Set 9 : SetLostContextCallback before creating CompositorFrameSinkSupport #
Total comments: 5
Patch Set 10 : rebase #Patch Set 11 : rebase exo #Patch Set 12 : Set display visible in CompositorFrameSinkSupport() #Patch Set 13 : ~CompositorFrameSink() doesn't invalidate FrameSinkId #Patch Set 14 : Use MakeUnique #
Total comments: 3
Patch Set 15 : CompositorFrameSinkSupport takes a flag indicating if it's supporting a DirectCompositorFrameSink #
Total comments: 2
Patch Set 16 : Added a pending_child_frame_sink_ids_ list in CompositorImplAndroid #Patch Set 17 : Add all pending FrameSinkIds after CompositorFrameSink is created #Patch Set 18 : DetachFromClient() calls DidLoseCompositorFrameSink() #
Total comments: 6
Patch Set 19 : set has_compositor_frame_sink_ to false in SetVisible(false) #
Total comments: 18
Patch Set 20 : Addressed comments #
Total comments: 11
Patch Set 21 : addressed comments #
Total comments: 26
Patch Set 22 : addressed comments #Patch Set 23 : CompositorImpl::AddChildFrameSink() and CompositorImpl::RemoveChildFrameSink() replace DelegatedFra… #
Total comments: 6
Patch Set 24 : Addressed comments #
Total comments: 4
Patch Set 25 : Removed redundant Add/RemoveChildFrameSink() #Patch Set 26 : Addressed nits #
Total comments: 25
Patch Set 27 : Addressed comments #Patch Set 28 : move display_->SetVisible(true) to GpuDisplayCompositorFrameSink() #
Total comments: 2
Patch Set 29 : Restored DelegatedFrameHostAndroid::RegisterFrameSinkHierarchy() and UnregisterFrameSinkHierarchy()… #
Total comments: 9
Patch Set 30 : Added Add/RemoveChildFrameSink() to WindowAndroidCompositor #
Total comments: 4
Patch Set 31 : Added override keyword #Patch Set 32 : Initialize parent_window_android_compositor_ to nullptr #
Total comments: 6
Patch Set 33 : Addressed comments #Patch Set 34 : Added for_direct_compositor_frame_sink_ flag to CompositorFrameSinkSupport #Patch Set 35 : Set NeedsBeginFrame to false before resetting support_ #
Total comments: 10
Patch Set 36 : Address comments and revert changes in Android #
Total comments: 16
Patch Set 37 : addressed comments #
Total comments: 7
Patch Set 38 : Addressed comments #Patch Set 39 : rebase #Patch Set 40 : Added NON_EXPORTED_BASE #
Messages
Total messages: 229 (138 generated)
Description was changed from ========== WIP: DirectCompositorFrameSink uses CompositorFrameSinkSupport BUG=673543 ========== to ========== WIP: DirectCompositorFrameSink uses CompositorFrameSinkSupport BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:121: void CompositorFrameSinkSupport::SetFactoryNeedsSyncPoints( nit: Call it SetNeedsSyncPoints? https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:131: void CompositorFrameSinkSupport::BindToClient() { Instead of calling BindToClient, create the CompositorFrameSinkSupport in DirectCompositorFrameSink::BindToClient() https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:137: void CompositorFrameSinkSupport::DetachFromClient() { Instead of calling DetachFromClient here, destroy the CompositorFrameSinkSupport in DirectCompositorFrameSink::DetachFromClient. https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:144: void CompositorFrameSinkSupport::SetDisplayVisible(bool visible) { No need to introduce this. support_->display()->SetVisible(...) https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:150: surface_manager_->RegisterFrameSinkId(frame_sink_id_); Remove this. This is unnecessary. https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:153: void CompositorFrameSinkSupport::InvalidateFrameSinkId() { Remove this. This is unnecessary. https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/direct_composit... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/direct_composit... cc/surfaces/direct_compositor_frame_sink.cc:69: support_.BindToClient(); Create support_ here instead. Use a unique_ptr. https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/direct_composit... cc/surfaces/direct_compositor_frame_sink.cc:82: support_.DetachFromClient(); support_.reset() support_ is a unique_ptr.
https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:121: void CompositorFrameSinkSupport::SetFactoryNeedsSyncPoints( On 2017/01/04 20:59:25, Fady Samuel wrote: > nit: Call it SetNeedsSyncPoints? Done. https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:131: void CompositorFrameSinkSupport::BindToClient() { On 2017/01/04 20:59:25, Fady Samuel wrote: > Instead of calling BindToClient, create the CompositorFrameSinkSupport in > DirectCompositorFrameSink::BindToClient() Done. https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:137: void CompositorFrameSinkSupport::DetachFromClient() { On 2017/01/04 20:59:25, Fady Samuel wrote: > Instead of calling DetachFromClient here, destroy the CompositorFrameSinkSupport > in DirectCompositorFrameSink::DetachFromClient. Done. https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:144: void CompositorFrameSinkSupport::SetDisplayVisible(bool visible) { On 2017/01/04 20:59:25, Fady Samuel wrote: > No need to introduce this. support_->display()->SetVisible(...) Done. https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:150: surface_manager_->RegisterFrameSinkId(frame_sink_id_); On 2017/01/04 20:59:25, Fady Samuel wrote: > Remove this. This is unnecessary. DirectCompositorFrameSink doesn't register and invalidate FrameSinkId. Doing it triggers the DCHECK at https://cs.chromium.org/chromium/src/cc/surfaces/surface_manager.cc?sq=packag.... https://codereview.chromium.org/2612083002/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:153: void CompositorFrameSinkSupport::InvalidateFrameSinkId() { On 2017/01/04 20:59:25, Fady Samuel wrote: > Remove this. This is unnecessary. DirectCompositorFrameSink doesn't register and invalidate FrameSinkId. Doing it triggers the DCHECK at https://cs.chromium.org/chromium/src/cc/surfaces/surface_manager.cc?sq=packag....
https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (left): https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:49: surface_manager_->InvalidateFrameSinkId(frame_sink_id_); Why was this deleted? Please restore this code? https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:139: void CompositorFrameSinkSupport::RegisterFrameSinkId() { Delete this? https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:143: void CompositorFrameSinkSupport::InvalidateFrameSinkId() { Delete this? https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/direct_comp... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:119: // TODO(staraz): Implement this. Implement this? This won't work properly until you implement this.
Ahh you need to change display_ to a raw pointer in CompositorFrameSinkSupport. https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:88: std::unique_ptr<Display> display_; This should be a raw pointer!
The CQ bit was checked by staraz@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/2612083002/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (left): https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:49: surface_manager_->InvalidateFrameSinkId(frame_sink_id_); On 2017/01/04 21:47:40, Fady Samuel wrote: > Why was this deleted? Please restore this code? Done. https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:139: void CompositorFrameSinkSupport::RegisterFrameSinkId() { On 2017/01/04 21:47:40, Fady Samuel wrote: > Delete this? Done. https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:143: void CompositorFrameSinkSupport::InvalidateFrameSinkId() { On 2017/01/04 21:47:40, Fady Samuel wrote: > Delete this? Done. https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:88: std::unique_ptr<Display> display_; On 2017/01/04 21:53:05, Fady Samuel wrote: > This should be a raw pointer! Done. https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/direct_comp... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/20001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:119: // TODO(staraz): Implement this. On 2017/01/04 21:47:41, Fady Samuel wrote: > Implement this? This won't work properly until you implement this. Done.
https://codereview.chromium.org/2612083002/diff/40001/cc/surfaces/direct_comp... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/40001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:66: support_->SetNeedsSyncPoints(capabilities_.delegated_sync_points_required); To be honest, I'm not entirely sure what this is used for. jbauman@ may provide some insight. I'd prefer not exposing this API if it's not necessary. https://codereview.chromium.org/2612083002/diff/40001/services/ui/surfaces/gp... File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/40001/services/ui/surfaces/gp... services/ui/surfaces/gpu_compositor_frame_sink.cc:34: support_.display()->SetVisible(true); We might not have a display(). Please add a guard: if (support_.display()) support_.display()->SetVisible(true);
staraz@chromium.org changed reviewers: + jbauman@chromium.org
jbauman@chromium.org: Please review changes in cc/surfaces. In addition, could you explain why we need the set_needs_sync_pionts(false) in DirectCompositorFrameSink()? Please see fsamuel@'s comment. https://codereview.chromium.org/2612083002/diff/40001/services/ui/surfaces/gp... File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/40001/services/ui/surfaces/gp... services/ui/surfaces/gpu_compositor_frame_sink.cc:34: support_.display()->SetVisible(true); On 2017/01/05 00:36:38, Fady Samuel wrote: > We might not have a display(). Please add a guard: > > if (support_.display()) > support_.display()->SetVisible(true); Done.
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2612083002/diff/40001/cc/surfaces/direct_comp... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/40001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:66: support_->SetNeedsSyncPoints(capabilities_.delegated_sync_points_required); On 2017/01/05 00:36:38, Fady Samuel wrote: > To be honest, I'm not entirely sure what this is used for. jbauman@ may provide > some insight. I'd prefer not exposing this API if it's not necessary. You need sync points if the display and ui compositors are using different GL contexts. You don't if they are using the same context. Right now they use the same context, so the CFS reports that they are not required. In future it will be OOP and will be required. The capability is there for ui compositor to know if it should insert sync points. The surface aggregator also wants to know if it should use sync points when returning resources back to the ui compositor, which it currently reads off the surfacefactory.
https://codereview.chromium.org/2612083002/diff/80001/cc/surfaces/direct_comp... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/80001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:83: begin_frame_source_.reset(); client_->SetBeginFrameSource(nullptr); first?
https://codereview.chromium.org/2612083002/diff/80001/cc/surfaces/direct_comp... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/80001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:83: begin_frame_source_.reset(); On 2017/01/05 21:46:56, Fady Samuel wrote: > client_->SetBeginFrameSource(nullptr); first? Yep. That fixed it. Thanks!
The CQ bit was checked by staraz@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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by staraz@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-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...)
The CQ bit was checked by staraz@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 staraz@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...
staraz@chromium.org changed reviewers: + reveman@chromium.org, xlai@chromium.org
reveman@chromium.org: Please review changes in components/exo/compositor_frame_sink.cc xlai@chromium.org: Please review changes in content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc
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...)
components/exo lgtm
https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:70: support_.reset(new CompositorFrameSinkSupport(this, surface_manager_, base::MakeUnique. https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:91: valid_frame_sink_ids_.insert(frame_sink_id); Why is removing this DCHECK necessary? We shouldn't be registering the same frame sink twice.
https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:91: valid_frame_sink_ids_.insert(frame_sink_id); On 2017/01/07 01:17:26, jbauman wrote: > Why is removing this DCHECK necessary? We shouldn't be registering the same > frame sink twice. Originally, DirectCompositorFrameSink::BindToClient() doesn't call RegisterFrameSinkId(). CompositorFrameSinkSupport calls RegisterFrameSinkId() in its constructor, which triggers the DCHECK for DirectCompositorFrameSink. fsamuel told me to remove the DCHECK instead of exposing RegisterFrameSinkId() to GpuCompositorFrameSink or make a custom CompositorFrameSink() for DirectCompositorFrameSink.
https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:91: valid_frame_sink_ids_.insert(frame_sink_id); On 2017/01/07 14:35:49, StarAZ1 wrote: > On 2017/01/07 01:17:26, jbauman wrote: > > Why is removing this DCHECK necessary? We shouldn't be registering the same > > frame sink twice. > > Originally, DirectCompositorFrameSink::BindToClient() doesn't call > RegisterFrameSinkId(). CompositorFrameSinkSupport calls RegisterFrameSinkId() > in its constructor, which triggers the DCHECK for DirectCompositorFrameSink. > > fsamuel told me to remove the DCHECK instead of exposing RegisterFrameSinkId() > to GpuCompositorFrameSink or make a custom CompositorFrameSink() for > DirectCompositorFrameSink. ui::Compositor Registers the FrameSinkId first: https://cs.chromium.org/chromium/src/ui/compositor/compositor.cc?sq=package:c... However, we eventually want to get rid of that in favor of the SurfaceReference approach to reference tracking. We're not there yet. For the time being, we are registering the FrameSinkId in CompositorFrameSinkSupport. However, while we're using SurfaceSequences, we cannot get rid of RegisterFrameSinkId in ui::Compositor, so I thought getting rid of the DCHECK would be easier for now. Note that once SurfaceReferences are used everywhere, RegisterFrameSinkId and InvalidateFrameSinkId will likely go away.
The CQ bit was checked by staraz@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by staraz@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 staraz@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 ========== WIP: DirectCompositorFrameSink uses CompositorFrameSinkSupport BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== WIP: DirectCompositorFrameSink uses CompositorFrameSinkSupport BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/150001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:70: support_.reset(new CompositorFrameSinkSupport(this, surface_manager_, On 2017/01/07 01:17:26, jbauman wrote: > base::MakeUnique. Done.
https://codereview.chromium.org/2612083002/diff/250001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/250001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:82: support_.reset(); So why did you get rid of InvalidateFrameSinkId? https://codereview.chromium.org/2612083002/diff/250001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/250001/components/exo/composi... components/exo/compositor_frame_sink.cc:25: support_.InvalidateFrameSinkId(); Why is this necessary? support should do this automatically when destroyed? https://codereview.chromium.org/2612083002/diff/250001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/250001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:56: support_.InvalidateFrameSinkId(); Why?
On 2017/01/23 21:56:13, Fady Samuel wrote: > https://codereview.chromium.org/2612083002/diff/250001/cc/surfaces/direct_com... > File cc/surfaces/direct_compositor_frame_sink.cc (right): > > https://codereview.chromium.org/2612083002/diff/250001/cc/surfaces/direct_com... > cc/surfaces/direct_compositor_frame_sink.cc:82: support_.reset(); > So why did you get rid of InvalidateFrameSinkId? > > https://codereview.chromium.org/2612083002/diff/250001/components/exo/composi... > File components/exo/compositor_frame_sink.cc (right): > > https://codereview.chromium.org/2612083002/diff/250001/components/exo/composi... > components/exo/compositor_frame_sink.cc:25: support_.InvalidateFrameSinkId(); > Why is this necessary? support should do this automatically when destroyed? > > https://codereview.chromium.org/2612083002/diff/250001/services/ui/surfaces/g... > File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): > > https://codereview.chromium.org/2612083002/diff/250001/services/ui/surfaces/g... > services/ui/surfaces/gpu_compositor_frame_sink.cc:56: > support_.InvalidateFrameSinkId(); > Why? In patchset 11, all content_browsertests failed on linux_android_rel_ng triggering the DCHECK in SurfaceManager::RegisterFrameSinkHierarchy() (https://cs.chromium.org/chromium/src/cc/surfaces/surface_manager.cc?q=registe...). Currently, DirectCompositorFrameSink::DetachFrameClient() calls only UnregisterSurfaceFactoryClient() but not InvalidateFrameSinkId() while ~CompositorFrameSinkSupport() calls them both. I suspect that this causes the DCHECK to fail. I want to add a flag in CompositorFrameSinkSupport indicating if the CompositorFrameSinkSupport is for DirectCompositorFrameSink. ~CompositorFrameSinkSupport doesn't call InvalidateFrameSinkId() if that's the case.
On 2017/01/23 22:26:38, StarAZ1 wrote: > On 2017/01/23 21:56:13, Fady Samuel wrote: > > > https://codereview.chromium.org/2612083002/diff/250001/cc/surfaces/direct_com... > > File cc/surfaces/direct_compositor_frame_sink.cc (right): > > > > > https://codereview.chromium.org/2612083002/diff/250001/cc/surfaces/direct_com... > > cc/surfaces/direct_compositor_frame_sink.cc:82: support_.reset(); > > So why did you get rid of InvalidateFrameSinkId? > > > > > https://codereview.chromium.org/2612083002/diff/250001/components/exo/composi... > > File components/exo/compositor_frame_sink.cc (right): > > > > > https://codereview.chromium.org/2612083002/diff/250001/components/exo/composi... > > components/exo/compositor_frame_sink.cc:25: support_.InvalidateFrameSinkId(); > > Why is this necessary? support should do this automatically when destroyed? > > > > > https://codereview.chromium.org/2612083002/diff/250001/services/ui/surfaces/g... > > File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): > > > > > https://codereview.chromium.org/2612083002/diff/250001/services/ui/surfaces/g... > > services/ui/surfaces/gpu_compositor_frame_sink.cc:56: > > support_.InvalidateFrameSinkId(); > > Why? > > In patchset 11, all content_browsertests failed on linux_android_rel_ng > triggering the DCHECK in > SurfaceManager::RegisterFrameSinkHierarchy() > (https://cs.chromium.org/chromium/src/cc/surfaces/surface_manager.cc?q=registe...). > > Currently, DirectCompositorFrameSink::DetachFrameClient() calls only > UnregisterSurfaceFactoryClient() but not > InvalidateFrameSinkId() while ~CompositorFrameSinkSupport() calls them both. I > suspect that this causes the DCHECK to > fail. > > I want to add a flag in CompositorFrameSinkSupport indicating if the > CompositorFrameSinkSupport is for > DirectCompositorFrameSink. ~CompositorFrameSinkSupport doesn't call > InvalidateFrameSinkId() if that's the case. I'd rather we figure out why Android is triggering this and fix the root cause. Do you have a stack trace?
On 2017/01/23 22:30:32, Fady Samuel wrote: > On 2017/01/23 22:26:38, StarAZ1 wrote: > > On 2017/01/23 21:56:13, Fady Samuel wrote: > > > > > > https://codereview.chromium.org/2612083002/diff/250001/cc/surfaces/direct_com... > > > File cc/surfaces/direct_compositor_frame_sink.cc (right): > > > > > > > > > https://codereview.chromium.org/2612083002/diff/250001/cc/surfaces/direct_com... > > > cc/surfaces/direct_compositor_frame_sink.cc:82: support_.reset(); > > > So why did you get rid of InvalidateFrameSinkId? > > > > > > > > > https://codereview.chromium.org/2612083002/diff/250001/components/exo/composi... > > > File components/exo/compositor_frame_sink.cc (right): > > > > > > > > > https://codereview.chromium.org/2612083002/diff/250001/components/exo/composi... > > > components/exo/compositor_frame_sink.cc:25: > support_.InvalidateFrameSinkId(); > > > Why is this necessary? support should do this automatically when destroyed? > > > > > > > > > https://codereview.chromium.org/2612083002/diff/250001/services/ui/surfaces/g... > > > File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): > > > > > > > > > https://codereview.chromium.org/2612083002/diff/250001/services/ui/surfaces/g... > > > services/ui/surfaces/gpu_compositor_frame_sink.cc:56: > > > support_.InvalidateFrameSinkId(); > > > Why? > > > > In patchset 11, all content_browsertests failed on linux_android_rel_ng > > triggering the DCHECK in > > SurfaceManager::RegisterFrameSinkHierarchy() > > > (https://cs.chromium.org/chromium/src/cc/surfaces/surface_manager.cc?q=registe...). > > > > Currently, DirectCompositorFrameSink::DetachFrameClient() calls only > > UnregisterSurfaceFactoryClient() but not > > InvalidateFrameSinkId() while ~CompositorFrameSinkSupport() calls them both. I > > suspect that this causes the DCHECK to > > fail. > > > > I want to add a flag in CompositorFrameSinkSupport indicating if the > > CompositorFrameSinkSupport is for > > DirectCompositorFrameSink. ~CompositorFrameSinkSupport doesn't call > > InvalidateFrameSinkId() if that's the case. > > I'd rather we figure out why Android is triggering this and fix the root cause. > Do you have a stack trace? I have the stack trace from the trybot (https://paste.googleplex.com/5268243275055104) but it isn't very helpful. I was trying to run the tests on an emulator or a device this morning but the gtest wrapper kept giving me NoDeviceError even though the device is there when I typed %adb devices.
The CQ bit was checked by staraz@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/2612083002/diff/270001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:33: bool direct_compositor_frame_sink); not lgtm. Please note that contradicts the direction we'd like to go in.
https://codereview.chromium.org/2612083002/diff/270001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/270001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:33: bool direct_compositor_frame_sink); On 2017/01/25 13:56:04, Fady Samuel wrote: > not lgtm. Please note that contradicts the direction we'd like to go in. Yes, I added this before we talked on Monday. I'm working on the queue in CompositorImplAndroid. I'll upload that when it's ready.
The CQ bit was checked by staraz@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_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by staraz@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 staraz@chromium.org to run a CQ dry run
staraz@chromium.org changed reviewers: + boliu@chromium.org
boliu@chromium.org: Please review changes in android code.
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/2612083002/diff/330001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:674: for (auto& frame_sink_id : pending_child_frame_sink_ids_) { This should probably go in DidInitializeCompositorFrameSink. https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:678: has_compositor_frame_sink_ = true; I don't think this is necessary. https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:744: if (!has_compositor_frame_sink_) { if (compositor_frame_sink_request_pending_)
https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:674: for (auto& frame_sink_id : pending_child_frame_sink_ids_) { This should probably go in DidInitializeCompositorFrameSink. https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:678: has_compositor_frame_sink_ = true; I don't think this is necessary. https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:744: if (!has_compositor_frame_sink_) { if (compositor_frame_sink_request_pending_)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:674: for (auto& frame_sink_id : pending_child_frame_sink_ids_) { On 2017/01/25 22:36:51, Fady Samuel wrote: > This should probably go in DidInitializeCompositorFrameSink. Done. https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:678: has_compositor_frame_sink_ = true; On 2017/01/25 22:36:51, Fady Samuel wrote: > I don't think this is necessary. We need the flag to change when DirectCompositorFrameSink detaches from client (when we "lost" DirectCompositorFrameSink). compositor_frame_sink_request_pending_ remains false when that happens. https://codereview.chromium.org/2612083002/diff/330001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:744: if (!has_compositor_frame_sink_) { On 2017/01/25 22:36:51, Fady Samuel wrote: > if (compositor_frame_sink_request_pending_) compositor_frame_sink_request_pending_ gets flipped when a new request is created or the request is fulfilled. It doesn't change when the DirectCompositorFrameSink detaches from client (this happens when SetVisible(false) is called). I missed that one in this patch.
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_...)
I assume you are still trying to land this? Skimmed cc, but not an owner there :p https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:83: client_->DidLoseCompositorFrameSink(); This doesn't make sense in my head.. was this called from Detach before? https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:128: void DirectCompositorFrameSink::ReclaimResources( this is really confusing with ForceReclaimResources, I guess this comment should be on ForceReclaimResources since that's the new method here.. https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:135: // TODO(staraz): Implement this. TODO needed? Looking at the old code, no-op seems like a perfectly valid implementation https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:124: valid_frame_sink_ids_.insert(frame_sink_id); why drop this DCHECK? is client inserting duplicates now, then that should be fixed? https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:469: has_compositor_frame_sink_ = false; why set this to false here? https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:537: AddChildFrameSink(frame_sink_id); this makes no sense to me.. has_compositor_frame_sink_ is probably still false at this point? then this loop does absolutely nothing, in fact, I think it's an infinite loop right here? https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.h (left): https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:169: nit: don't remove this blank line https://codereview.chromium.org/2612083002/diff/350001/ui/android/window_andr... File ui/android/window_android_compositor.h (right): https://codereview.chromium.org/2612083002/diff/350001/ui/android/window_andr... ui/android/window_android_compositor.h:33: virtual void AddChildFrameSink(cc::FrameSinkId frame_sink_id) = 0; you don't need this here, this is an interface for WindowAndroid to call up to the compositor. WindowAndroid isn't using this method RWHVA can cast the pointer to CompositorImplAndroid directly. Yes other methods are already wrong, but let's not make it worse
https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:83: client_->DidLoseCompositorFrameSink(); On 2017/01/26 22:39:41, boliu wrote: > This doesn't make sense in my head.. was this called from Detach before? This was from one of my experiments and I forgot to remove it. It is removed in my next patchset. https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:128: void DirectCompositorFrameSink::ReclaimResources( On 2017/01/26 22:39:41, boliu wrote: > this is really confusing with ForceReclaimResources, I guess this comment should > be on ForceReclaimResources since that's the new method here.. ForceReclaimResources() and ReclaimResources() are from two interfaces. The two interfaces will merge in the near future. https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:135: // TODO(staraz): Implement this. On 2017/01/26 22:39:40, boliu wrote: > TODO needed? Looking at the old code, no-op seems like a perfectly valid > implementation This is one step towards fsamuel@'s vision of unified CompositorFrameSinks. There's a bit of redundancy between cc::CompositorFrameSink and cc::mojom::MojoCompositorFrameSink due to the subtle differences in their usages. This code might change again soon. https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:124: valid_frame_sink_ids_.insert(frame_sink_id); On 2017/01/26 22:39:41, boliu wrote: > why drop this DCHECK? is client inserting duplicates now, then that should be > fixed? From fsamuel@'s comment: ui::Compositor Registers the FrameSinkId first: https://cs.chromium.org/chromium/src/ui/compositor/compositor.cc?sq=package:c... However, we eventually want to get rid of that in favor of the SurfaceReference approach to reference tracking. We're not there yet. For the time being, we are registering the FrameSinkId in CompositorFrameSinkSupport. However, while we're using SurfaceSequences, we cannot get rid of RegisterFrameSinkId in ui::Compositor, so I thought getting rid of the DCHECK would be easier for now. Note that once SurfaceReferences are used everywhere, RegisterFrameSinkId and InvalidateFrameSinkId will likely go away. https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:469: has_compositor_frame_sink_ = false; On 2017/01/26 22:39:41, boliu wrote: > why set this to false here? host_->ReleaseCompositorFrameSink() calls DirectCompositorFrameSink::DetachFromClient(). DetachFromClient() destroys CompositorFrameSinkSupport(). The frame_sink_id is invalidated in ~CompositorFrameSinkSupport(). https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:537: AddChildFrameSink(frame_sink_id); On 2017/01/26 22:39:41, boliu wrote: > this makes no sense to me.. has_compositor_frame_sink_ is probably still false > at this point? then this loop does absolutely nothing, in fact, I think it's an > infinite loop right here? My bad. has_compositor_frame_sink_ should be set to true before the loop. https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.h (left): https://codereview.chromium.org/2612083002/diff/350001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:169: On 2017/01/26 22:39:41, boliu wrote: > nit: don't remove this blank line Done. https://codereview.chromium.org/2612083002/diff/350001/ui/android/window_andr... File ui/android/window_android_compositor.h (right): https://codereview.chromium.org/2612083002/diff/350001/ui/android/window_andr... ui/android/window_android_compositor.h:33: virtual void AddChildFrameSink(cc::FrameSinkId frame_sink_id) = 0; On 2017/01/26 22:39:41, boliu wrote: > you don't need this here, this is an interface for WindowAndroid to call up to > the compositor. WindowAndroid isn't using this method > > RWHVA can cast the pointer to CompositorImplAndroid directly. Yes other methods > are already wrong, but let's not make it worse Done.
Feel free to ignore my comments on cc https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:128: void DirectCompositorFrameSink::ReclaimResources( On 2017/01/27 20:27:02, StarAZ1 wrote: > On 2017/01/26 22:39:41, boliu wrote: > > this is really confusing with ForceReclaimResources, I guess this comment > should > > be on ForceReclaimResources since that's the new method here.. > > ForceReclaimResources() and ReclaimResources() are from two interfaces. > The two interfaces will merge in the near future. I understand it's on two interfaces. But this is still hard to read.. I had to look up what those interfaces are and how objects are hooked up and whatnot. Could rename one of these to something else, although I'm generally terrible with naming.. https://codereview.chromium.org/2612083002/diff/350001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:135: // TODO(staraz): Implement this. On 2017/01/27 20:27:02, StarAZ1 wrote: > On 2017/01/26 22:39:40, boliu wrote: > > TODO needed? Looking at the old code, no-op seems like a perfectly valid > > implementation > > This is one step towards fsamuel@'s vision of unified CompositorFrameSinks. > There's a bit of redundancy between cc::CompositorFrameSink and > cc::mojom::MojoCompositorFrameSink due to the subtle differences in their > usages. This code might change again soon. So this is correct until the two frame sinks merge? That's not really a TODO, at least not on this method. https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:745: if (!has_compositor_frame_sink_) { reverse the the condition and flip the body, it's eaiser to read https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:750: DCHECK(pending_child_frame_sink_ids_.empty()); this DCHECK clearly doesn't hold when called from DidInitializeCompositorFrameSink https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:753: ->RegisterFrameSinkHierarchy(frame_sink_id_, frame_sink_id); it's not immediately clear to me why this can't be called when there is no frame sink for this compositor? https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1492: static_cast<CompositorImpl*>(compositor) nit: cast in the assignment line also, what's the argument for this going through the browser compositor? (not that it's wrong or anything)
The CQ bit was checked by staraz@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/2612083002/diff/370001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:745: if (!has_compositor_frame_sink_) { On 2017/01/27 23:17:59, boliu wrote: > reverse the the condition and flip the body, it's eaiser to read Done. https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:750: DCHECK(pending_child_frame_sink_ids_.empty()); On 2017/01/27 23:17:59, boliu wrote: > this DCHECK clearly doesn't hold when called from > DidInitializeCompositorFrameSink My bad. I was thinking of two ways to approach this and got mixed. It's removed. https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:753: ->RegisterFrameSinkHierarchy(frame_sink_id_, frame_sink_id); On 2017/01/27 23:17:59, boliu wrote: > it's not immediately clear to me why this can't be called when there is no frame > sink for this compositor? ~CompositorFrameSinkSupport() calls InvalidateFrameSinkId(). The FrameSinkId will be invalid until a DirectCompositorFrameSink is attached again. https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1492: static_cast<CompositorImpl*>(compositor) On 2017/01/27 23:17:59, boliu wrote: > nit: cast in the assignment line > > also, what's the argument for this going through the browser compositor? (not > that it's wrong or anything) Done. In the case for desktop chrome, there's a mojo message pipe between the browser process and gpu process. Mojo would automatically queue messages like AddChileFrameSink() until a CompositorFrameSink is attached. We decided to manage manually the queue here so that it'll be easier to unify all CompositorFrameSinks later on.
https://codereview.chromium.org/2612083002/diff/390001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:23: display_(std::move(display)), Move to GpuDisplayCompositorFrameSInk https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:24: display_begin_frame_source_(std::move(begin_frame_source)), Move to GpuDisplayCompositorFrameSInk https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:36: display_->SetVisible(true); Move to GpuDisplayCompositorFrameSink. No need to check if we have a display there. https://codereview.chromium.org/2612083002/diff/390001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.h (right): https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.h:77: std::unique_ptr<cc::Display> display_; Move this to GpuDisplayCompositorFrameSink. The comment above becomes irrelevant because by definition a GpuDisplayCompositorFrameSink has a display. https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.h:78: std::unique_ptr<cc::BeginFrameSource> display_begin_frame_source_; Move this to GpuDisplayCompositorFrameSink. https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (left): https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:674: Please avoid unnecessary code churn and restore this line. https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:537: for (auto& frame_sink_id : pending_child_frame_sink_ids_) { no need for {, } if the block is a single statement. https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:744: void CompositorImpl::AddChildFrameSink(cc::FrameSinkId frame_sink_id) { const cc::FrameSinkId& https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:61: void AddChildFrameSink(cc::FrameSinkId frame_sink_id); const cc::FrameSinkId& https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1495: if (compositor) { No need for {, } if block is a single statement.
https://codereview.chromium.org/2612083002/diff/390001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/390001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:143: if (support_) I'm guessing this is probably broken. If you need this guard then that suggests you probably have a start up or tear down issue. Either way, I think you should store |needs_begin_frame| in a local variable, and pass it into |support_| immediately when you create |support_|. That way, you don't miss a NeedsBeginFrame and neglect to generate new frames.
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...)
https://codereview.chromium.org/2612083002/diff/390001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/390001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:72: support_->SetNeedsSyncPoints(capabilities_.delegated_sync_points_required); Another question to danakj@. Does it hurt to always use sync points at this point and not introduce this API? DirectCompositorFrameSink will no longer be direct and so we'll always need sync points. Can we drop this API now and just have sync points always on?
https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1492: static_cast<CompositorImpl*>(compositor) On 2017/01/29 14:58:51, StarAZ1 wrote: > On 2017/01/27 23:17:59, boliu wrote: > > nit: cast in the assignment line > > > > also, what's the argument for this going through the browser compositor? (not > > that it's wrong or anything) > > Done. > > In the case for desktop chrome, there's a mojo message pipe between the browser > process and gpu process. Mojo would automatically queue messages like > AddChileFrameSink() until a CompositorFrameSink is attached. > > We decided to manage manually the queue here so that it'll be easier to unify > all CompositorFrameSinks later on. That doesn't actually answer my question Is the answer that desktop also goes through browser compositor as well, and this is just matching desktop? https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:535: compositor_frame_sink_request_pending_ = true; why flip this? https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1493: content::CompositorImpl* compositor = nit: no need for content::
The CQ bit was checked by staraz@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/2612083002/diff/390001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/390001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:143: if (support_) On 2017/01/29 15:52:56, Fady Samuel wrote: > I'm guessing this is probably broken. If you need this guard then that suggests > you probably have a start up or tear down issue. Either way, I think you should > store |needs_begin_frame| in a local variable, and pass it into |support_| > immediately when you create |support_|. That way, you don't miss a > NeedsBeginFrame and neglect to generate new frames. Done. https://codereview.chromium.org/2612083002/diff/390001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:23: display_(std::move(display)), On 2017/01/29 15:43:01, Fady Samuel wrote: > Move to GpuDisplayCompositorFrameSInk Done. https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:24: display_begin_frame_source_(std::move(begin_frame_source)), On 2017/01/29 15:43:01, Fady Samuel wrote: > Move to GpuDisplayCompositorFrameSInk Done. https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:36: display_->SetVisible(true); On 2017/01/29 15:43:01, Fady Samuel wrote: > Move to GpuDisplayCompositorFrameSink. No need to check if we have a display > there. Done. https://codereview.chromium.org/2612083002/diff/390001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.h (right): https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.h:77: std::unique_ptr<cc::Display> display_; On 2017/01/29 15:43:01, Fady Samuel wrote: > Move this to GpuDisplayCompositorFrameSink. The comment above becomes irrelevant > because by definition a GpuDisplayCompositorFrameSink has a display. Done. https://codereview.chromium.org/2612083002/diff/390001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.h:78: std::unique_ptr<cc::BeginFrameSource> display_begin_frame_source_; On 2017/01/29 15:43:01, Fady Samuel wrote: > Move this to GpuDisplayCompositorFrameSink. Done. https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:535: compositor_frame_sink_request_pending_ = true; On 2017/01/30 17:20:22, boliu wrote: > why flip this? Done. https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:537: for (auto& frame_sink_id : pending_child_frame_sink_ids_) { On 2017/01/29 15:43:01, Fady Samuel wrote: > no need for {, } if the block is a single statement. Done. https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:744: void CompositorImpl::AddChildFrameSink(cc::FrameSinkId frame_sink_id) { On 2017/01/29 15:43:01, Fady Samuel wrote: > const cc::FrameSinkId& Done. https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:61: void AddChildFrameSink(cc::FrameSinkId frame_sink_id); On 2017/01/29 15:43:02, Fady Samuel wrote: > const cc::FrameSinkId& Done. https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1493: content::CompositorImpl* compositor = On 2017/01/30 17:20:22, boliu wrote: > nit: no need for content:: Done. https://codereview.chromium.org/2612083002/diff/390001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1495: if (compositor) { On 2017/01/29 15:43:02, Fady Samuel wrote: > No need for {, } if block is a single statement. Done.
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...)
https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1492: static_cast<CompositorImpl*>(compositor) On 2017/01/30 17:20:22, boliu wrote: > On 2017/01/29 14:58:51, StarAZ1 wrote: > > On 2017/01/27 23:17:59, boliu wrote: > > > nit: cast in the assignment line > > > > > > also, what's the argument for this going through the browser compositor? > (not > > > that it's wrong or anything) > > > > Done. > > > > In the case for desktop chrome, there's a mojo message pipe between the > browser > > process and gpu process. Mojo would automatically queue messages like > > AddChileFrameSink() until a CompositorFrameSink is attached. > > > > We decided to manage manually the queue here so that it'll be easier to unify > > all CompositorFrameSinks later on. > > That doesn't actually answer my question > > Is the answer that desktop also goes through browser compositor as well, and > this is just matching desktop? this is still unanswered? https://codereview.chromium.org/2612083002/diff/430001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/430001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1521: if (delegated_frame_host_ && compositor) delegated_frame_host_ check is redundant if you check compositor
https://codereview.chromium.org/2612083002/diff/430001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/430001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:75: support_->SetNeedsSyncPoints(capabilities_.delegated_sync_points_required); Make this a constructor parameter. https://codereview.chromium.org/2612083002/diff/430001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.h (right): https://codereview.chromium.org/2612083002/diff/430001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.h:84: bool needs_begin_frame_; Initialize this? bool needs_begin_frame_ = false;
https://codereview.chromium.org/2612083002/diff/430001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/430001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:75: support_->SetNeedsSyncPoints(capabilities_.delegated_sync_points_required); On 2017/01/30 21:22:04, Fady Samuel wrote: > Make this a constructor parameter. Done. https://codereview.chromium.org/2612083002/diff/430001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.h (right): https://codereview.chromium.org/2612083002/diff/430001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.h:84: bool needs_begin_frame_; On 2017/01/30 21:22:04, Fady Samuel wrote: > Initialize this? bool needs_begin_frame_ = false; Done. https://codereview.chromium.org/2612083002/diff/430001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/430001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1521: if (delegated_frame_host_ && compositor) On 2017/01/30 21:13:51, boliu wrote: > delegated_frame_host_ check is redundant if you check compositor Done.
The CQ bit was checked by fsamuel@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/2612083002/diff/370001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2612083002/diff/370001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1492: static_cast<CompositorImpl*>(compositor) On 2017/01/30 21:13:51, boliu wrote: > On 2017/01/30 17:20:22, boliu wrote: > > On 2017/01/29 14:58:51, StarAZ1 wrote: > > > On 2017/01/27 23:17:59, boliu wrote: > > > > nit: cast in the assignment line > > > > > > > > also, what's the argument for this going through the browser compositor? > > (not > > > > that it's wrong or anything) > > > > > > Done. > > > > > > In the case for desktop chrome, there's a mojo message pipe between the > > browser > > > process and gpu process. Mojo would automatically queue messages like > > > AddChileFrameSink() until a CompositorFrameSink is attached. > > > > > > We decided to manage manually the queue here so that it'll be easier to > unify > > > all CompositorFrameSinks later on. > > > > That doesn't actually answer my question > > > > Is the answer that desktop also goes through browser compositor as well, and > > this is just matching desktop? > > this is still unanswered? We cannot currently register a BeginFrame hierarchy until the parent has created a CompositorFrameSink. The parent knows when it has created a CompositorFrameSink so setting up registration there seems to make sense. This simplifies display compositor restart, because we can easily call AddChildFrameSinks whenever until the parent creates a CompositorFrameSink.
lgtm if all bots are green.
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...)
https://codereview.chromium.org/2612083002/diff/450001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/450001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:48: void SetNeedsSyncPoints(bool needs_sync_points); nit: delete. https://codereview.chromium.org/2612083002/diff/450001/ui/android/window_andr... File ui/android/window_android.cc (right): https://codereview.chromium.org/2612083002/diff/450001/ui/android/window_andr... ui/android/window_android.cc:182: compositor_ = NULL; nit: nullptr while you're moving this code.
The CQ bit was checked by staraz@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/2612083002/diff/450001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/450001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:48: void SetNeedsSyncPoints(bool needs_sync_points); On 2017/01/31 15:43:59, Fady Samuel wrote: > nit: delete. Done. https://codereview.chromium.org/2612083002/diff/450001/ui/android/window_andr... File ui/android/window_android.cc (right): https://codereview.chromium.org/2612083002/diff/450001/ui/android/window_andr... ui/android/window_android.cc:182: compositor_ = NULL; On 2017/01/31 15:43:59, Fady Samuel wrote: > nit: nullptr while you're moving this code. Done.
The CQ bit was checked by fsamuel@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 ========== WIP: DirectCompositorFrameSink uses CompositorFrameSinkSupport BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== DirectCompositorFrameSink uses CompositorFrameSinkSupport for its implementation, similar to what GpuCompositorFrameSink is doing. Unlike GpuCompositorFrameSink, DirectCompositorFrameSink creates CompositorFrameSinkSupport in BindToClient() and destroys it in DetachFrameClient(). DirectCompositorFrameSink holds a FrameSinkId but it won't be valid until it has a CompositorFrameSinkSupport. DirectCompositorFrameSink doesn't own a cc::Display or the BeginFrameSource of the Display, instead, PerCompositorData owns them. Therefore, CompositorFrameSinkSupport now holds a cc::Display*. GpuDisplayCompositorFrameSink owns the Display and its BeginFrameSource on that side. DelegateFrameHostAndroid used to handle FrameSink hierarchies. That causes problem since the FrameSinkId isn't valid anymore when DirectCompositorFrameSink isn't bound to a client. CompositorImpl now has AddChildFrameSink() and RemoveChildFrameSink() to replay RegisterFrameSinkHierarchy() and UnregisterFrameSinkHierarchy() in DelegatedFrameHostAndroid. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== DirectCompositorFrameSink uses CompositorFrameSinkSupport for its implementation, similar to what GpuCompositorFrameSink is doing. Unlike GpuCompositorFrameSink, DirectCompositorFrameSink creates CompositorFrameSinkSupport in BindToClient() and destroys it in DetachFrameClient(). DirectCompositorFrameSink holds a FrameSinkId but it won't be valid until it has a CompositorFrameSinkSupport. DirectCompositorFrameSink doesn't own a cc::Display or the BeginFrameSource of the Display, instead, PerCompositorData owns them. Therefore, CompositorFrameSinkSupport now holds a cc::Display*. GpuDisplayCompositorFrameSink owns the Display and its BeginFrameSource on that side. DelegateFrameHostAndroid used to handle FrameSink hierarchies. That causes problem since the FrameSinkId isn't valid anymore when DirectCompositorFrameSink isn't bound to a client. CompositorImpl now has AddChildFrameSink() and RemoveChildFrameSink() to replay RegisterFrameSinkHierarchy() and UnregisterFrameSinkHierarchy() in DelegatedFrameHostAndroid. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport holds implementations for both GpuCompositorFrameSink and DirectCompositorFrameSink. Implementation of DirectCompositorFrameSink and GpuCompositorFrameSink are similar. Letting them both use CompositorFrameSinkSupport reduces code redundancy. DirectCompositorFrameSink doesn't own a cc::Display or the BeginFrameSource of the Display, instead, PerCompositorData owns them. Therefore, CompositorFrameSinkSupport now holds a cc::Display*. GpuDisplayCompositorFrameSink owns the Display and its BeginFrameSource on that side. DelegateFrameHostAndroid used to handle FrameSink hierarchies. That causes problem since the FrameSinkId isn't valid anymore when DirectCompositorFrameSink isn't bound to a client. CompositorImpl now has AddChildFrameSink() and RemoveChildFrameSink() to replace RegisterFrameSinkHierarchy() and UnregisterFrameSinkHierarchy() in DelegatedFrameHostAndroid. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
> CompositorFrameSinkSupport holds implementations for both > GpuCompositorFrameSink and DirectCompositorFrameSink. I think this means to say "CFSS is used to implement both..." Just read a bit so far, onward. https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:65: if (display_) { why this reordering? https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:91: random whitespace? https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:29: CompositorFrameSinkSupport(CompositorFrameSinkSupportClient* client, Comment: When can |display| be null? https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:80: Display* display_; this is a const variable right, make it so? i'm not a fan of this variable-whitespace-variable-whitespace-repeat pattern going on in this class, it makes it 2x taller than needed. maybe group all the const things together at the least?
Description was changed from ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport holds implementations for both GpuCompositorFrameSink and DirectCompositorFrameSink. Implementation of DirectCompositorFrameSink and GpuCompositorFrameSink are similar. Letting them both use CompositorFrameSinkSupport reduces code redundancy. DirectCompositorFrameSink doesn't own a cc::Display or the BeginFrameSource of the Display, instead, PerCompositorData owns them. Therefore, CompositorFrameSinkSupport now holds a cc::Display*. GpuDisplayCompositorFrameSink owns the Display and its BeginFrameSource on that side. DelegateFrameHostAndroid used to handle FrameSink hierarchies. That causes problem since the FrameSinkId isn't valid anymore when DirectCompositorFrameSink isn't bound to a client. CompositorImpl now has AddChildFrameSink() and RemoveChildFrameSink() to replace RegisterFrameSinkHierarchy() and UnregisterFrameSinkHierarchy() in DelegatedFrameHostAndroid. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. DirectCompositorFrameSink doesn't own a cc::Display or the BeginFrameSource of the Display, instead, PerCompositorData owns them. Therefore, CompositorFrameSinkSupport now holds a cc::Display*. GpuDisplayCompositorFrameSink owns the Display and its BeginFrameSource on that side. DelegateFrameHostAndroid used to handle FrameSink hierarchies. That causes problem since the FrameSinkId isn't valid anymore when DirectCompositorFrameSink isn't bound to a client. CompositorImpl now has AddChildFrameSink() and RemoveChildFrameSink() to replace RegisterFrameSinkHierarchy() and UnregisterFrameSinkHierarchy() in DelegatedFrameHostAndroid. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. DirectCompositorFrameSink doesn't own a cc::Display or the BeginFrameSource of the Display, instead, PerCompositorData owns them. Therefore, CompositorFrameSinkSupport now holds a cc::Display*. GpuDisplayCompositorFrameSink owns the Display and its BeginFrameSource on that side. DelegateFrameHostAndroid used to handle FrameSink hierarchies. That causes problem since the FrameSinkId isn't valid anymore when DirectCompositorFrameSink isn't bound to a client. CompositorImpl now has AddChildFrameSink() and RemoveChildFrameSink() to replace RegisterFrameSinkHierarchy() and UnregisterFrameSinkHierarchy() in DelegatedFrameHostAndroid. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. This CL takes us in the direction of splitting DirectCompositorFrameSink into two pieces: a client side component and a service side component. DelegateFrameHostAndroid used to handle FrameSink hierarchies. That causes problem since the FrameSinkId isn't valid anymore when DirectCompositorFrameSink isn't bound to a client. CompositorImpl now has AddChildFrameSink() and RemoveChildFrameSink() to replace RegisterFrameSinkHierarchy() and UnregisterFrameSinkHierarchy() in DelegatedFrameHostAndroid. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:123: if (client_) how will client be null? https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:133: if (client_) how will client be null? https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:123: valid_frame_sink_ids_.insert(frame_sink_id); Why this change? https://codereview.chromium.org/2612083002/diff/490001/components/display_com... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/490001/components/display_com... components/display_compositor/gpu_display_compositor_frame_sink.cc:33: display_->SetVisible(true); Why is this?
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...)
https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:65: if (display_) { On 2017/01/31 19:25:01, danakj (slow) wrote: > why this reordering? The reordering was to fix this crash in the state_machine: https://paste.googleplex.com/5211561517907968 https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:91: On 2017/01/31 19:25:01, danakj (slow) wrote: > random whitespace? Done. https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:29: CompositorFrameSinkSupport(CompositorFrameSinkSupportClient* client, On 2017/01/31 19:25:01, danakj (slow) wrote: > Comment: When can |display| be null? Done. https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:80: Display* display_; On 2017/01/31 19:25:01, danakj (slow) wrote: > this is a const variable right, make it so? > > i'm not a fan of this variable-whitespace-variable-whitespace-repeat pattern > going on in this class, it makes it 2x taller than needed. maybe group all the > const things together at the least? Done.
https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:65: if (display_) { On 2017/01/31 20:34:18, StarAZ1 wrote: > On 2017/01/31 19:25:01, danakj (slow) wrote: > > why this reordering? > > The reordering was to fix this crash in the state_machine: > https://paste.googleplex.com/5211561517907968 That crash is from CompositorFrameSinkSupport::SubmitCompositorFrame and Scheduler::DidReceiveCompositorFrameAck happening on the same stack frame right? Normally we prevent that by posting acks back. How does this fix it and why is it the right way to fix?
https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:65: if (display_) { On 2017/01/31 20:41:38, danakj (slow) wrote: > On 2017/01/31 20:34:18, StarAZ1 wrote: > > On 2017/01/31 19:25:01, danakj (slow) wrote: > > > why this reordering? > > > > The reordering was to fix this crash in the state_machine: > > https://paste.googleplex.com/5211561517907968 > > That crash is from CompositorFrameSinkSupport::SubmitCompositorFrame and > Scheduler::DidReceiveCompositorFrameAck happening on the same stack frame right? > Normally we prevent that by posting acks back. How does this fix it and why is > it the right way to fix? I don't know. I did the reordering because SetLocalSurfaceId() is called before SubmitCompositorFrame() in DirectCompositorFrameSink::SubmitCompositorFrame() currently, and that fixed the crash. https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:123: if (client_) On 2017/01/31 20:19:17, danakj (slow) wrote: > how will client be null? client_ is set to nullptr in CompositorFrameSink::DetachFromeClient(). https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:133: if (client_) On 2017/01/31 20:19:17, danakj (slow) wrote: > how will client be null? client_ is set to nullptr in CompositorFrameSink::DetachFromeClient(). https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:123: valid_frame_sink_ids_.insert(frame_sink_id); On 2017/01/31 20:19:17, danakj (slow) wrote: > Why this change? From fsamuel's earlier comment: ui::Compositor Registers the FrameSinkId first: https://cs.chromium.org/chromium/src/ui/compositor/compositor.cc?sq=package:c... However, we eventually want to get rid of that in favor of the SurfaceReference approach to reference tracking. We're not there yet. For the time being, we are registering the FrameSinkId in CompositorFrameSinkSupport. However, while we're using SurfaceSequences, we cannot get rid of RegisterFrameSinkId in ui::Compositor, so I thought getting rid of the DCHECK would be easier for now. Note that once SurfaceReferences are used everywhere, RegisterFrameSinkId and InvalidateFrameSinkId will likely go away. https://codereview.chromium.org/2612083002/diff/490001/components/display_com... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/490001/components/display_com... components/display_compositor/gpu_display_compositor_frame_sink.cc:33: display_->SetVisible(true); On 2017/01/31 20:19:17, danakj (slow) wrote: > Why is this? DirectCompositorFrameSink doesn't call SetVisible so I am moving it here from CompositorFrameSinkSupport.
https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:65: if (display_) { On 2017/01/31 21:09:30, StarAZ1 wrote: > On 2017/01/31 20:41:38, danakj (slow) wrote: > > On 2017/01/31 20:34:18, StarAZ1 wrote: > > > On 2017/01/31 19:25:01, danakj (slow) wrote: > > > > why this reordering? > > > > > > The reordering was to fix this crash in the state_machine: > > > https://paste.googleplex.com/5211561517907968 > > > > That crash is from CompositorFrameSinkSupport::SubmitCompositorFrame and > > Scheduler::DidReceiveCompositorFrameAck happening on the same stack frame > right? > > Normally we prevent that by posting acks back. How does this fix it and why is > > it the right way to fix? > > I don't know. I did the reordering because SetLocalSurfaceId() is > called before SubmitCompositorFrame() in > DirectCompositorFrameSink::SubmitCompositorFrame() currently, and that > fixed the crash. > Er, well, I don't think that's a very good motivation for reordering them :/ Can you look at the call stacks and explain why this fixes it for me? Or look at the history there maybe it would explain. If you're stuck I can help out lmk. https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:123: if (client_) On 2017/01/31 21:09:30, StarAZ1 wrote: > On 2017/01/31 20:19:17, danakj (slow) wrote: > > how will client be null? > > client_ is set to nullptr in CompositorFrameSink::DetachFromeClient(). Sure, but if you're detached how is it calling this? This is the client of CompositorFrameSinkSupport which is destroyed in there. https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:123: valid_frame_sink_ids_.insert(frame_sink_id); On 2017/01/31 21:09:30, StarAZ1 wrote: > On 2017/01/31 20:19:17, danakj (slow) wrote: > > Why this change? > > From fsamuel's earlier comment: > > ui::Compositor Registers the FrameSinkId first: > https://cs.chromium.org/chromium/src/ui/compositor/compositor.cc?sq=package:c... > > However, we eventually want to get rid of that in favor of the SurfaceReference > approach to reference tracking. We're not there yet. For the time being, we are > registering the FrameSinkId in CompositorFrameSinkSupport. However, while we're > using SurfaceSequences, we cannot get rid of RegisterFrameSinkId in > ui::Compositor, so I thought getting rid of the DCHECK would be easier for now. Can we have a parameter passed around maybe saying to use refs or surfaces. And ui::Compositor would register if appropriate, and CompositorFrameSinkSupport would if appropriate? Or would that be insane? > > Note that once SurfaceReferences are used everywhere, RegisterFrameSinkId and > InvalidateFrameSinkId will likely go away. FWIW, likely shouldn't motivate code too much, as it's also likely to change.
The CQ bit was checked by staraz@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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2612083002/diff/530001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/530001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:37: bool needs_sync_points = true); Why introduce one more argument "needs_sync_points" in the constructor of CompositorFrameSinkSupport in this CL when all of the use cases use the default value "true"? SurfaceFactory already sets it to "true" at default.
https://codereview.chromium.org/2612083002/diff/550001/ui/android/delegated_f... File ui/android/delegated_frame_host_android.h (right): https://codereview.chromium.org/2612083002/diff/550001/ui/android/delegated_f... ui/android/delegated_frame_host_android.h:76: void AttachToCompositor(content::CompositorImpl* compositor); This doesn't work. ui/android cannot depend on content. Maybe make thi sui::WindowAndroidCompositor instead?
https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:65: if (display_) { On 2017/01/31 21:49:40, danakj (slow) wrote: > On 2017/01/31 21:09:30, StarAZ1 wrote: > > On 2017/01/31 20:41:38, danakj (slow) wrote: > > > On 2017/01/31 20:34:18, StarAZ1 wrote: > > > > On 2017/01/31 19:25:01, danakj (slow) wrote: > > > > > why this reordering? > > > > > > > > The reordering was to fix this crash in the state_machine: > > > > https://paste.googleplex.com/5211561517907968 > > > > > > That crash is from CompositorFrameSinkSupport::SubmitCompositorFrame and > > > Scheduler::DidReceiveCompositorFrameAck happening on the same stack frame > > right? > > > Normally we prevent that by posting acks back. How does this fix it and why > is > > > it the right way to fix? > > > > I don't know. I did the reordering because SetLocalSurfaceId() is > > called before SubmitCompositorFrame() in > > DirectCompositorFrameSink::SubmitCompositorFrame() currently, and that > > fixed the crash. > > > > Er, well, I don't think that's a very good motivation for reordering them :/ Can > you look at the call stacks and explain why this fixes it for me? Or look at the > history there maybe it would explain. If you're stuck I can help out lmk. Like we've discussed on slack, CompositorFrameSinkSupport got away with a weird ordering of SetLocalSurfaceId / SubmitCompositorFrame because we only used that code in Mus Window Server, and it did nothing in DidReceiveCompositorFrameAck. https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:123: if (client_) On 2017/01/31 21:49:40, danakj (slow) wrote: > On 2017/01/31 21:09:30, StarAZ1 wrote: > > On 2017/01/31 20:19:17, danakj (slow) wrote: > > > how will client be null? > > > > client_ is set to nullptr in CompositorFrameSink::DetachFromeClient(). > > Sure, but if you're detached how is it calling this? This is the client of > CompositorFrameSinkSupport which is destroyed in there. Good point. I removed the guards. https://codereview.chromium.org/2612083002/diff/550001/ui/android/delegated_f... File ui/android/delegated_frame_host_android.h (right): https://codereview.chromium.org/2612083002/diff/550001/ui/android/delegated_f... ui/android/delegated_frame_host_android.h:76: void AttachToCompositor(content::CompositorImpl* compositor); On 2017/02/02 00:07:43, Fady Samuel wrote: > This doesn't work. ui/android cannot depend on content. Maybe make thi > sui::WindowAndroidCompositor instead? AttachToCompositor calls compositor->AddChildFrameSink(). boliu wasn't happy about adding AddChildFrameSink() to WindowAndroidCompositor. According to him, "this is an interface for WindowAndroid to call up to the compositor. WindowAndroid isn't using this method". I'll ask him if there's any other way.
https://codereview.chromium.org/2612083002/diff/530001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/530001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:37: bool needs_sync_points = true); On 2017/02/01 22:46:06, xlai (Olivia) wrote: > Why introduce one more argument "needs_sync_points" in the constructor of > CompositorFrameSinkSupport in this CL when all of the use cases use the default > value "true"? SurfaceFactory already sets it to "true" at default. DirectCompositorFrameSink() sets it to false for optimization purposes since display and ui compositor are sharing the same GL context.
https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:59: DCHECK(thread_checker_.CalledOnValidThread()); Duplicate DCHECK as in CompositorFrameSink::BindToClient? https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:73: support_ = base::MakeUnique<CompositorFrameSinkSupport>( Ah, I missed that here creates a unique pointer of CompositorFrameSinkSupport, instead of being in the constructor like the other use cases. I notice that all the arguments needed for creating the support_ are already provided in DirectCompositorFrameSink's constructor; is there any specific reason why you delay the creation until this function? If this is intentional then I think you should put "if (support_) {...}" in the rest of functions that use support_ or DCHECK to prevent nullpointer access.
The CQ bit was checked by staraz@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/2612083002/diff/550001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:73: support_ = base::MakeUnique<CompositorFrameSinkSupport>( On 2017/02/02 06:48:44, xlai (Olivia) wrote: > Ah, I missed that here creates a unique pointer of CompositorFrameSinkSupport, > instead of being in the constructor like the other use cases. I notice that all > the arguments needed for creating the support_ are already provided in > DirectCompositorFrameSink's constructor; is there any specific reason why you > delay the creation until this function? If this is intentional then I think you > should put "if (support_) {...}" in the rest of functions that use support_ or > DCHECK to prevent nullpointer access. It is intentional. Originally, DirectCompositorFrameSink registers and unregisters SurfaceFactoryClient (and other initializations) in BindToClient and DetachFromClient() to ensure that only one client is alive for the namespace at any given time. It makes sense to create CompositorFrameSinkSupport in BindToClient() because most of the initializations are in CompositorFrameSinkSupport(). The if (support_) is not needed since DirectCompositorFrameSink has support_ whenever it is bound to a client and those methods are called by its client only.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
https://codereview.chromium.org/2612083002/diff/570001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/2612083002/diff/570001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:107: void AddChildFrameSink(const cc::FrameSinkId& frame_sink_id); Add override; to the end. https://codereview.chromium.org/2612083002/diff/570001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:108: void RemoveChildFrameSink(const cc::FrameSinkId& frame_sink_id); Add override; to the end.
The CQ bit was checked by staraz@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/2612083002/diff/550001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:59: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/02/02 06:48:44, xlai (Olivia) wrote: > Duplicate DCHECK as in CompositorFrameSink::BindToClient? danakj thinks it makes sense to get rid of the thread checker in the base class. I will do it in a separate CL. https://codereview.chromium.org/2612083002/diff/570001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.h (right): https://codereview.chromium.org/2612083002/diff/570001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:107: void AddChildFrameSink(const cc::FrameSinkId& frame_sink_id); On 2017/02/02 16:40:26, Fady Samuel wrote: > Add override; to the end. Done. https://codereview.chromium.org/2612083002/diff/570001/content/browser/render... content/browser/renderer_host/compositor_impl_android.h:108: void RemoveChildFrameSink(const cc::FrameSinkId& frame_sink_id); On 2017/02/02 16:40:26, Fady Samuel wrote: > Add override; to the end. Done.
https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:73: support_ = base::MakeUnique<CompositorFrameSinkSupport>( On 2017/02/02 06:48:44, xlai (Olivia) wrote: > Ah, I missed that here creates a unique pointer of CompositorFrameSinkSupport, > instead of being in the constructor like the other use cases. I notice that all > the arguments needed for creating the support_ are already provided in > DirectCompositorFrameSink's constructor; is there any specific reason why you > delay the creation until this function? If this is intentional then I think you > should put "if (support_) {...}" in the rest of functions that use support_ or > DCHECK to prevent nullpointer access. FWIW I guess isn't spelled out very well in comments here https://cs.chromium.org/chromium/src/cc/output/compositor_frame_sink.h but this class is not able to be used until BindToClient is called. That should always be the first method used.
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_...)
https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:59: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/02/02 17:01:56, StarAZ1 wrote: > On 2017/02/02 06:48:44, xlai (Olivia) wrote: > > Duplicate DCHECK as in CompositorFrameSink::BindToClient? > > danakj thinks it makes sense to get rid of the thread checker in the > base class. I will do it in a separate CL. Okay. https://codereview.chromium.org/2612083002/diff/550001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:73: support_ = base::MakeUnique<CompositorFrameSinkSupport>( On 2017/02/02 17:03:34, danakj (slow) wrote: > On 2017/02/02 06:48:44, xlai (Olivia) wrote: > > Ah, I missed that here creates a unique pointer of CompositorFrameSinkSupport, > > instead of being in the constructor like the other use cases. I notice that > all > > the arguments needed for creating the support_ are already provided in > > DirectCompositorFrameSink's constructor; is there any specific reason why you > > delay the creation until this function? If this is intentional then I think > you > > should put "if (support_) {...}" in the rest of functions that use support_ or > > DCHECK to prevent nullpointer access. > > FWIW I guess isn't spelled out very well in comments here > https://cs.chromium.org/chromium/src/cc/output/compositor_frame_sink.h but this > class is not able to be used until BindToClient is called. That should always be > the first method used. I see. lgtm on this and for offscreen_canvas_compositor_frame_sink.
The CQ bit was checked by staraz@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 staraz@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/2612083002/diff/490001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:123: valid_frame_sink_ids_.insert(frame_sink_id); On 2017/01/31 21:49:40, danakj (slow) wrote: > On 2017/01/31 21:09:30, StarAZ1 wrote: > > On 2017/01/31 20:19:17, danakj (slow) wrote: > > > Why this change? > > > > From fsamuel's earlier comment: > > > > ui::Compositor Registers the FrameSinkId first: > > > https://cs.chromium.org/chromium/src/ui/compositor/compositor.cc?sq=package:c... > > > > However, we eventually want to get rid of that in favor of the > SurfaceReference > > approach to reference tracking. We're not there yet. For the time being, we > are > > registering the FrameSinkId in CompositorFrameSinkSupport. However, while > we're > > using SurfaceSequences, we cannot get rid of RegisterFrameSinkId in > > ui::Compositor, so I thought getting rid of the DCHECK would be easier for > now. > > Can we have a parameter passed around maybe saying to use refs or surfaces. And > ui::Compositor would register if appropriate, and CompositorFrameSinkSupport > would if appropriate? Or would that be insane? > > > > > Note that once SurfaceReferences are used everywhere, RegisterFrameSinkId and > > InvalidateFrameSinkId will likely go away. > > FWIW, likely shouldn't motivate code too much, as it's also likely to change. I'm not seeing anything in the replies to this, did I miss it? And how would we ensure this check came back in a timely manner if we remove it now? https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:30: // CompositorFrames to a buffer instead of a DisplayCompositor. buffer => offscreen texture/bitmap https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:32: // GpuOffscreenCompositorframeSink. Frame https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:145: if (support_) If you make the BFS after the support and destroy it before, you don't need this or the needs_begin_frame_, why did you choose this way?
https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:123: valid_frame_sink_ids_.insert(frame_sink_id); On 2017/02/03 18:48:43, danakj (slow) wrote: > On 2017/01/31 21:49:40, danakj (slow) wrote: > > On 2017/01/31 21:09:30, StarAZ1 wrote: > > > On 2017/01/31 20:19:17, danakj (slow) wrote: > > > > Why this change? > > > > > > From fsamuel's earlier comment: > > > > > > ui::Compositor Registers the FrameSinkId first: > > > > > > https://cs.chromium.org/chromium/src/ui/compositor/compositor.cc?sq=package:c... > > > > > > However, we eventually want to get rid of that in favor of the > > SurfaceReference > > > approach to reference tracking. We're not there yet. For the time being, we > > are > > > registering the FrameSinkId in CompositorFrameSinkSupport. However, while > > we're > > > using SurfaceSequences, we cannot get rid of RegisterFrameSinkId in > > > ui::Compositor, so I thought getting rid of the DCHECK would be easier for > > now. > > > > Can we have a parameter passed around maybe saying to use refs or surfaces. > And > > ui::Compositor would register if appropriate, and CompositorFrameSinkSupport > > would if appropriate? Or would that be insane? > > > > > > > > Note that once SurfaceReferences are used everywhere, RegisterFrameSinkId > and > > > InvalidateFrameSinkId will likely go away. > > > > FWIW, likely shouldn't motivate code too much, as it's also likely to change. > > I'm not seeing anything in the replies to this, did I miss it? And how would we > ensure this check came back in a timely manner if we remove it now? I added a flag to CompositorFrameSinkSupport in patchset 15 where CompositorFrameSinkSupport wouldn't register or invalidate FrameSinkId if it's supporting DCFS. fsamuel wasn't happy about using the flag as he'd like to go with the design where CFSSupport manages FrameSinkId. Would adding a "TODO: DCHECK if the insert is successful." be enough assurance?
https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:30: // CompositorFrames to a buffer instead of a DisplayCompositor. On 2017/02/03 18:48:43, danakj (slow) wrote: > buffer => offscreen texture/bitmap Done. https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:32: // GpuOffscreenCompositorframeSink. On 2017/02/03 18:48:43, danakj (slow) wrote: > Frame Done. https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/610001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:145: if (support_) On 2017/02/03 18:48:43, danakj (slow) wrote: > If you make the BFS after the support and destroy it before, you don't need this > or the needs_begin_frame_, why did you choose this way? I didn't realize I could do that. It's done now.
On 2017/02/03 19:32:20, StarAZ1 wrote: > https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... > File cc/surfaces/surface_manager.cc (right): > > https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... > cc/surfaces/surface_manager.cc:123: valid_frame_sink_ids_.insert(frame_sink_id); > On 2017/02/03 18:48:43, danakj (slow) wrote: > > On 2017/01/31 21:49:40, danakj (slow) wrote: > > > On 2017/01/31 21:09:30, StarAZ1 wrote: > > > > On 2017/01/31 20:19:17, danakj (slow) wrote: > > > > > Why this change? > > > > > > > > From fsamuel's earlier comment: > > > > > > > > ui::Compositor Registers the FrameSinkId first: > > > > > > > > > > https://cs.chromium.org/chromium/src/ui/compositor/compositor.cc?sq=package:c... > > > > > > > > However, we eventually want to get rid of that in favor of the > > > SurfaceReference > > > > approach to reference tracking. We're not there yet. For the time being, > we > > > are > > > > registering the FrameSinkId in CompositorFrameSinkSupport. However, while > > > we're > > > > using SurfaceSequences, we cannot get rid of RegisterFrameSinkId in > > > > ui::Compositor, so I thought getting rid of the DCHECK would be easier for > > > now. > > > > > > Can we have a parameter passed around maybe saying to use refs or surfaces. > > And > > > ui::Compositor would register if appropriate, and CompositorFrameSinkSupport > > > would if appropriate? Or would that be insane? > > > > > > > > > > > Note that once SurfaceReferences are used everywhere, RegisterFrameSinkId > > and > > > > InvalidateFrameSinkId will likely go away. > > > > > > FWIW, likely shouldn't motivate code too much, as it's also likely to > change. > > > > I'm not seeing anything in the replies to this, did I miss it? And how would > we > > ensure this check came back in a timely manner if we remove it now? > > I added a flag to CompositorFrameSinkSupport in patchset 15 where > CompositorFrameSinkSupport wouldn't register or invalidate > FrameSinkId if it's supporting DCFS. fsamuel wasn't happy about > using the flag as he'd like to go with the design where CFSSupport > manages FrameSinkId. > > Would adding a "TODO: DCHECK if the insert is successful." be enough assurance? fsamuel says he is okay with having the flag if there is a TODO that points out eventually it won't need to exist and we remove it once it's always a single value. This is not an uncommon strategy we do this with LayerTreeSettings sometimes as well, not all flags are forever.
The CQ bit was checked by staraz@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...
On 2017/02/03 20:42:24, danakj (slow) wrote: > On 2017/02/03 19:32:20, StarAZ1 wrote: > > > https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... > > File cc/surfaces/surface_manager.cc (right): > > > > > https://codereview.chromium.org/2612083002/diff/490001/cc/surfaces/surface_ma... > > cc/surfaces/surface_manager.cc:123: > valid_frame_sink_ids_.insert(frame_sink_id); > > On 2017/02/03 18:48:43, danakj (slow) wrote: > > > On 2017/01/31 21:49:40, danakj (slow) wrote: > > > > On 2017/01/31 21:09:30, StarAZ1 wrote: > > > > > On 2017/01/31 20:19:17, danakj (slow) wrote: > > > > > > Why this change? > > > > > > > > > > From fsamuel's earlier comment: > > > > > > > > > > ui::Compositor Registers the FrameSinkId first: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/ui/compositor/compositor.cc?sq=package:c... > > > > > > > > > > However, we eventually want to get rid of that in favor of the > > > > SurfaceReference > > > > > approach to reference tracking. We're not there yet. For the time being, > > we > > > > are > > > > > registering the FrameSinkId in CompositorFrameSinkSupport. However, > while > > > > we're > > > > > using SurfaceSequences, we cannot get rid of RegisterFrameSinkId in > > > > > ui::Compositor, so I thought getting rid of the DCHECK would be easier > for > > > > now. > > > > > > > > Can we have a parameter passed around maybe saying to use refs or > surfaces. > > > And > > > > ui::Compositor would register if appropriate, and > CompositorFrameSinkSupport > > > > would if appropriate? Or would that be insane? > > > > > > > > > > > > > > Note that once SurfaceReferences are used everywhere, > RegisterFrameSinkId > > > and > > > > > InvalidateFrameSinkId will likely go away. > > > > > > > > FWIW, likely shouldn't motivate code too much, as it's also likely to > > change. > > > > > > I'm not seeing anything in the replies to this, did I miss it? And how would > > we > > > ensure this check came back in a timely manner if we remove it now? > > > > I added a flag to CompositorFrameSinkSupport in patchset 15 where > > CompositorFrameSinkSupport wouldn't register or invalidate > > FrameSinkId if it's supporting DCFS. fsamuel wasn't happy about > > using the flag as he'd like to go with the design where CFSSupport > > manages FrameSinkId. > > > > Would adding a "TODO: DCHECK if the insert is successful." be enough > assurance? > > fsamuel says he is okay with having the flag if there is a TODO that points out > eventually it won't need to exist and we remove it once it's always a single > value. This is not an uncommon strategy we do this with LayerTreeSettings > sometimes as well, not all flags are forever. Done.
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 staraz@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/2612083002/diff/670001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:83: support_->SetNeedsBeginFrame(false); Why? Please explain this.
https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:30: if (!for_direct_compositor_frame_sink_) The naming of this violates layering. It should make sense in the context of this class, not in the context of the class that uses it. something_about_frame_sink_id or so would be better. I'm not sure what word to use to describe this class' relationship to the framesink id but hopefully you have something for it. https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:37: bool needs_sync_points = true, Why do these have default values instead of writing the wishes of each caller explicitly? https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:105: bool for_direct_compositor_frame_sink_; const?
https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:83: support_->SetNeedsBeginFrame(false); On 2017/02/06 03:50:34, Fady Samuel wrote: > Why? Please explain this. patch 34 failed browser_tests on mac with stack trace https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...
https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:83: support_->SetNeedsBeginFrame(false); On 2017/02/06 17:36:52, StarAZ1 wrote: > On 2017/02/06 03:50:34, Fady Samuel wrote: > > Why? Please explain this. > > patch 34 failed browser_tests on mac with stack trace > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel... > Ahh this is a bug in CompositorFrameSinkSupport I think. It's still observing BeginFrames AFTER it's been deleted and so the BeginFrameSource is holding on to an invalid pointer. I would add SetNeedsBeginFrame(false); in CompositorFrameSinkSupport's destructor. Add a comment above that line: // Unregister |this| as a BeginFrameObserver so that the BeginFrameSource does not call into |this| after it's deleted.
Patchset #36 (id:690001) has been deleted
The CQ bit was checked by staraz@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/2612083002/diff/670001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.cc:30: if (!for_direct_compositor_frame_sink_) On 2017/02/06 16:42:58, danakj (slow) wrote: > The naming of this violates layering. It should make sense in the context of > this class, not in the context of the class that uses it. > something_about_frame_sink_id or so would be better. I'm not sure what word to > use to describe this class' relationship to the framesink id but hopefully you > have something for it. Done. https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:37: bool needs_sync_points = true, On 2017/02/06 16:42:58, danakj (slow) wrote: > Why do these have default values instead of writing the wishes of each caller > explicitly? Done. https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:105: bool for_direct_compositor_frame_sink_; On 2017/02/06 16:42:58, danakj (slow) wrote: > const? Done. https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/670001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:83: support_->SetNeedsBeginFrame(false); On 2017/02/06 20:07:46, Fady Samuel wrote: > On 2017/02/06 17:36:52, StarAZ1 wrote: > > On 2017/02/06 03:50:34, Fady Samuel wrote: > > > Why? Please explain this. > > > > patch 34 failed browser_tests on mac with stack trace > > > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel... > > > > Ahh this is a bug in CompositorFrameSinkSupport I think. It's still observing > BeginFrames AFTER it's been deleted and so the BeginFrameSource is holding on to > an invalid pointer. I would add SetNeedsBeginFrame(false); in > CompositorFrameSinkSupport's destructor. > > Add a comment above that line: > > // Unregister |this| as a BeginFrameObserver so that the BeginFrameSource does > not call into |this| after it's deleted. Done.
Description was changed from ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. This CL takes us in the direction of splitting DirectCompositorFrameSink into two pieces: a client side component and a service side component. DelegateFrameHostAndroid used to handle FrameSink hierarchies. That causes problem since the FrameSinkId isn't valid anymore when DirectCompositorFrameSink isn't bound to a client. CompositorImpl now has AddChildFrameSink() and RemoveChildFrameSink() to replace RegisterFrameSinkHierarchy() and UnregisterFrameSinkHierarchy() in DelegatedFrameHostAndroid. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. This CL takes us in the direction of splitting DirectCompositorFrameSink into two pieces: a client side component and a service side component. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:104: // RegisterFrameSinkId(). This needs to be far more explicit. How about this? A SurfaceSequence's validity is bound to the lifetime of the parent FrameSink that created it. We track the lifetime of FrameSink's through RegisterFrameSinkId and InvalidateFrameSink. During startup and GPU restart, a SurfaceSequence created by the top most layer compositor may be used prior to the creation of the associated CompositorFrameSinkSupport. CompositorFrameSinkSupport is created asynchronously when a new GPU channel is established. Once we switch to SurfaceReferences, this ordering concern goes away and we can remove this bool.
https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:104: // RegisterFrameSinkId(). This needs to be far more explicit. How about this? A SurfaceSequence's validity is bound to the lifetime of the parent FrameSink that created it. We track the lifetime of FrameSink's through RegisterFrameSinkId and InvalidateFrameSink. During startup and GPU restart, a SurfaceSequence created by the top most layer compositor may be used prior to the creation of the associated CompositorFrameSinkSupport. CompositorFrameSinkSupport is created asynchronously when a new GPU channel is established. Once we switch to SurfaceReferences, this ordering concern goes away and we can remove this bool.
https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:104: // RegisterFrameSinkId(). On 2017/02/07 17:33:55, Fady Samuel wrote: > This needs to be far more explicit. How about this? > > A SurfaceSequence's validity is bound to the lifetime of the parent FrameSink > that created it. We track the lifetime of FrameSink's through > RegisterFrameSinkId and InvalidateFrameSink. During startup and GPU restart, a > SurfaceSequence created by the top most layer compositor may be used prior to > the creation of the associated > CompositorFrameSinkSupport. CompositorFrameSinkSupport is created asynchronously > when a new GPU channel is established. Once we switch to SurfaceReferences, this > ordering concern goes away and we can remove this bool. Done.
Some nits. https://codereview.chromium.org/2612083002/diff/710001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:20: support_(this, surface_manager, frame_sink_id, display, true, true), Please specify what these trues mean: e.g. true /* handles_frame_sink_id_invalidation */, true /* needs_sync_points*/ https://codereview.chromium.org/2612083002/diff/710001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/components/exo/composi... components/exo/compositor_frame_sink.cc:21: : support_(this, surface_manager, frame_sink_id, nullptr, true, true), Please specify what these trues mean: e.g. true /* handles_frame_sink_id_invalidation */, true /* needs_sync_points*/ https://codereview.chromium.org/2612083002/diff/710001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:25: true, Please specify what these trues mean: e.g. true /* handles_frame_sink_id_invalidation */, true /* needs_sync_points*/
https://codereview.chromium.org/2612083002/diff/710001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:20: support_(this, surface_manager, frame_sink_id, display, true, true), On 2017/02/07 17:42:18, Fady Samuel wrote: > Please specify what these trues mean: e.g. true /* > handles_frame_sink_id_invalidation */, true /* needs_sync_points*/ Done. https://codereview.chromium.org/2612083002/diff/710001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/components/exo/composi... components/exo/compositor_frame_sink.cc:21: : support_(this, surface_manager, frame_sink_id, nullptr, true, true), On 2017/02/07 17:42:18, Fady Samuel wrote: > Please specify what these trues mean: e.g. true /* > handles_frame_sink_id_invalidation */, true /* needs_sync_points*/ Done. https://codereview.chromium.org/2612083002/diff/710001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:25: true, On 2017/02/07 17:42:18, Fady Samuel wrote: > Please specify what these trues mean: e.g. true /* > handles_frame_sink_id_invalidation */, true /* needs_sync_points*/ Done.
https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:83: support_->SetNeedsBeginFrame(false); It's unclear to me if you need this after changing CompositorFrameSinkSupport's destructor? https://codereview.chromium.org/2612083002/diff/710001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:20: support_(this, surface_manager, frame_sink_id, display, true, true), can you put a comment beside these boolean literals to say what they are? always document boolean literals as function arguments either with a temp var (not possible here) or a comment https://codereview.chromium.org/2612083002/diff/710001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/components/exo/composi... components/exo/compositor_frame_sink.cc:21: : support_(this, surface_manager, frame_sink_id, nullptr, true, true), same here https://codereview.chromium.org/2612083002/diff/710001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:25: true, and here
Patchset #37 (id:730001) has been deleted
https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:83: support_->SetNeedsBeginFrame(false); On 2017/02/07 17:47:55, danakj (slow) wrote: > It's unclear to me if you need this after changing CompositorFrameSinkSupport's > destructor? Done. https://codereview.chromium.org/2612083002/diff/710001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:20: support_(this, surface_manager, frame_sink_id, display, true, true), On 2017/02/07 17:47:55, danakj (slow) wrote: > can you put a comment beside these boolean literals to say what they are? always > document boolean literals as function arguments either with a temp var (not > possible here) or a comment Done. https://codereview.chromium.org/2612083002/diff/710001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/components/exo/composi... components/exo/compositor_frame_sink.cc:21: : support_(this, surface_manager, frame_sink_id, nullptr, true, true), On 2017/02/07 17:47:55, danakj (slow) wrote: > same here Done. https://codereview.chromium.org/2612083002/diff/710001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/710001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:25: true, On 2017/02/07 17:47:55, danakj (slow) wrote: > and here Done.
Patchset #37 (id:750001) has been deleted
The CQ bit was checked by fsamuel@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/2612083002/diff/770001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.h (right): https://codereview.chromium.org/2612083002/diff/770001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.h:83: std::unique_ptr<ExternalBeginFrameSource> begin_frame_source_; Can u order these pointers in the same order they are created/destroyed. No need for whitespace between them? https://codereview.chromium.org/2612083002/diff/770001/components/display_com... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/770001/components/display_com... components/display_compositor/gpu_display_compositor_frame_sink.cc:29: display_begin_frame_source_(std::move(begin_frame_source)) { This BFS is used by the display, not by this class. Before this CL it was owned in the same places as the Display. Can we maintain that?
https://codereview.chromium.org/2612083002/diff/770001/components/display_com... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/770001/components/display_com... components/display_compositor/gpu_display_compositor_frame_sink.cc:29: display_begin_frame_source_(std::move(begin_frame_source)) { On 2017/02/07 19:34:44, danakj (slow) wrote: > This BFS is used by the display, not by this class. Before this CL it was owned > in the same places as the Display. Can we maintain that? Oh the Display is owned here too I see.
ok I think this is right LGTM - CFSSupport has the service-side BFS. - GpuCFS and DirectCFS both have a support and listen to it for BFS notifications. - GpuCFS sends a mojo message to the client which gets forwarded there to the ExternalBFS - DirectCFS just has an ExternalBFS and forwards there. https://codereview.chromium.org/2612083002/diff/770001/components/display_com... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/770001/components/display_com... components/display_compositor/gpu_display_compositor_frame_sink.cc:40: support_.display()->SetVisible(visible); All of these can go thru display_-> now
https://codereview.chromium.org/2612083002/diff/770001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.h (right): https://codereview.chromium.org/2612083002/diff/770001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.h:83: std::unique_ptr<ExternalBeginFrameSource> begin_frame_source_; On 2017/02/07 19:34:44, danakj (slow) wrote: > Can u order these pointers in the same order they are created/destroyed. No need > for whitespace between them? Done. https://codereview.chromium.org/2612083002/diff/770001/components/display_com... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/770001/components/display_com... components/display_compositor/gpu_display_compositor_frame_sink.cc:29: display_begin_frame_source_(std::move(begin_frame_source)) { On 2017/02/07 19:36:07, danakj (slow) wrote: > On 2017/02/07 19:34:44, danakj (slow) wrote: > > This BFS is used by the display, not by this class. Before this CL it was > owned > > in the same places as the Display. Can we maintain that? > > Oh the Display is owned here too I see. Done.
Patchset #38 (id:790001) has been deleted
https://codereview.chromium.org/2612083002/diff/770001/components/display_com... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2612083002/diff/770001/components/display_com... components/display_compositor/gpu_display_compositor_frame_sink.cc:40: support_.display()->SetVisible(visible); On 2017/02/07 19:39:53, danakj (slow) wrote: > All of these can go thru display_-> now Done.
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, fsamuel@chromium.org, xlai@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2612083002/#ps830001 (title: "Addressed 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": 830001, "attempt_start_ts": 1486497624488990,
"parent_rev": "5a2beb655d42509edec55b7438bf6887a6454d0a", "commit_rev":
"c405416053b99aa4d6f98f3126608c660773f6b4"}
Message was sent while issue was closed.
Description was changed from ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. This CL takes us in the direction of splitting DirectCompositorFrameSink into two pieces: a client side component and a service side component. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. This CL takes us in the direction of splitting DirectCompositorFrameSink into two pieces: a client side component and a service side component. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2612083002 Cr-Commit-Position: refs/heads/master@{#448731} Committed: https://chromium.googlesource.com/chromium/src/+/c405416053b99aa4d6f98f312660... ==========
Message was sent while issue was closed.
Committed patchset #39 (id:830001) as https://chromium.googlesource.com/chromium/src/+/c405416053b99aa4d6f98f312660...
Message was sent while issue was closed.
A revert of this CL (patchset #39 id:830001) has been created in https://codereview.chromium.org/2682023004/ by jam@chromium.org. The reason for reverting is: Causing lots of flake with Android tests https://bugs.chromium.org/p/chromium/issues/detail?id=676070.
Message was sent while issue was closed.
A revert of this CL (patchset #39 id:830001) has been created in https://codereview.chromium.org/2682943004/ by jam@chromium.org. The reason for reverting is: Causing lots of flake with Android tests https://bugs.chromium.org/p/chromium/issues/detail?id=676070..
Message was sent while issue was closed.
A revert of this CL (patchset #39 id:830001) has been created in https://codereview.chromium.org/2681003005/ by jam@chromium.org. The reason for reverting is: Causing lots of flake with Android tests https://bugs.chromium.org/p/chromium/issues/detail?id=676070..
Message was sent while issue was closed.
Description was changed from ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. This CL takes us in the direction of splitting DirectCompositorFrameSink into two pieces: a client side component and a service side component. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2612083002 Cr-Commit-Position: refs/heads/master@{#448731} Committed: https://chromium.googlesource.com/chromium/src/+/c405416053b99aa4d6f98f312660... ========== to ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. This CL takes us in the direction of splitting DirectCompositorFrameSink into two pieces: a client side component and a service side component. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2612083002 Cr-Commit-Position: refs/heads/master@{#448731} Committed: https://chromium.googlesource.com/chromium/src/+/c405416053b99aa4d6f98f312660... ==========
Patchset #38 (id:810001) has been deleted
The CQ bit was checked by fsamuel@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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by fsamuel@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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
I think you'll likely want to add CC_SURFACES_EXPORT to CompositorFrameSinkSupportClient.
On Thu, Feb 16, 2017 at 6:05 AM, <fsamuel@chromium.org> wrote: > I think you'll likely want to add CC_SURFACES_EXPORT to > CompositorFrameSinkSupportClient. > Or NON_EXPORTED_BASE around the use of it as a parent class like DisplayClient. > > https://codereview.chromium.org/2612083002/ > -- 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.
Yea, NON_EXPORTED_BASE is probably preferable. On 2017/02/16 15:35:55, danakj wrote: > On Thu, Feb 16, 2017 at 6:05 AM, <mailto:fsamuel@chromium.org> wrote: > > > I think you'll likely want to add CC_SURFACES_EXPORT to > > > CompositorFrameSinkSupportClient. > > > > Or NON_EXPORTED_BASE around the use of it as a parent class like > DisplayClient. > > > > > > https://codereview.chromium.org/2612083002/ > > > > -- > 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.
The CQ bit was checked by fsamuel@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 staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, danakj@chromium.org, fsamuel@chromium.org, xlai@chromium.org Link to the patchset: https://codereview.chromium.org/2612083002/#ps870001 (title: "Added NON_EXPORTED_BASE")
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": 870001, "attempt_start_ts": 1487425676403100,
"parent_rev": "3d875dfaa149b616ef07c753b4ad55777b636c7e", "commit_rev":
"9d4e3df6e621c6bd80dfaee7008b3e33d0c4fe79"}
Message was sent while issue was closed.
Description was changed from ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. This CL takes us in the direction of splitting DirectCompositorFrameSink into two pieces: a client side component and a service side component. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2612083002 Cr-Commit-Position: refs/heads/master@{#448731} Committed: https://chromium.googlesource.com/chromium/src/+/c405416053b99aa4d6f98f312660... ========== to ========== DirectCompositorFrameSink Uses CompositorFrameSinkSupport CompositorFrameSinkSupport is the common service-side implementation of CompositorFrameSink. In an effort to evaluate mojo performance independently of refactoring existing code, CompositorFrameSinkSupport provides the same public interface as MojoCompositorFrameSink but does not depend on mojo. This CL takes us in the direction of splitting DirectCompositorFrameSink into two pieces: a client side component and a service side component. BUG=673543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2612083002 Cr-Original-Commit-Position: refs/heads/master@{#448731} Committed: https://chromium.googlesource.com/chromium/src/+/c405416053b99aa4d6f98f312660... Review-Url: https://codereview.chromium.org/2612083002 Cr-Commit-Position: refs/heads/master@{#451462} Committed: https://chromium.googlesource.com/chromium/src/+/9d4e3df6e621c6bd80dfaee7008b... ==========
Message was sent while issue was closed.
Committed patchset #40 (id:870001) as https://chromium.googlesource.com/chromium/src/+/9d4e3df6e621c6bd80dfaee7008b...
Message was sent while issue was closed.
A revert of this CL (patchset #40 id:870001) has been created in https://codereview.chromium.org/2719073002/ by staraz@chromium.org. The reason for reverting is: This CL broke several perf tests on Windows. (https://bugs.chromium.org/p/chromium/issues/detail?id=696030). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
