ui::ContextFactory should not create SurfaceIdAllocators

ui::ContextFactory should not create SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients
and so ui::ContextFactory should not create a
SurfaceIdAllocator directly. Instead, ui::ContextFactory
allocates a new surface client ID, which can be used to
create a SurfaceIdAllocator in clients.

This CL also moves calls to RegisterSurfaceClientId and
InvalidateSurfaceClientId close to
RegisterSurfaceClientFactory and
UnregisterSurfaceClientFactory respectively. In a
subsequent CL, these two will be merged together into
SurfaceFactory.

BUG=627283, 629940
TBR=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0fd36aebebd0d908cd9f7fcc51d45b68d1c73e9f
Cr-Commit-Position: refs/heads/master@{#407273}

[Fady Samuel, 2016-07-21 16:51:49]

Description was changed from

==========
ui::Compositor should not allocate SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients and so
ui::Compositor should not allocate a SurfaceIdAllocator directly.
Instead, ui::Compositor allocates a new surface client ID, which
can be used to create a SurfaceIdAllocator in clients.

BUG=627283, 629940
==========

to

==========
ui::Compositor should not allocate SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients and so
ui::Compositor should not allocate a SurfaceIdAllocator directly.
Instead, ui::Compositor allocates a new surface client ID, which
can be used to create a SurfaceIdAllocator in clients.

BUG=627283, 629940
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2016-07-21 16:53:28]

fsamuel@chromium.org changed reviewers:
+ piman@chromium.org, sky@chromium.org

[Fady Samuel, 2016-07-21 16:53:29]

+piman@ for content
+sky@ for ui and ash.

[Fady Samuel, 2016-07-21 16:53:36]

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

[commit-bot: I haz the power, 2016-07-21 16:53:59]

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-07-21 17:05:32]

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

[commit-bot: I haz the power, 2016-07-21 17:05:34]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[piman, 2016-07-21 17:07:11]

I was thrown off by the CL description at first... You say "ui::Compositor
should not allocate SurfaceIdAllocators" but the SurfaceIdAllocators allocation
does move to ui::Compositor. Did you mean that ContextFactory should not
allocate SurfaceIdAllocators?

https://codereview.chromium.org/2166423002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.cc (left):

https://codereview.chromium.org/2166423002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.cc:695:
allocator->RegisterSurfaceClientId(GetSurfaceManager());
What does this now?

[Fady Samuel, 2016-07-21 17:10:54]

Description was changed from

==========
ui::Compositor should not allocate SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients and so
ui::Compositor should not allocate a SurfaceIdAllocator directly.
Instead, ui::Compositor allocates a new surface client ID, which
can be used to create a SurfaceIdAllocator in clients.

BUG=627283, 629940
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
ui::ContextFactory should not allocate SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients and so
ui::ContextFactory should not allocate a 
SurfaceIdAllocator directly. Instead, ui::ContextFactory 
allocates a new surface client ID, which can be used to 
create a SurfaceIdAllocator in clients.

BUG=627283, 629940
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2016-07-21 17:15:01]

Yea, that shouldn't said ContextFactory not Compositor. Fixed + Addressed your
comment! PTAL Antoine!

https://codereview.chromium.org/2166423002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.cc (left):

https://codereview.chromium.org/2166423002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.cc:695:
allocator->RegisterSurfaceClientId(GetSurfaceManager());
On 2016/07/21 17:07:11, piman wrote:
> What does this now?

Ooops fixed. For context on where I'm going with this see this experimental
patch: https://codereview.chromium.org/2144733005/

I'd like SurfaceFactory to have a 1-1 correspondence to a single client and for
the local ID and nonce part of a surface ID to be allocated in the client.

[Fady Samuel, 2016-07-21 17:17:19]

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

[commit-bot: I haz the power, 2016-07-21 17:17: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, 2016-07-21 17:19:09]

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

[commit-bot: I haz the power, 2016-07-21 17:19:12]

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-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[piman, 2016-07-21 17:19:47]

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:693:
GetSurfaceManager()->RegisterSurfaceClientId(client_id);
So, the previous code also relied on SurfaceIdAllocator::RegisterSurfaceClientId
setting up the manager_ field on the the SurfaceIdAllocator, so that in
~SurfaceIdAllocator, SurfaceManager::InvalidateSurfaceClientId is called.

The latter doesn't happen any more. Is it not needed?

[Fady Samuel, 2016-07-21 17:22:23]

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:693:
GetSurfaceManager()->RegisterSurfaceClientId(client_id);
On 2016/07/21 17:19:47, piman wrote:
> So, the previous code also relied on
SurfaceIdAllocator::RegisterSurfaceClientId
> setting up the manager_ field on the the SurfaceIdAllocator, so that in
> ~SurfaceIdAllocator, SurfaceManager::InvalidateSurfaceClientId is called.
> 
> The latter doesn't happen any more. Is it not needed?

Well, it is still needed, but SurfaceIdAllocator will take care of that on
destruction so the code shouldn't be any different here. In the end state, this
won't be needed though because SurfaceFactory will take in a client ID and do
all the necessary set up and tear down work.

[piman, 2016-07-21 17:28:40]

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:693:
GetSurfaceManager()->RegisterSurfaceClientId(client_id);
On 2016/07/21 17:22:23, Fady Samuel wrote:
> On 2016/07/21 17:19:47, piman wrote:
> > So, the previous code also relied on
> SurfaceIdAllocator::RegisterSurfaceClientId
> > setting up the manager_ field on the the SurfaceIdAllocator, so that in
> > ~SurfaceIdAllocator, SurfaceManager::InvalidateSurfaceClientId is called.
> > 
> > The latter doesn't happen any more. Is it not needed?
> 
> Well, it is still needed, but SurfaceIdAllocator will take care of that on
> destruction so the code shouldn't be any different here.

With this patch, the SurfaceIdAllocator doesn't know about the SurfaceManager
any more, so skips InvalidateSurfaceClientId in the destructor (manager_ would
be null - unless I missed something?)

> In the end state, this
> won't be needed though because SurfaceFactory will take in a client ID and do
> all the necessary set up and tear down work.

Got it.

[piman, 2016-07-21 17:28:40]

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:693:
GetSurfaceManager()->RegisterSurfaceClientId(client_id);
On 2016/07/21 17:22:23, Fady Samuel wrote:
> On 2016/07/21 17:19:47, piman wrote:
> > So, the previous code also relied on
> SurfaceIdAllocator::RegisterSurfaceClientId
> > setting up the manager_ field on the the SurfaceIdAllocator, so that in
> > ~SurfaceIdAllocator, SurfaceManager::InvalidateSurfaceClientId is called.
> > 
> > The latter doesn't happen any more. Is it not needed?
> 
> Well, it is still needed, but SurfaceIdAllocator will take care of that on
> destruction so the code shouldn't be any different here.

With this patch, the SurfaceIdAllocator doesn't know about the SurfaceManager
any more, so skips InvalidateSurfaceClientId in the destructor (manager_ would
be null - unless I missed something?)

> In the end state, this
> won't be needed though because SurfaceFactory will take in a client ID and do
> all the necessary set up and tear down work.

Got it.

[sky, 2016-07-21 19:34:05]

LGTM

[Fady Samuel, 2016-07-21 20:36:17]

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

[commit-bot: I haz the power, 2016-07-21 20:36:36]

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

[Fady Samuel, 2016-07-21 20:43:33]

PTAL Antoine

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2166423002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:693:
GetSurfaceManager()->RegisterSurfaceClientId(client_id);
On 2016/07/21 17:28:40, piman wrote:
> On 2016/07/21 17:22:23, Fady Samuel wrote:
> > On 2016/07/21 17:19:47, piman wrote:
> > > So, the previous code also relied on
> > SurfaceIdAllocator::RegisterSurfaceClientId
> > > setting up the manager_ field on the the SurfaceIdAllocator, so that in
> > > ~SurfaceIdAllocator, SurfaceManager::InvalidateSurfaceClientId is called.
> > > 
> > > The latter doesn't happen any more. Is it not needed?
> > 
> > Well, it is still needed, but SurfaceIdAllocator will take care of that on
> > destruction so the code shouldn't be any different here.
> 
> With this patch, the SurfaceIdAllocator doesn't know about the SurfaceManager
> any more, so skips InvalidateSurfaceClientId in the destructor (manager_ would
> be null - unless I missed something?)
> 
> > In the end state, this
> > won't be needed though because SurfaceFactory will take in a client ID and
do
> > all the necessary set up and tear down work.
> 
> Got it.

I've made all the clients that create SurfaceIdAllocators Register and
invalidate the client IDs themselves. This takes us in the right direction for
bundling this into SurfaceFactory in a subsequent CL.

[Fady Samuel, 2016-07-21 20:43:43]

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

[commit-bot: I haz the power, 2016-07-21 20:44:05]

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

[piman, 2016-07-21 21:02:22]

lgtm

https://codereview.chromium.org/2166423002/diff/60001/cc/test/test_delegating...
File cc/test/test_delegating_output_surface.cc (right):

https://codereview.chromium.org/2166423002/diff/60001/cc/test/test_delegating...
cc/test/test_delegating_output_surface.cc:63: if (surface_manager_)
nit: {}

[Fady Samuel, 2016-07-21 21:11:29]

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

[Fady Samuel, 2016-07-21 21:11:30]

+boliu@ for android_webview

[Fady Samuel, 2016-07-21 21:21:02]

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

[Fady Samuel, 2016-07-21 21:21:03]

+reveman@ for exo

https://codereview.chromium.org/2166423002/diff/60001/cc/test/test_delegating...
File cc/test/test_delegating_output_surface.cc (right):

https://codereview.chromium.org/2166423002/diff/60001/cc/test/test_delegating...
cc/test/test_delegating_output_surface.cc:63: if (surface_manager_)
On 2016/07/21 21:02:22, piman wrote:
> nit: {}

actually TestDelegatingOuptutSurface owns SurfaceManager, so I don't need this
test.

[Fady Samuel, 2016-07-21 21:22:53]

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

[commit-bot: I haz the power, 2016-07-21 21:23:26]

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

[boliu, 2016-07-21 21:24:13]

lgtm

[Fady Samuel, 2016-07-21 21:58:17]

reveman@: I'm TBR'ing you for a small change in components/exo/surface.cc since
this CL has gotten overall review + OWNER coverage from all but one file.

[Fady Samuel, 2016-07-21 21:58:30]

Description was changed from

==========
ui::ContextFactory should not allocate SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients and so
ui::ContextFactory should not allocate a 
SurfaceIdAllocator directly. Instead, ui::ContextFactory 
allocates a new surface client ID, which can be used to 
create a SurfaceIdAllocator in clients.

BUG=627283, 629940
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
ui::ContextFactory should not allocate SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients and so
ui::ContextFactory should not allocate a 
SurfaceIdAllocator directly. Instead, ui::ContextFactory 
allocates a new surface client ID, which can be used to 
create a SurfaceIdAllocator in clients.

BUG=627283, 629940
TBR=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2016-07-21 22:09:10]

On 2016/07/21 21:58:17, Fady Samuel wrote:
> reveman@: I'm TBR'ing you for a small change in components/exo/surface.cc
since
> this CL has gotten overall review + OWNER coverage from all but one file.

Actually, I got things wrong in exo. Sending a fix out shortly before CQ'ing.

[Fady Samuel, 2016-07-21 22:19:02]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-07-21 22:19:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
piman@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2166423002/#ps140001
(title: "No-op")

[commit-bot: I haz the power, 2016-07-21 22:19:44]

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-07-21 22:59:43]

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

[commit-bot: I haz the power, 2016-07-21 22:59:45]

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

[Fady Samuel, 2016-07-22 03:00:07]

Some tests fail BindToClient which clears the surface_manager in
TestDelegatingOutputSurface, then we try to access it in the destructor and that
can cause tests to crash. I added a guard on surface_manager in the destructor
to catch this.

[Fady Samuel, 2016-07-22 03:01:12]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-07-22 03:01:13]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
boliu@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2166423002/#ps160001
(title: "Fixed TestDelegatingOutputSurface")

[danakj, 2016-07-22 03:02:18]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2016-07-22 03:02:19]

https://codereview.chromium.org/2166423002/diff/160001/cc/test/test_delegatin...
File cc/test/test_delegating_output_surface.cc (right):

https://codereview.chromium.org/2166423002/diff/160001/cc/test/test_delegatin...
cc/test/test_delegating_output_surface.cc:64:
surface_manager_->InvalidateSurfaceClientId(
Why is this here and not in DetachOutputSurface where
UnregisterSurfaceFactoryClient is? Is that intended? Are you aware this
currently runs on the main thread not the impl thread?

[commit-bot: I haz the power, 2016-07-22 03:02:27]

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

[danakj, 2016-07-22 03:02:28]

The CQ bit was unchecked by danakj@chromium.org

[Fady Samuel, 2016-07-22 03:27:24]

The CQ bit was checked by fsamuel@chromium.org

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

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

[Fady Samuel, 2016-07-22 03:42:31]

The CQ bit was unchecked by fsamuel@chromium.org

[Fady Samuel, 2016-07-22 03:53:26]

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

[commit-bot: I haz the power, 2016-07-22 03:53: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, 2016-07-22 05:11:23]

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

[commit-bot: I haz the power, 2016-07-22 05:11:24]

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

[Fady Samuel, 2016-07-22 12:23:08]

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

[commit-bot: I haz the power, 2016-07-22 12:23:21]

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

[Fady Samuel, 2016-07-22 12:23:38]

Fixed TestDelegatingOutputSurface + RenderWidgetHostViewAndroid. PTAL Dana and
David!

https://codereview.chromium.org/2166423002/diff/160001/cc/test/test_delegatin...
File cc/test/test_delegating_output_surface.cc (right):

https://codereview.chromium.org/2166423002/diff/160001/cc/test/test_delegatin...
cc/test/test_delegating_output_surface.cc:64:
surface_manager_->InvalidateSurfaceClientId(
On 2016/07/22 03:02:19, danakj wrote:
> Why is this here and not in DetachOutputSurface where
> UnregisterSurfaceFactoryClient is? Is that intended? Are you aware this
> currently runs on the main thread not the impl thread?

It should absolutely be next to UnregisterSurfaceFactoryClient given I want to
merge the two! Doh! Fixed. Thanks for catching this!

[Fady Samuel, 2016-07-22 13:25:34]

Doh! I made the same mistake in the SynchronousCompositorOutputSurface! I was
calling InvalidateSurfaceClientId in the destructor instead of DetachFromClient
near UnregisterSurfaceClientFactory. It should be fixed now! Feeding the CL to
the bots one more time!

[Fady Samuel, 2016-07-22 13:25:51]

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

[commit-bot: I haz the power, 2016-07-22 13:26:03]

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

[reveman, 2016-07-22 14:08:46]

reveman@chromium.org changed reviewers:
+ jbauman@chromium.org

[reveman, 2016-07-22 14:08:47]

lgtm % jbauman's review of components/exo

[commit-bot: I haz the power, 2016-07-22 14:21:33]

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

[commit-bot: I haz the power, 2016-07-22 14:21:34]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-07-22 15:03:20]

OK, all tests pass! FINALLY! John and Dana, I'd appreciate your stamp of
approval! Thanks!

[Fady Samuel, 2016-07-22 15:34:28]

Description was changed from

==========
ui::ContextFactory should not allocate SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients and so
ui::ContextFactory should not allocate a 
SurfaceIdAllocator directly. Instead, ui::ContextFactory 
allocates a new surface client ID, which can be used to 
create a SurfaceIdAllocator in clients.

BUG=627283, 629940
TBR=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
ui::ContextFactory should not create SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients 
and so ui::ContextFactory should not create a 
SurfaceIdAllocator directly. Instead, ui::ContextFactory 
allocates a new surface client ID, which can be used to 
create a SurfaceIdAllocator in clients.

This CL also moves calls to RegisterSurfaceClientId and
InvalidateSurfaceClientId close to 
RegisterSurfaceClientFactory and 
UnregisterSurfaceClientFactory respectively. In a 
subsequent CL, these two will be merged together into 
SurfaceFactory.

BUG=627283, 629940
TBR=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jbauman, 2016-07-22 18:25:05]

lgtm

[danakj, 2016-07-22 19:01:59]

Thanks that looks better LGTM

https://codereview.chromium.org/2166423002/diff/240001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2166423002/diff/240001/content/renderer/andro...
content/renderer/android/synchronous_compositor_output_surface.cc:111:
SynchronousCompositorOutputSurface::~SynchronousCompositorOutputSurface() {}
nit: = default

[Fady Samuel, 2016-07-22 19:08:40]

Thanks Dana! CQ'ing! Thanks for your reviews, everyone!

https://codereview.chromium.org/2166423002/diff/240001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2166423002/diff/240001/content/renderer/andro...
content/renderer/android/synchronous_compositor_output_surface.cc:111:
SynchronousCompositorOutputSurface::~SynchronousCompositorOutputSurface() {}
On 2016/07/22 19:01:59, danakj wrote:
> nit: = default

Done.

[Fady Samuel, 2016-07-22 19:08:50]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-07-22 19:08:50]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
boliu@chromium.org, sky@chromium.org, reveman@chromium.org, danakj@chromium.org,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2166423002/#ps260001
(title: "Addressed Dana's nit")

[commit-bot: I haz the power, 2016-07-22 19:09:58]

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-07-22 21:50:33]

Description was changed from

==========
ui::ContextFactory should not create SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients 
and so ui::ContextFactory should not create a 
SurfaceIdAllocator directly. Instead, ui::ContextFactory 
allocates a new surface client ID, which can be used to 
create a SurfaceIdAllocator in clients.

This CL also moves calls to RegisterSurfaceClientId and
InvalidateSurfaceClientId close to 
RegisterSurfaceClientFactory and 
UnregisterSurfaceClientFactory respectively. In a 
subsequent CL, these two will be merged together into 
SurfaceFactory.

BUG=627283, 629940
TBR=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
ui::ContextFactory should not create SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients 
and so ui::ContextFactory should not create a 
SurfaceIdAllocator directly. Instead, ui::ContextFactory 
allocates a new surface client ID, which can be used to 
create a SurfaceIdAllocator in clients.

This CL also moves calls to RegisterSurfaceClientId and
InvalidateSurfaceClientId close to 
RegisterSurfaceClientFactory and 
UnregisterSurfaceClientFactory respectively. In a 
subsequent CL, these two will be merged together into 
SurfaceFactory.

BUG=627283, 629940
TBR=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-07-22 21:50:37]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-07-22 21:52:26]

Description was changed from

==========
ui::ContextFactory should not create SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients 
and so ui::ContextFactory should not create a 
SurfaceIdAllocator directly. Instead, ui::ContextFactory 
allocates a new surface client ID, which can be used to 
create a SurfaceIdAllocator in clients.

This CL also moves calls to RegisterSurfaceClientId and
InvalidateSurfaceClientId close to 
RegisterSurfaceClientFactory and 
UnregisterSurfaceClientFactory respectively. In a 
subsequent CL, these two will be merged together into 
SurfaceFactory.

BUG=627283, 629940
TBR=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
ui::ContextFactory should not create SurfaceIdAllocators

In the near future, SurfaceIdAllocator will live in clients
and so ui::ContextFactory should not create a
SurfaceIdAllocator directly. Instead, ui::ContextFactory
allocates a new surface client ID, which can be used to
create a SurfaceIdAllocator in clients.

This CL also moves calls to RegisterSurfaceClientId and
InvalidateSurfaceClientId close to
RegisterSurfaceClientFactory and
UnregisterSurfaceClientFactory respectively. In a
subsequent CL, these two will be merged together into
SurfaceFactory.

BUG=627283, 629940
TBR=reveman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0fd36aebebd0d908cd9f7fcc51d45b68d1c73e9f
Cr-Commit-Position: refs/heads/master@{#407273}
==========

[commit-bot: I haz the power, 2016-07-22 21:52:29]

Patchset 14 (id:??) landed as
https://crrev.com/0fd36aebebd0d908cd9f7fcc51d45b68d1c73e9f
Cr-Commit-Position: refs/heads/master@{#407273}