Remove SurfaceFactory And SurfaceFactoryClient

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}
Committed: https://chromium.googlesource.com/chromium/src/+/c0f2d73db929c46c1dd9fc14e627a0f7d39469c1

[Alex Z., 2017-04-06 13:19:53]

Description was changed from

==========
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
==========

to

==========
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.chromium.linux:linux_site_isolation
==========

[Alex Z., 2017-04-06 13:19:54]

Description was changed from

==========
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.chromium.linux:linux_site_isolation
==========

to

==========
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
==========

[Alex Z., 2017-04-06 15:12:29]

Description was changed from

==========
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
==========

to

==========
Remove SurfaceFactory And SurfaceFactoryClient

NOTE: This is currently blocked by Issue 2795683003.

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
==========

[Alex Z., 2017-04-06 21:42:21]

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

[commit-bot: I haz the power, 2017-04-06 21:43:41]

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

[Alex Z., 2017-04-06 21:44:27]

staraz@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Alex Z., 2017-04-06 21:44:28]

This CL depends on my other CL that removes SurfaceFactory 
from tests.

The two CLs together eliminate all occurrences of 
SurfaceFactory and SurfaceFactoryClient.

PTAL.

[Fady Samuel, 2017-04-06 21:53:23]

not lgtm. We seem to be losing surface_factory_unittest? Maybe we can move those
tests to compositor_frame_sink_support_unittest.cc first?

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/compositor_...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support.h:58: void ReceiveFromChild(const
TransferableResourceArray& resources);
Make these private?

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/framesink_m...
File cc/surfaces/framesink_manager.cc (right):

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.cc:31: // All compositor frame supports should be
unregistered prior to
All CompositorFrameSinks should be unregistered...

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.cc:46: void
FrameSinkManager::RegisterCompositorFrameSinkSupport(
How about just RegisterCompositorFrameSink?

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.cc:61: void
FrameSinkManager::UnregisterCompositorFrameSinkSupport(
How about just UnregisterCompositorFrameSink?

[commit-bot: I haz the power, 2017-04-06 22:00:47]

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

[commit-bot: I haz the power, 2017-04-06 22:00:48]

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...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Alex Z., 2017-04-13 17:15:30]

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/compositor_...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support.h:58: void ReceiveFromChild(const
TransferableResourceArray& resources);
On 2017/04/06 21:53:23, Fady Samuel wrote:
> Make these private?

I'm leaving them public following our discussion on Slack.

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/framesink_m...
File cc/surfaces/framesink_manager.cc (right):

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.cc:31: // All compositor frame supports should be
unregistered prior to
On 2017/04/06 21:53:23, Fady Samuel wrote:
> All CompositorFrameSinks should be unregistered...

Done.

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.cc:46: void
FrameSinkManager::RegisterCompositorFrameSinkSupport(
On 2017/04/06 21:53:23, Fady Samuel wrote:
> How about just RegisterCompositorFrameSink?

Done.

https://codereview.chromium.org/2802023002/diff/40001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.cc:61: void
FrameSinkManager::UnregisterCompositorFrameSinkSupport(
On 2017/04/06 21:53:23, Fady Samuel wrote:
> How about just UnregisterCompositorFrameSink?

Done.

[Alex Z., 2017-04-13 21:54:22]

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

[commit-bot: I haz the power, 2017-04-13 21:55:15]

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, 2017-04-13 22:24:40]

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

[commit-bot: I haz the power, 2017-04-13 22:24:41]

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

[Alex Z., 2017-04-18 12:54:20]

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

[commit-bot: I haz the power, 2017-04-18 12:54:39]

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, 2017-04-18 13:31:11]

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

[commit-bot: I haz the power, 2017-04-18 13:31:12]

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

[Alex Z., 2017-04-25 21:29:21]

I have rebased and moved SurfaceFactoryTest to 
compositor_frame_sink_support_unittest.cc. PTAL

[Alex Z., 2017-04-25 21:54:59]

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

[commit-bot: I haz the power, 2017-04-25 21:55:32]

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, 2017-04-25 22:38:53]

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

[commit-bot: I haz the power, 2017-04-25 22:38:54]

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

[Fady Samuel, 2017-04-26 00:44:14]

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.cc (left):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:216: void
CompositorFrameSinkSupport::ForceReclaimResources() {
This should be going away now.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:57: 
unnecessary change. Please delete.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:109: TRACE_EVENT0("cc",
"CompositorFrameSinkSupport::SubmitCompositorFrame");
Why is this trace event listed twice?

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:256: void
CompositorFrameSinkSupport::ReceiveFromChild(
It would be nice not having these public..

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:271: void
CompositorFrameSinkSupport::ForceAck() {
What uses this? Delete it perhaps?

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (left):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:7: #include
"base/macros.h"
Wow, this file has become massive. How about two files?

surface_factory_unittest => compositor_frame_sink_support_unittest

surface sync related unit tests in here => surface_synchronization_unittest

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1075:
TEST_F(SurfaceReferenceTest, DisplayCompositorLockingBlockedOnTwo) {
Please rename "SurfaceReferenceTest" to SurfaceSynchronizationTest everywhere.
Please also delete all the DisplayCompositorLocking prefixes. They are
redundant.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/surface_re...
File cc/surfaces/surface_resource_holder.cc (right):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/surface_re...
cc/surfaces/surface_resource_holder.cc:24: for
(TransferableResourceArray::const_iterator it = resources.begin();
for (const auto& resource : resources) {
  ...
}

[Alex Z., 2017-04-27 22:09:50]

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

[Alex Z., 2017-04-27 22:10:01]

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.cc (left):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:216: void
CompositorFrameSinkSupport::ForceReclaimResources() {
On 2017/04/26 00:44:13, Fady Samuel wrote:
> This should be going away now.

Done.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:57: 
On 2017/04/26 00:44:13, Fady Samuel wrote:
> unnecessary change. Please delete.

Done.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:109: TRACE_EVENT0("cc",
"CompositorFrameSinkSupport::SubmitCompositorFrame");
On 2017/04/26 00:44:13, Fady Samuel wrote:
> Why is this trace event listed twice?

It must've got during rebase. It's removed now.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:256: void
CompositorFrameSinkSupport::ReceiveFromChild(
On 2017/04/26 00:44:13, Fady Samuel wrote:
> It would be nice not having these public..

Done.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:271: void
CompositorFrameSinkSupport::ForceAck() {
On 2017/04/26 00:44:13, Fady Samuel wrote:
> What uses this? Delete it perhaps?

Done.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (left):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:7: #include
"base/macros.h"
On 2017/04/26 00:44:14, Fady Samuel wrote:
> Wow, this file has become massive. How about two files?
> 
> surface_factory_unittest => compositor_frame_sink_support_unittest
> 
> surface sync related unit tests in here => surface_synchronization_unittest

Done.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:1075:
TEST_F(SurfaceReferenceTest, DisplayCompositorLockingBlockedOnTwo) {
On 2017/04/26 00:44:13, Fady Samuel wrote:
> Please rename "SurfaceReferenceTest" to SurfaceSynchronizationTest everywhere.
> Please also delete all the DisplayCompositorLocking prefixes. They are
> redundant.

Done.

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/surface_re...
File cc/surfaces/surface_resource_holder.cc (right):

https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/surface_re...
cc/surfaces/surface_resource_holder.cc:24: for
(TransferableResourceArray::const_iterator it = resources.begin();
On 2017/04/26 00:44:14, Fady Samuel wrote:
> for (const auto& resource : resources) {
>   ...
> }

const auto& doesn't work since we have ++it. I changed it to 
auto.

[commit-bot: I haz the power, 2017-04-27 22:29:49]

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, 2017-04-28 01:36:13]

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

[commit-bot: I haz the power, 2017-04-28 01:36:14]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Alex Z., 2017-04-28 12:28:59]

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

[commit-bot: I haz the power, 2017-04-28 12:29:16]

Dry run: CQ is trying da patch.

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

[Fady Samuel, 2017-04-28 12:30:30]

https://codereview.chromium.org/2802023002/diff/180001/cc/surfaces/surface_re...
File cc/surfaces/surface_resource_holder.cc (right):

https://codereview.chromium.org/2802023002/diff/180001/cc/surfaces/surface_re...
cc/surfaces/surface_resource_holder.cc:13:
SurfaceResourceHolder::~SurfaceResourceHolder() {}
SurfaceResourceHolder::~SurfaceResourceHolder() = default;

https://codereview.chromium.org/2802023002/diff/180001/cc/surfaces/surface_re...
cc/surfaces/surface_resource_holder.cc:24: for (auto it = resources.begin(); it
!= resources.end(); ++it) {
This should work.

for (auto& resource : resources) {
  .. 
}

[Alex Z., 2017-04-28 12:40:22]

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

[Alex Z., 2017-04-28 12:40:33]

https://codereview.chromium.org/2802023002/diff/180001/cc/surfaces/surface_re...
File cc/surfaces/surface_resource_holder.cc (right):

https://codereview.chromium.org/2802023002/diff/180001/cc/surfaces/surface_re...
cc/surfaces/surface_resource_holder.cc:13:
SurfaceResourceHolder::~SurfaceResourceHolder() {}
On 2017/04/28 12:30:30, Fady Samuel wrote:
> SurfaceResourceHolder::~SurfaceResourceHolder() = default;

Done.

https://codereview.chromium.org/2802023002/diff/180001/cc/surfaces/surface_re...
cc/surfaces/surface_resource_holder.cc:24: for (auto it = resources.begin(); it
!= resources.end(); ++it) {
On 2017/04/28 12:30:30, Fady Samuel wrote:
> This should work.
> 
> for (auto& resource : resources) {
>   .. 
> }

That can't be done either. "Non-const lvalue cannot bind to a 
temporary."

[commit-bot: I haz the power, 2017-04-28 12:40:41]

Dry run: CQ is trying da patch.

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

[Alex Z., 2017-04-28 12:41:21]

Patchset #12 (id:220001) has been deleted

[Alex Z., 2017-04-28 12:42:13]

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

[commit-bot: I haz the power, 2017-04-28 12:42:25]

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, 2017-04-28 13:51:43]

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

[commit-bot: I haz the power, 2017-04-28 13:51:44]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2017-04-28 14:17:10]

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

[commit-bot: I haz the power, 2017-04-28 14:17:32]

Dry run: CQ is trying da patch.

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

[Fady Samuel, 2017-04-28 14:19:56]

lgtm

[Alex Z., 2017-04-28 14:26:31]

staraz@chromium.org changed reviewers:
+ piman@chromium.org

[Alex Z., 2017-04-28 14:26:32]

piman@chromium.org: PTAL.

[Alex Z., 2017-04-28 14:26:42]

Description was changed from

==========
Remove SurfaceFactory And SurfaceFactoryClient

NOTE: This is currently blocked by Issue 2795683003.

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
==========

to

==========
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
==========

[Fady Samuel, 2017-04-28 14:28:55]

piman@ is OOO. Please pass to danakj@?

[Alex Z., 2017-04-28 15:19:01]

staraz@chromium.org changed reviewers:
+ danakj@chromium.org, nasko@chromium.org
- piman@chromium.org

[Alex Z., 2017-04-28 15:19:02]

danakj@: Please review changes in cc.

nasko@: Please review changes in content/browser.

[commit-bot: I haz the power, 2017-04-28 15:24:25]

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

[commit-bot: I haz the power, 2017-04-28 15:24:26]

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

[danakj, 2017-04-28 17:44:23]

https://codereview.chromium.org/2802023002/diff/260001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/2802023002/diff/260001/cc/BUILD.gn#newcode689
cc/BUILD.gn:689: "//testing/gmock",
These are already both here?

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:354:
copy_request->SendEmptyResult();
The destructor already does this
https://cs.chromium.org/chromium/src/cc/output/copy_output_request.cc?rcl=856...

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:27: }  // namespae test
typo, but you don't need this comment when the block is this small.

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:82: friend class Surface;
Can you explain these friends, why they are needed? How can we get rid of them?

[nasko, 2017-04-28 18:16:06]

content/ rubberstamp LGTM

[Alex Z., 2017-05-01 14:47:01]

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

[commit-bot: I haz the power, 2017-05-01 14:47:05]

Dry run: CQ is trying da patch.

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

[Alex Z., 2017-05-01 14:48:34]

https://codereview.chromium.org/2802023002/diff/260001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/2802023002/diff/260001/cc/BUILD.gn#newcode689
cc/BUILD.gn:689: "//testing/gmock",
On 2017/04/28 17:44:23, danakj wrote:
> These are already both here?

Done.

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:354:
copy_request->SendEmptyResult();
On 2017/04/28 17:44:23, danakj wrote:
> The destructor already does this
>
https://cs.chromium.org/chromium/src/cc/output/copy_output_request.cc?rcl=856...

Done.

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:27: }  // namespae test
On 2017/04/28 17:44:23, danakj wrote:
> typo, but you don't need this comment when the block is this small.

Done.

https://codereview.chromium.org/2802023002/diff/260001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:82: friend class Surface;
On 2017/04/28 17:44:23, danakj wrote:
> Can you explain these friends, why they are needed? How can we get rid of
them?

Done.

[commit-bot: I haz the power, 2017-05-01 16:09:31]

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

[commit-bot: I haz the power, 2017-05-01 16:09:32]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-05-01 22:21:39]

https://codereview.chromium.org/2802023002/diff/280001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2802023002/diff/280001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:82: // Surface calls
ReceiveFromChild from Surface::QueueFrame to receive and
It looks like this is creating a method-call pattern of 

A::Foo() -> B::Bar() -> A::Baz()

Which creates a cycle between classes and is usually a code smell issue for me.
Why would this class call Surface::QueueFrame to have it call back to
ReceiveFromChild? Why wouldn't this class just do its ReceiveFromChild work on
its own, then call QueueFrame (or whatever order works)?

https://codereview.chromium.org/2802023002/diff/280001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:93: // SurfaceAggregator calls
RefResources in PrewalkTree to validate the
I don't understand this pattern here of making friends for classes. If the
method is used externally, then why isn't it public? It looks like that would
cover tests, too, which are just testing the public API used by the aggregator.
Can we make Ref/Unref public?

[Alex Z., 2017-05-02 14:46:06]

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

[Alex Z., 2017-05-02 14:46:17]

https://codereview.chromium.org/2802023002/diff/280001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2802023002/diff/280001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:82: // Surface calls
ReceiveFromChild from Surface::QueueFrame to receive and
On 2017/05/01 22:21:39, danakj wrote:
> It looks like this is creating a method-call pattern of 
> 
> A::Foo() -> B::Bar() -> A::Baz()
> 
> Which creates a cycle between classes and is usually a code smell issue for
me.
> Why would this class call Surface::QueueFrame to have it call back to
> ReceiveFromChild? Why wouldn't this class just do its ReceiveFromChild work on
> its own, then call QueueFrame (or whatever order works)?

CompositorFrameSinkSupport now calls ReceiveFromChild prior to 
QueueFrame. I'm still thinking about Ref/UnrefResources.

fsamuel@ doesn't want to expose them because they are not part of the 
public CompositorFrameSink interface.

[commit-bot: I haz the power, 2017-05-02 14:46:23]

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, 2017-05-02 16:03:28]

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

[commit-bot: I haz the power, 2017-05-02 16:03:29]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2017-05-02 16:32:00]

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

[commit-bot: I haz the power, 2017-05-02 16:32:26]

Dry run: CQ is trying da patch.

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

[Alex Z., 2017-05-02 16:34:32]

https://codereview.chromium.org/2802023002/diff/280001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2802023002/diff/280001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:93: // SurfaceAggregator calls
RefResources in PrewalkTree to validate the
On 2017/05/01 22:21:39, danakj wrote:
> I don't understand this pattern here of making friends for classes. If the
> method is used externally, then why isn't it public? It looks like that would
> cover tests, too, which are just testing the public API used by the
aggregator.
> Can we make Ref/Unref public?

Ref/UnrefResources are public now. I added a TODO to find a workaround 
and set them back to private in the future.

[Alex Z., 2017-05-02 16:36:30]

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

[commit-bot: I haz the power, 2017-05-02 16:36:56]

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, 2017-05-02 18:29:14]

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

[commit-bot: I haz the power, 2017-05-02 18:29:16]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-05-03 16:08:40]

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:141: if (current_surface_ &&
create_new_surface) {
if current_surface_ is not null, then create_new_surface is true (if
create_new_surface is false we move current_surface_ making it null). So these
are both true together always when current_surface_ is true, making the 2nd part
redundant, if I read this correct.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:69: // TODO(fsamuel, staraz): Move
RefResources and UnrefResources to private.
nit: Just need one name in a TODO, whoever would best be able to explain it to
future readers

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:148: bool needs_sync_points_;
const? (and pass to the constructor?)

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (left):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:257:
TEST_F(CompositorFrameSinkSupportTest, RootSurfaceReceivesReferences) {
I don't understand why the CFS tests would go away when it's just taking over
more responsibilities?

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:121: void
OnSurfaceDamaged(const SurfaceId& id, bool* changed) override {
no whitespace between or add a comment here to say what this is overriding

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:125:
~CompositorFrameSinkSupportTest() override {
destructor right after constructor

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/frame_sink...
File cc/surfaces/frame_sink_manager.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/frame_sink...
cc/surfaces/frame_sink_manager.cc:129: if (client_iter != clients_.end()) {
unrelated diff

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:31:
compositor_frame_sink_support_(compositor_frame_sink_support),
move?

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.h#...
cc/surfaces/surface.h:41: using DrawCallback = base::Callback<void()>;
this is written base::Closure

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:100: void SurfaceAggregator::UnrefHelper(
Leave this as a file-static function, not a static method in SurfaceAggregator

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface_sy...
File cc/surfaces/surface_synchronization_unittest.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface_sy...
cc/surfaces/surface_synchronization_unittest.cc:548: MakeResource(1337 /* id */,
ALPHA_8 /* format */, 1234 /* filter */,
This is exactly 1 caller of MakeResource.. can you just do the thing here and
not use a function? Then I don't have to feel a bit off about MakeResource being
in a file for CompositorFrames

https://codereview.chromium.org/2802023002/diff/320001/cc/test/compositor_fra...
File cc/test/compositor_frame_helpers.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/test/compositor_fra...
cc/test/compositor_frame_helpers.cc:21: return
MakeCompositorFrame(embedded_surfaces, embedded_surfaces,
You could copy once to a local, and move both, instead of making 2 copies (this
would have 3 vectors with the same data in them instead of 2 atm)

https://codereview.chromium.org/2802023002/diff/320001/cc/test/compositor_fra...
File cc/test/compositor_frame_helpers.h (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/test/compositor_fra...
cc/test/compositor_frame_helpers.h:35: CompositorFrame
MakeCompositorFrame(std::vector<SurfaceId> embedded_surfaces,
wdyt of removing other MakeCompositorFrame() overrides that take vectors and
just keep this one. Passing a {} is pretty easy. There's a lot going on here in
this file, but it's actually not a lot

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:11: bool
create_surface_during_eviction) {
this is never true right? remove it?

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:27: }  // namespace test
whitespace above this

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.h (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.h:29: ReturnedResourceArray&
last_returned_resources() {
Why isn't the test instead expecting the resources via EXPECT_CALL/EXPECT_THAT
on the appropriate method? Do we need this?

[Alex Z., 2017-05-03 18:07:26]

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

[Alex Z., 2017-05-03 18:07:37]

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.cc:141: if (current_surface_ &&
create_new_surface) {
On 2017/05/03 16:08:39, danakj wrote:
> if current_surface_ is not null, then create_new_surface is true (if
> create_new_surface is false we move current_surface_ making it null). So these
> are both true together always when current_surface_ is true, making the 2nd
part
> redundant, if I read this correct.

You are right. Thanks for pointing it out.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:69: // TODO(fsamuel, staraz): Move
RefResources and UnrefResources to private.
On 2017/05/03 16:08:39, danakj wrote:
> nit: Just need one name in a TODO, whoever would best be able to explain it to
> future readers

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:148: bool needs_sync_points_;
On 2017/05/03 16:08:39, danakj wrote:
> const? (and pass to the constructor?)

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (left):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:257:
TEST_F(CompositorFrameSinkSupportTest, RootSurfaceReceivesReferences) {
On 2017/05/03 16:08:39, danakj wrote:
> I don't understand why the CFS tests would go away when it's just taking over
> more responsibilities?

After bringing the original SurfaceFactoryTest in here the file became
huge (fsamuel@'s comment: 
https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...).

The old CFSS tests (now "SurfaceSynchronizationTest") and the 
SurfaceFactoryTest are mutually exclusive in terms of the 
functionalities they are testing. We decided to put them in separate 
files to keep things clean.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:121: void
OnSurfaceDamaged(const SurfaceId& id, bool* changed) override {
On 2017/05/03 16:08:39, danakj wrote:
> no whitespace between or add a comment here to say what this is overriding

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:125:
~CompositorFrameSinkSupportTest() override {
On 2017/05/03 16:08:39, danakj wrote:
> destructor right after constructor

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/frame_sink...
File cc/surfaces/frame_sink_manager.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/frame_sink...
cc/surfaces/frame_sink_manager.cc:129: if (client_iter != clients_.end()) {
On 2017/05/03 16:08:39, danakj wrote:
> unrelated diff

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:31:
compositor_frame_sink_support_(compositor_frame_sink_support),
On 2017/05/03 16:08:39, danakj wrote:
> move?

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.h#...
cc/surfaces/surface.h:41: using DrawCallback = base::Callback<void()>;
On 2017/05/03 16:08:39, danakj wrote:
> this is written base::Closure

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:100: void SurfaceAggregator::UnrefHelper(
On 2017/05/03 16:08:39, danakj wrote:
> Leave this as a file-static function, not a static method in SurfaceAggregator

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface_sy...
File cc/surfaces/surface_synchronization_unittest.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface_sy...
cc/surfaces/surface_synchronization_unittest.cc:548: MakeResource(1337 /* id */,
ALPHA_8 /* format */, 1234 /* filter */,
On 2017/05/03 16:08:39, danakj wrote:
> This is exactly 1 caller of MakeResource.. can you just do the thing here and
> not use a function? Then I don't have to feel a bit off about MakeResource
being
> in a file for CompositorFrames

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/test/compositor_fra...
File cc/test/compositor_frame_helpers.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/test/compositor_fra...
cc/test/compositor_frame_helpers.cc:21: return
MakeCompositorFrame(embedded_surfaces, embedded_surfaces,
On 2017/05/03 16:08:39, danakj wrote:
> You could copy once to a local, and move both, instead of making 2 copies
(this
> would have 3 vectors with the same data in them instead of 2 atm)

This function is removed.

https://codereview.chromium.org/2802023002/diff/320001/cc/test/compositor_fra...
File cc/test/compositor_frame_helpers.h (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/test/compositor_fra...
cc/test/compositor_frame_helpers.h:35: CompositorFrame
MakeCompositorFrame(std::vector<SurfaceId> embedded_surfaces,
On 2017/05/03 16:08:39, danakj wrote:
> wdyt of removing other MakeCompositorFrame() overrides that take vectors and
> just keep this one. Passing a {} is pretty easy. There's a lot going on here
in
> this file, but it's actually not a lot

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:11: bool
create_surface_during_eviction) {
On 2017/05/03 16:08:40, danakj wrote:
> this is never true right? remove it?

It is true in CompositorFrameSinkSupportTest.AddDuringEviction.

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:27: }  // namespace test
On 2017/05/03 16:08:39, danakj wrote:
> whitespace above this

Done.

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.h (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.h:29: ReturnedResourceArray&
last_returned_resources() {
On 2017/05/03 16:08:40, danakj wrote:
> Why isn't the test instead expecting the resources via EXPECT_CALL/EXPECT_THAT
> on the appropriate method? Do we need this?

Done.

[commit-bot: I haz the power, 2017-05-03 18:08:47]

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, 2017-05-03 20:05:22]

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

[commit-bot: I haz the power, 2017-05-03 20:05:24]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-05-05 15:54:06]

Is there anything blocking this CL at this point?

[danakj, 2017-05-05 15:56:38]

No I'm in the mid of reviewing it again.

On Fri, May 5, 2017 at 11:54 AM, <fsamuel@chromium.org> wrote:

> Is there anything blocking this CL at this point?
>
> https://codereview.chromium.org/2802023002/
>

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

[danakj, 2017-05-05 16:04:19]

LGTM but lets move the use of the mock class outside of the mock itself

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support_unittest.cc (left):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support_unittest.cc:257:
TEST_F(CompositorFrameSinkSupportTest, RootSurfaceReceivesReferences) {
On 2017/05/03 18:07:37, Alex Z. wrote:
> On 2017/05/03 16:08:39, danakj wrote:
> > I don't understand why the CFS tests would go away when it's just taking
over
> > more responsibilities?
> 
> After bringing the original SurfaceFactoryTest in here the file became
> huge (fsamuel@'s comment: 
>
https://codereview.chromium.org/2802023002/diff/160001/cc/surfaces/compositor...).
> 
> The old CFSS tests (now "SurfaceSynchronizationTest") and the 
> SurfaceFactoryTest are mutually exclusive in terms of the 
> functionalities they are testing. We decided to put them in separate 
> files to keep things clean.

Oh I see, the diff shows the changes in the file there as being very small, but
these just moved, thanks.

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/surfaces/surface.h#...
cc/surfaces/surface.h:41: using DrawCallback = base::Callback<void()>;
On 2017/05/03 18:07:37, Alex Z. wrote:
> On 2017/05/03 16:08:39, danakj wrote:
> > this is written base::Closure
> 
> Done.

FWIW I was okay with the typedef, but I mean Callback<void()> is Closure. This
is fine how you did it now too if you like.

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.cc (right):

https://codereview.chromium.org/2802023002/diff/320001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:11: bool
create_surface_during_eviction) {
On 2017/05/03 18:07:37, Alex Z. wrote:
> On 2017/05/03 16:08:40, danakj wrote:
> > this is never true right? remove it?
> 
> It is true in CompositorFrameSinkSupportTest.AddDuringEviction.

I see. Can you move the ON_CALL stuff to that test then and remove it from this
class? Then support_ can go too, and this is just a mock interface here.

https://codereview.chromium.org/2802023002/diff/340001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.cc (right):

https://codereview.chromium.org/2802023002/diff/340001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:18:
&MockCompositorFrameSinkSupportClient::ReclaimResourcesInternal));
no longer does anything, you can remove it

https://codereview.chromium.org/2802023002/diff/340001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:32: const
ReturnedResourceArray& resources) {}
remove this method

[Fady Samuel, 2017-05-05 16:50:29]

https://codereview.chromium.org/2802023002/diff/340001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (left):

https://codereview.chromium.org/2802023002/diff/340001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:65: // regardless of whether it's pending or active.
nit: Move this back. Thanks!

[Alex Z., 2017-05-05 18:57:49]

Thanks for taking the time to review this!

https://codereview.chromium.org/2802023002/diff/340001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (left):

https://codereview.chromium.org/2802023002/diff/340001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:65: // regardless of whether it's pending or active.
On 2017/05/05 16:50:28, Fady Samuel wrote:
> nit: Move this back. Thanks!

For documentation's sake: CompositorFrameSinkSupport::ReceiveFromChild
is restored because we haven't found a good solution for 
Ref/UnrefResources() and it is generally a good practice to ref and 
unref resources in one class (cc::Surface in this case).

https://codereview.chromium.org/2802023002/diff/340001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.cc (right):

https://codereview.chromium.org/2802023002/diff/340001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:18:
&MockCompositorFrameSinkSupportClient::ReclaimResourcesInternal));
On 2017/05/05 16:04:19, danakj wrote:
> no longer does anything, you can remove it

Done.

https://codereview.chromium.org/2802023002/diff/340001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:32: const
ReturnedResourceArray& resources) {}
On 2017/05/05 16:04:19, danakj wrote:
> remove this method

Done.

[Alex Z., 2017-05-05 18:58:19]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2017-05-05 18:58:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, nasko@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2802023002/#ps360001
(title: "Address nits and rebase")

[commit-bot: I haz the power, 2017-05-05 18:58:49]

CQ is trying da patch.

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

[danakj, 2017-05-05 19:47:53]

Thanks :)

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.cc (right):

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:7: using testing::Invoke;
not used now

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.h (right):

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.h:8: #include
"cc/surfaces/compositor_frame_sink_support.h"
not used

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.h:10: #include
"cc/test/compositor_frame_helpers.h"
not used

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.h:12: #include
"testing/gtest/include/gtest/gtest.h"
same?

[Alex Z., 2017-05-05 19:49:37]

The CQ bit was unchecked by staraz@chromium.org

[Alex Z., 2017-05-05 19:57:20]

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

[Alex Z., 2017-05-05 19:57:29]

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.cc (right):

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.cc:7: using testing::Invoke;
On 2017/05/05 19:47:52, danakj wrote:
> not used now

Done.

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
File cc/test/mock_compositor_frame_sink_support_client.h (right):

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.h:8: #include
"cc/surfaces/compositor_frame_sink_support.h"
On 2017/05/05 19:47:52, danakj wrote:
> not used

Done.

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.h:10: #include
"cc/test/compositor_frame_helpers.h"
On 2017/05/05 19:47:52, danakj wrote:
> not used

Done.

https://codereview.chromium.org/2802023002/diff/360001/cc/test/mock_composito...
cc/test/mock_compositor_frame_sink_support_client.h:12: #include
"testing/gtest/include/gtest/gtest.h"
On 2017/05/05 19:47:53, danakj wrote:
> same?

Done.

[commit-bot: I haz the power, 2017-05-05 19:58:18]

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, 2017-05-05 23:22:23]

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

[commit-bot: I haz the power, 2017-05-05 23:22:25]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2017-05-06 01:36:40]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2017-05-06 01:36:41]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
fsamuel@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2802023002/#ps380001
(title: "Address Comments")

[commit-bot: I haz the power, 2017-05-06 01:37:17]

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, 2017-05-06 01:47:19]

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

[commit-bot: I haz the power, 2017-05-06 01:47:21]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alex Z., 2017-05-08 02:10:16]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2017-05-08 02:10:17]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
fsamuel@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2802023002/#ps420001
(title: "Rebase SurfaceSynchronizationTest")

[commit-bot: I haz the power, 2017-05-08 02:10:28]

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, 2017-05-08 03:19:05]

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

[commit-bot: I haz the power, 2017-05-08 03:19:07]

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

[Fady Samuel, 2017-05-08 03:19:48]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2017-05-08 03:19:56]

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, 2017-05-08 04:38:47]

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

[commit-bot: I haz the power, 2017-05-08 04:38:49]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2017-05-08 09:39:39]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2017-05-08 09:40:00]

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, 2017-05-08 10:47:20]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1494236379138980,
"parent_rev": "8c70962f2be02fc57fc5bea12c7f7052605709ce", "commit_rev":
"c0f2d73db929c46c1dd9fc14e627a0f7d39469c1"}

[commit-bot: I haz the power, 2017-05-08 10:47:32]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/c0f2d73db929c46c1dd9fc14e627...
==========

[commit-bot: I haz the power, 2017-05-08 10:47:34]

Committed patchset #21 (id:420001) as
https://chromium.googlesource.com/chromium/src/+/c0f2d73db929c46c1dd9fc14e627...