New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 707105 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Merge SurfaceFactory into CompositorFrameSinkSupport

Project Member Reported by fsam...@chromium.org, Mar 31 2017

Issue description

SurfaceFactory is no longer really a factory of surfaces because SurfaceManager manages the lifetime of surfaces.

Now that we've replaced all usage of SurfaceFactory with CompositorFrameSinkSupport, we should move all functionality in SurfaceFactory out to CompositorFrameSinkSupport and delete SurfaceFactory at this point.
 
As a first step, we probably want to port all tests over to CompositorFrameSinkSupport. FakeSurfaceFactoryClient should change to be a CompositorFrameSinkSupport I suppose.
surface_unittest.cc uses a SurfaceFactory with a FakeSurfaceFactoryClient. The test requires SurfaceFactory::SubmitCompositorFrame and EvictFrame. I think we need a FakeCompositorFrameSinkSupportClient instead of a FakeCompositorFrameSinkSupport.

I'm working on surface_aggregator_unittest now, which is similar. I'm thinking to put FakeCompositorFrameSinkSupportClient into its own files or some test util files so that we don't have to write one for each test that's using it.
Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86ccbc2d91a472b49db3dc48ba9c586eaa1995d5

commit 86ccbc2d91a472b49db3dc48ba9c586eaa1995d5
Author: staraz <staraz@chromium.org>
Date: Tue Apr 11 00:13:35 2017

Remove SurfaceFactory Occurrences Outside Of cc

SurfaceFactory and SurfaceFactoryClient is being removed.
This CL removes SurfaceFactory usages outside of cc.

BUG= 707105 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2800833005
Cr-Commit-Position: refs/heads/master@{#463460}

[modify] https://crrev.com/86ccbc2d91a472b49db3dc48ba9c586eaa1995d5/cc/surfaces/compositor_frame_sink_support.cc
[modify] https://crrev.com/86ccbc2d91a472b49db3dc48ba9c586eaa1995d5/cc/surfaces/compositor_frame_sink_support.h
[modify] https://crrev.com/86ccbc2d91a472b49db3dc48ba9c586eaa1995d5/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/86ccbc2d91a472b49db3dc48ba9c586eaa1995d5/content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
[modify] https://crrev.com/86ccbc2d91a472b49db3dc48ba9c586eaa1995d5/content/browser/frame_host/render_widget_host_view_guest_unittest.cc
[modify] https://crrev.com/86ccbc2d91a472b49db3dc48ba9c586eaa1995d5/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/86ccbc2d91a472b49db3dc48ba9c586eaa1995d5/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/86ccbc2d91a472b49db3dc48ba9c586eaa1995d5/ui/compositor/compositor_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/962f6109508a91c8110ac855d018d334b274913e

commit 962f6109508a91c8110ac855d018d334b274913e
Author: staraz <staraz@chromium.org>
Date: Wed Apr 12 00:20:06 2017

Move Work From CompositorFrameSinkSupport() To Init()

SurfaceManager::RegisterSurfaceFactoryClient calls
SurfaceFactoryClient::SetBeginFrameSource() which is a virtual method. Calling
virtual methods on an object during its construction in the constructor is against the
style guide.

Call to RegisterFrameSinkId and RegisterSurfaceFactoryClient is moved into
Init(). Init() is called right after constructing CompositorFrameSinkSupport
everywhere.

BUG= 707105 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2807653003
Cr-Commit-Position: refs/heads/master@{#463853}

[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/ash/laser/laser_pointer_view.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/ash/laser/laser_pointer_view.h
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/cc/surfaces/compositor_frame_sink_support.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/cc/surfaces/compositor_frame_sink_support.h
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/cc/surfaces/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/cc/surfaces/direct_compositor_frame_sink.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/cc/test/test_compositor_frame_sink.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/components/exo/compositor_frame_sink.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/components/exo/compositor_frame_sink.h
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/components/viz/frame_sinks/gpu_compositor_frame_sink.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/content/renderer/android/synchronous_compositor_frame_sink.cc
[modify] https://crrev.com/962f6109508a91c8110ac855d018d334b274913e/ui/android/delegated_frame_host_android.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/04278218c9ef29904b209f077fd3ee5869898ba2

commit 04278218c9ef29904b209f077fd3ee5869898ba2
Author: staraz <staraz@chromium.org>
Date: Wed Apr 12 23:07:19 2017

Replace use of SurfaceFactory with CompositorFrameSinkSupport in cc tests

SurfaceFactory is being deleted since all non-test usages have been replaced by
CompositorFrameSinkSupport. This CL replaces usages in all but SurfaceAggregator
tests.

BUG= 707105 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2795683003
Cr-Commit-Position: refs/heads/master@{#464194}

[modify] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/BUILD.gn
[modify] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/surfaces/compositor_frame_sink_support.cc
[modify] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/surfaces/display_unittest.cc
[modify] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/surfaces/surface_aggregator_unittest.cc
[modify] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/surfaces/surface_hittest_unittest.cc
[modify] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/surfaces/surface_manager_ref_unittest.cc
[modify] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/surfaces/surface_unittest.cc
[modify] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/surfaces/surfaces_pixeltest.cc
[add] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/test/fake_compositor_frame_sink_support_client.cc
[add] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/test/fake_compositor_frame_sink_support_client.h
[modify] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/cc/test/surface_hittest_test_helpers.h

Labels: Type-Feature
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0838e367b020c960036c1e706b40f5bfd3226e32

commit 0838e367b020c960036c1e706b40f5bfd3226e32
Author: staraz <staraz@chromium.org>
Date: Fri Apr 21 12:39:12 2017

Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

As discussed in another CL (https://codereview.chromium.org/2824053003/),
WillDrawSurface is noisier than it needs to be. It is called on every draw
regardless of damage and reports damage in the submitted frame that the client
already knows about while the client is only interested in damage to child
surfaces.

By replacing WillDrawSurface with a base::RepeatingCallback, SurfaceFactory
doesn't need to know about the details and we avoid having to go through the
factory on the way back.

BUG= 707105 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2834553002
Cr-Commit-Position: refs/heads/master@{#466318}

[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/cc/surfaces/compositor_frame_sink_support.cc
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/cc/surfaces/display.cc
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/cc/surfaces/surface.cc
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/cc/surfaces/surface.h
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/cc/surfaces/surface_aggregator.cc
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/cc/surfaces/surface_aggregator_perftest.cc
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/cc/surfaces/surface_factory.cc
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/cc/surfaces/surface_factory.h
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/cc/surfaces/surface_factory_unittest.cc
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/0838e367b020c960036c1e706b40f5bfd3226e32/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dcf699d9ec671447372b367a375a622271396db5

commit dcf699d9ec671447372b367a375a622271396db5
Author: staraz <staraz@chromium.org>
Date: Fri Apr 21 22:38:55 2017

Split SurfaceFactoryClient Into Four Interfaces

SurfaceFactoryClient has four methods and each one of them is being used by
a different class: ReferencedSurfacesChanged is used by SurfaceFactory;
ReturnResources is used by SurfaceResourceHolder; WillDrawSurface is called by
SurfaceFactory but SurfaceFactory is simply forwarding the call from
SurfaceAggregator; SetBeginFrameSource is used by FrameSinkManager.

Giving the four classes their own client types allows the implementation class
to be more flexible. The implementation class would no longer have to implement
all four methods when only some of them are needed.

SurfaceAggregator::PreWalkTree() no longer goes through SurfaceFactory
when calling WillDrawSurface.

BUG= 707105 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2824053003
Cr-Commit-Position: refs/heads/master@{#466473}

[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/BUILD.gn
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/BUILD.gn
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/compositor_frame_sink_support.cc
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/compositor_frame_sink_support.h
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/framesink_manager.cc
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/framesink_manager.h
[add] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/framesink_manager_client.h
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_aggregator_perftest.cc
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_factory.cc
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_factory.h
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_factory_client.h
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_factory_unittest.cc
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_manager.cc
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_manager.h
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_manager_unittest.cc
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_resource_holder.cc
[modify] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_resource_holder.h
[add] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/surfaces/surface_resource_holder_client.h
[add] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/test/fake_surface_resource_holder_client.cc
[add] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/test/fake_surface_resource_holder_client.h
[add] https://crrev.com/dcf699d9ec671447372b367a375a622271396db5/cc/test/stub_surface_factory_client.h

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75e518861ddbdf82531605d1bdf4d678e8d4ffe6

commit 75e518861ddbdf82531605d1bdf4d678e8d4ffe6
Author: xing.xu <xing.xu@intel.com>
Date: Mon Apr 24 01:14:05 2017

Remove SurfaceFactory in SurfaceAggregatorPerfTest

Also change the size of rect and visible_rect to fix DrawQuad::SetAll checks.

BUG= 707105 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2830203002
Cr-Commit-Position: refs/heads/master@{#466574}

[modify] https://crrev.com/75e518861ddbdf82531605d1bdf4d678e8d4ffe6/cc/surfaces/surface_aggregator_perftest.cc

Cc: weiliangc@chromium.org
Project Member

Comment 12 by bugdroid1@chromium.org, May 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1

commit c0f2d73db929c46c1dd9fc14e627a0f7d39469c1
Author: staraz <staraz@chromium.org>
Date: Mon May 08 10:47:16 2017

Remove SurfaceFactory And SurfaceFactoryClient

Now that all usages of SurfaceFacotry are replaced with
CompositorFrameSinkSuppot, we can move all functionality in SurfaceFactory to
CompositorFrameSinkSupport and delete the class.

BUG= 707105 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2802023002
Cr-Commit-Position: refs/heads/master@{#469954}

[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/BUILD.gn
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/BUILD.gn
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/compositor_frame_sink_support.cc
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/compositor_frame_sink_support.h
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/compositor_frame_sink_support_unittest.cc
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/direct_compositor_frame_sink.h
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/frame_sink_manager.cc
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/frame_sink_manager.h
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/surface.cc
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/surface.h
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/surface_aggregator.cc
[delete] https://crrev.com/8c70962f2be02fc57fc5bea12c7f7052605709ce/cc/surfaces/surface_factory.cc
[delete] https://crrev.com/8c70962f2be02fc57fc5bea12c7f7052605709ce/cc/surfaces/surface_factory.h
[delete] https://crrev.com/8c70962f2be02fc57fc5bea12c7f7052605709ce/cc/surfaces/surface_factory_client.h
[delete] https://crrev.com/8c70962f2be02fc57fc5bea12c7f7052605709ce/cc/surfaces/surface_factory_unittest.cc
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/surface_manager.cc
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/surface_manager.h
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/surface_manager_unittest.cc
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/surface_resource_holder.cc
[add] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/surfaces/surface_synchronization_unittest.cc
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/test/compositor_frame_helpers.cc
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/test/compositor_frame_helpers.h
[add] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/test/mock_compositor_frame_sink_support_client.cc
[add] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/cc/test/mock_compositor_frame_sink_support_client.h
[modify] https://crrev.com/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1/content/browser/frame_host/render_widget_host_view_guest.cc

Status: Fixed (was: Started)
Blocking: -601863
Components: -Internals>MUS Internals>Services>WindowService

Sign in to add a comment