Add SurfaceFactory::Reset

Add SurfaceFactory::Reset

This can be used to keep the SurfaceFactory alive across CompositorFrameSink
loss, to have its lifetime matchup better with the frame_sink_id_ registration,
allowing further refactoring / simplification

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

Committed: https://crrev.com/50ca188c4d5206a8475c76133d69acc066ee3856
Cr-Commit-Position: refs/heads/master@{#430349}

[piman, 2016-11-05 00:53:35]

Description was changed from

==========
Add SurfaceFactory::Reset

This can be used to keep the SurfaceFactory alive across CompositorFrameSink
loss, to have its lifetime matchup better with the frame_sink_id_ registration,
allowing further refactoring / simplification

BUG=657959
==========

to

==========
Add SurfaceFactory::Reset

This can be used to keep the SurfaceFactory alive across CompositorFrameSink
loss, to have its lifetime matchup better with the frame_sink_id_ registration,
allowing further refactoring / simplification

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

[piman, 2016-11-05 00:53:38]

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

[commit-bot: I haz the power, 2016-11-05 00:54:06]

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

[piman, 2016-11-05 01:09:27]

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

[commit-bot: I haz the power, 2016-11-05 01:09:37]

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

[piman, 2016-11-05 01:09:55]

Patchset #1 (id:1) has been deleted

[piman, 2016-11-05 01:11:48]

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

[piman, 2016-11-05 01:11:49]

PTAL. See https://codereview.chromium.org/2483623002/ for a use case (I have a
few more).

[commit-bot: I haz the power, 2016-11-05 03:01:56]

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

[commit-bot: I haz the power, 2016-11-05 03:01:57]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-11-05 05:26:45]

lgtm, although I wonder if a Reset() API makes sense once the display compositor
moves to the GPU process? Presumably SurfaceFactory will live there too and thus
if the context is lost, SurfaceFactory will go away, right? Thus, the problem of
returning resources after a lost context is a non-issue. I guess this API makes
sense in the interim though as a cleanup. Thanks!

https://codereview.chromium.org/2475773005/diff/20001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/2475773005/diff/20001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.cc:46: weak_factory_.InvalidateWeakPtrs();
nit: might be useful to comment about how Surfaces hold weak ptrs to their
factories to return resources

[piman, 2016-11-07 18:55:16]

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

[piman, 2016-11-07 18:55:22]

The CQ bit was checked by piman@chromium.org

[piman, 2016-11-07 18:55:23]

The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2475773005/#ps40001
(title: "Add comment per review comment")

[commit-bot: I haz the power, 2016-11-07 18:55:38]

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

[piman, 2016-11-07 18:55:41]

https://codereview.chromium.org/2475773005/diff/20001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/2475773005/diff/20001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.cc:46: weak_factory_.InvalidateWeakPtrs();
On 2016/11/05 05:26:44, Fady Samuel wrote:
> nit: might be useful to comment about how Surfaces hold weak ptrs to their
> factories to return resources

Done.

[piman, 2016-11-07 18:59:18]

On 2016/11/05 05:26:45, Fady Samuel wrote:
> lgtm, although I wonder if a Reset() API makes sense once the display
compositor
> moves to the GPU process? Presumably SurfaceFactory will live there too and
thus
> if the context is lost, SurfaceFactory will go away, right? Thus, the problem
of
> returning resources after a lost context is a non-issue. I guess this API
makes
> sense in the interim though as a cleanup. Thanks!

Right now it's needed because of code in the browser that manages resources for
other processes (renderer). When the renderer loses its output surface, it needs
the browser to discard its corresponding resources, and that's what this does.
Once the renderer submits frames to the GPU process directly, then that logic
can go away.

[commit-bot: I haz the power, 2016-11-07 19:50:55]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-11-07 20:04:58]

Description was changed from

==========
Add SurfaceFactory::Reset

This can be used to keep the SurfaceFactory alive across CompositorFrameSink
loss, to have its lifetime matchup better with the frame_sink_id_ registration,
allowing further refactoring / simplification

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

to

==========
Add SurfaceFactory::Reset

This can be used to keep the SurfaceFactory alive across CompositorFrameSink
loss, to have its lifetime matchup better with the frame_sink_id_ registration,
allowing further refactoring / simplification

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

Committed: https://crrev.com/50ca188c4d5206a8475c76133d69acc066ee3856
Cr-Commit-Position: refs/heads/master@{#430349}
==========

[commit-bot: I haz the power, 2016-11-07 20:04:59]

Patchset 2 (id:??) landed as
https://crrev.com/50ca188c4d5206a8475c76133d69acc066ee3856
Cr-Commit-Position: refs/heads/master@{#430349}