Move GpuMemoryBufferManager and SharedBitmapManager to CompositorFrameSink

Move GpuMemoryBufferManager and SharedBitmapManager to CompositorFrameSink

They're only used by the ResourceProvider, so there's no fundamental reason for
them to take a separate path through the compositor. Besides, it is natural that
the CompositorFrameSink contains all the objects needed to create resources, all
at the same place. Finally, it's less code overall.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/c44437a22b4b75aa2b780ddb8f8adb562844d994
Cr-Commit-Position: refs/heads/master@{#428551}

[piman, 2016-10-25 01:43:24]

Description was changed from

==========
Move GpuMemoryBufferManager and SharedBitmapManager to CompositorFrameSink

They're only used by the ResourceProvider, so there's no fundamental reason for
them to take a separate path through the compositor. Besides, it is natural that
the CompositorFrameSink contains all the objects needed to create resources, all
at the same place. Finally, it's less code overall.

BUG=None
==========

to

==========
Move GpuMemoryBufferManager and SharedBitmapManager to CompositorFrameSink

They're only used by the ResourceProvider, so there's no fundamental reason for
them to take a separate path through the compositor. Besides, it is natural that
the CompositorFrameSink contains all the objects needed to create resources, all
at the same place. Finally, it's less code overall.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[piman, 2016-10-25 01:43:36]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 01:44:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 01:55:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 01:55:40]

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...)

[piman, 2016-10-25 23:34:40]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 23:34:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 23:54:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 23:54:58]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[piman, 2016-10-26 00:12:54]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 00:13:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 00:33:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 00:33:19]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[piman, 2016-10-26 00:50:42]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 00:51:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 04:06:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 04:06:59]

Dry run: This issue passed the CQ dry run.

[piman, 2016-10-26 23:18:45]

Patchset #3 (id:40001) has been deleted

[piman, 2016-10-26 23:18:53]

Patchset #2 (id:20001) has been deleted

[piman, 2016-10-26 23:18:58]

Patchset #1 (id:1) has been deleted

[piman, 2016-10-26 23:29:24]

piman@chromium.org changed reviewers:
+ danakj@chromium.org, kbr@chromium.org, khushalsagar@chromium.org,
sky@chromium.org

[piman, 2016-10-26 23:29:25]

danakj: please review
sky: OWNER for services/ui/ and ui/views/mus/
khushalsagar: OWNER for blimp/
kbr: OWNER for
third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp

Sorry for long patch, but most of it is unit tests (mostly deletions).

[Ken Russell (switch to Gerrit), 2016-10-26 23:35:25]

CompositorMutableStateTest.cpp lgtm

[Khushal, 2016-10-26 23:45:51]

lgtm

[sky, 2016-10-27 15:31:50]

LGTM

[danakj, 2016-10-27 21:00:35]

https://codereview.chromium.org/2452483002/diff/60001/cc/layers/layer_perftes...
File cc/layers/layer_perftest.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/cc/layers/layer_perftes...
cc/layers/layer_perftest.cc:48: TestSharedBitmapManager shared_bitmap_manager_;
Not used?

https://codereview.chromium.org/2452483002/diff/60001/cc/output/compositor_fr...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2452483002/diff/60001/cc/output/compositor_fr...
cc/output/compositor_frame_sink.h:66: // shared_bitmap_manager is optional
(won't be used) if context_provider is
Maybe we should have 2 constructors at this point? This kinda logic is :/ (It's
fine for this CL)

https://codereview.chromium.org/2452483002/diff/60001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:2029: shared_bitmap_manager(), this);
why not null shared bitmap manager for this and below?

https://codereview.chromium.org/2452483002/diff/60001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/content/test/layouttest...
content/test/layouttest_support.cc:333: cc::SharedBitmapManager*
shared_bitmap_manager,
layout tests are never software, so no need for SBM?

https://codereview.chromium.org/2452483002/diff/60001/ui/compositor/compositor.h
File ui/compositor/compositor.h (left):

https://codereview.chromium.org/2452483002/diff/60001/ui/compositor/composito...
ui/compositor/compositor.h:133: virtual gpu::GpuMemoryBufferManager*
GetGpuMemoryBufferManager() = 0;
Is this used?

https://codereview.chromium.org/2452483002/diff/60001/ui/compositor/test/in_p...
File ui/compositor/test/in_process_context_factory.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/ui/compositor/test/in_p...
ui/compositor/test/in_process_context_factory.cc:193: &shared_bitmap_manager_,
GetGpuMemoryBufferManager(),
why the asymetry here with SBM and GMBM?

https://codereview.chromium.org/2452483002/diff/60001/ui/views/mus/surface_co...
File ui/views/mus/surface_context_factory.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/ui/views/mus/surface_co...
ui/views/mus/surface_context_factory.cc:46: GetGpuMemoryBufferManager());
this could go to gpu_service_ right, do we need this in the API?

[piman, 2016-10-28 00:36:55]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 00:37:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-10-28 00:42:37]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 00:43:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-10-28 00:58:37]

https://codereview.chromium.org/2452483002/diff/60001/cc/layers/layer_perftes...
File cc/layers/layer_perftest.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/cc/layers/layer_perftes...
cc/layers/layer_perftest.cc:48: TestSharedBitmapManager shared_bitmap_manager_;
On 2016/10/27 21:00:34, danakj wrote:
> Not used?

Removed.

https://codereview.chromium.org/2452483002/diff/60001/cc/output/compositor_fr...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2452483002/diff/60001/cc/output/compositor_fr...
cc/output/compositor_frame_sink.h:66: // shared_bitmap_manager is optional
(won't be used) if context_provider is
On 2016/10/27 21:00:34, danakj wrote:
> Maybe we should have 2 constructors at this point? This kinda logic is :/
(It's
> fine for this CL)

So, that's what we used to have in the past iirc. The reason to coalesce them
was that having separate constructors here means separate/duplicated
constructors in derived classes (browser ones in particular) which was even more
:/

I suppose an alternative could be to make (pure) virtual accessors for each of
the fields, so that we're not constrained by the constructors, but I haven't
looked in detail at if it would really help or make things worse.
Another option is to pack all 4 of these (or 5 with Vulkan) into a separate
struct with 2 (3) constructors, so that the split happens at the construction
site?
Dunno if it's worth doing though.

https://codereview.chromium.org/2452483002/diff/60001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:2029: shared_bitmap_manager(), this);
On 2016/10/27 21:00:34, danakj wrote:
> why not null shared bitmap manager for this and below?

Fair enough, changed (or removed parameter altogether as appropriate). I
originally just meant to keep the same things flowing through, but you're right
that this leads to further simplifications, so it's better.

https://codereview.chromium.org/2452483002/diff/60001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/content/test/layouttest...
content/test/layouttest_support.cc:333: cc::SharedBitmapManager*
shared_bitmap_manager,
On 2016/10/27 21:00:34, danakj wrote:
> layout tests are never software, so no need for SBM?

Done.

https://codereview.chromium.org/2452483002/diff/60001/ui/compositor/compositor.h
File ui/compositor/compositor.h (left):

https://codereview.chromium.org/2452483002/diff/60001/ui/compositor/composito...
ui/compositor/compositor.h:133: virtual gpu::GpuMemoryBufferManager*
GetGpuMemoryBufferManager() = 0;
On 2016/10/27 21:00:34, danakj wrote:
> Is this used?

Unfortunately yes, by exo. I was originally hoping we could remove, but found it
the hard way. The way it's hooked up (using this interface) is not ideal, but I
didn't find an easy way to replace. I'll think about it more though.

https://codereview.chromium.org/2452483002/diff/60001/ui/compositor/test/in_p...
File ui/compositor/test/in_process_context_factory.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/ui/compositor/test/in_p...
ui/compositor/test/in_process_context_factory.cc:193: &shared_bitmap_manager_,
GetGpuMemoryBufferManager(),
On 2016/10/27 21:00:34, danakj wrote:
> why the asymetry here with SBM and GMBM?

Meh, mostly because GetGpuMemoryManager could become slightly more complex in
the future (e.g. as  in GPTF), but I really don't feel strongly, so changed to
&gpu_memory_buffer_manager_

https://codereview.chromium.org/2452483002/diff/60001/ui/views/mus/surface_co...
File ui/views/mus/surface_context_factory.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/ui/views/mus/surface_co...
ui/views/mus/surface_context_factory.cc:46: GetGpuMemoryBufferManager());
On 2016/10/27 21:00:34, danakj wrote:
> this could go to gpu_service_ right, do we need this in the API?

I changed to gpu_service_->gpu_memory_buffer_manager() here (is that what you
were looking for?). GetGpuMemoryBufferManager needs to exist because of exo (see
above)

[commit-bot: I haz the power, 2016-10-28 01:00:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 01:00:31]

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...)

[piman, 2016-10-28 01:13:08]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 01:13:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-10-28 01:33:20]

LGTM

https://codereview.chromium.org/2452483002/diff/60001/ui/views/mus/surface_co...
File ui/views/mus/surface_context_factory.cc (right):

https://codereview.chromium.org/2452483002/diff/60001/ui/views/mus/surface_co...
ui/views/mus/surface_context_factory.cc:46: GetGpuMemoryBufferManager());
On 2016/10/28 00:58:36, piman wrote:
> On 2016/10/27 21:00:34, danakj wrote:
> > this could go to gpu_service_ right, do we need this in the API?
> 
> I changed to gpu_service_->gpu_memory_buffer_manager() here (is that what you
> were looking for?). GetGpuMemoryBufferManager needs to exist because of exo
(see
> above)

Was hoping the method could go away entirely but thanks for the explanation.

https://codereview.chromium.org/2452483002/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2452483002/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_frame_sink.cc:181: nullptr /*
shared_bitmap_manager */, gpu_memory_buffer_manager(),
In this one they were both null before because its for resourceless software
draw. If anything it'd need SBM but actually neither is needed. (There was a bit
comment about this, maybe it'd make sense to leave this both null and put that
back.)

https://codereview.chromium.org/2452483002/diff/80001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2452483002/diff/80001/content/test/layouttest...
content/test/layouttest_support.cc:350: std::move(worker_context_provider),
nullptr,
nit: /* shared_bitmap_manager */

[commit-bot: I haz the power, 2016-10-28 02:22:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 02:22:13]

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_...)

[piman, 2016-10-28 22:06:42]

The CQ bit was checked by piman@chromium.org

[piman, 2016-10-28 22:06:43]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
sky@chromium.org, khushalsagar@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2452483002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-28 22:07:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-10-28 22:08:34]

https://codereview.chromium.org/2452483002/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2452483002/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_frame_sink.cc:181: nullptr /*
shared_bitmap_manager */, gpu_memory_buffer_manager(),
On 2016/10/28 01:33:20, danakj wrote:
> In this one they were both null before because its for resourceless software
> draw. If anything it'd need SBM but actually neither is needed. (There was a
bit
> comment about this, maybe it'd make sense to leave this both null and put that
> back.)

Done.

https://codereview.chromium.org/2452483002/diff/80001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2452483002/diff/80001/content/test/layouttest...
content/test/layouttest_support.cc:350: std::move(worker_context_provider),
nullptr,
On 2016/10/28 01:33:20, danakj wrote:
> nit: /* shared_bitmap_manager */

Done.

[commit-bot: I haz the power, 2016-10-29 00:09:53]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-10-29 00:44:25]

Description was changed from

==========
Move GpuMemoryBufferManager and SharedBitmapManager to CompositorFrameSink

They're only used by the ResourceProvider, so there's no fundamental reason for
them to take a separate path through the compositor. Besides, it is natural that
the CompositorFrameSink contains all the objects needed to create resources, all
at the same place. Finally, it's less code overall.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Move GpuMemoryBufferManager and SharedBitmapManager to CompositorFrameSink

They're only used by the ResourceProvider, so there's no fundamental reason for
them to take a separate path through the compositor. Besides, it is natural that
the CompositorFrameSink contains all the objects needed to create resources, all
at the same place. Finally, it's less code overall.

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/c44437a22b4b75aa2b780ddb8f8adb562844d994
Cr-Commit-Position: refs/heads/master@{#428551}
==========

[commit-bot: I haz the power, 2016-10-29 00:44:27]

Patchset 6 (id:??) landed as
https://crrev.com/c44437a22b4b75aa2b780ddb8f8adb562844d994
Cr-Commit-Position: refs/heads/master@{#428551}