Make SurfaceFactory lifetime match frame_sink_id_ registration in BlimpCompositor

Make SurfaceFactory lifetime match frame_sink_id_ registration in BlimpCompositor

This will allow further simplification/refactoring.

BUG=657959
Committed: https://crrev.com/a5abb8e899397924617f1bac3c932146c2e5826e
Cr-Commit-Position: refs/heads/master@{#430475}

[piman, 2016-11-07 22:24:11]

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

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

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-07 22:31:37]

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

[piman, 2016-11-07 22:31:38]

https://codereview.chromium.org/2481213003/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor.cc (left):

https://codereview.chromium.org/2481213003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.cc:563: surface_factory_.reset();
I believe there's no need to do an explicit Reset here, because we reset the
host below, which synchronously unbinds and destroys the CompositorFrameSink
(which is the ProxyClient, and so, will call UnbindProxyClient which will do the
Reset). Either way, this is only called from the destructor, so it's probably
not necessary anyway.

https://codereview.chromium.org/2481213003/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2481213003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.cc:189: if (!proxy_client_)
Here and other places: even though proxy_client_ is a WeakPtr, it looks to have
exactly the same lifetime as what surface_factory_ used to have. It is set to a
!null value in BlimpCompositor::BindToProxyClient (which is called from the
ProxyClient), and reset to null in BlimpCompositor::UnbindProxyClient (which is
also called from the ProxyClient, i.e. it hasn't been destroyed yet). So I think
this transformation is valid, and preferable to add another boolean state
("bound to proxy"), but LMK if I missed something.

[commit-bot: I haz the power, 2016-11-07 22:57:14]

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

[commit-bot: I haz the power, 2016-11-07 22:57:15]

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

[piman, 2016-11-07 23:16:35]

Ah, NVM, I missed the threading constraints. Let me work on this a bit more.

[piman, 2016-11-08 00:14:26]

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

[commit-bot: I haz the power, 2016-11-08 00:14:49]

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-08 00:15:15]

On 2016/11/07 23:16:35, piman wrote:
> Ah, NVM, I missed the threading constraints. Let me work on this a bit more.

Ok, PTAL. I ended up adding a boolean.

[Khushal, 2016-11-08 00:17:22]

https://codereview.chromium.org/2481213003/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor.cc (left):

https://codereview.chromium.org/2481213003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.cc:563: surface_factory_.reset();
On 2016/11/07 22:31:38, piman wrote:
> I believe there's no need to do an explicit Reset here, because we reset the
> host below, which synchronously unbinds and destroys the CompositorFrameSink
> (which is the ProxyClient, and so, will call UnbindProxyClient which will do
the
> Reset). Either way, this is only called from the destructor, so it's probably
> not necessary anyway.

Makes sense. Thanks for the clean up!

https://codereview.chromium.org/2481213003/diff/1/blimp/client/core/composito...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2481213003/diff/1/blimp/client/core/composito...
blimp/client/core/compositor/blimp_compositor.cc:189: if (!proxy_client_)
On 2016/11/07 22:31:38, piman wrote:
> Here and other places: even though proxy_client_ is a WeakPtr, it looks to
have
> exactly the same lifetime as what surface_factory_ used to have. It is set to
a
> !null value in BlimpCompositor::BindToProxyClient (which is called from the
> ProxyClient), and reset to null in BlimpCompositor::UnbindProxyClient (which
is
> also called from the ProxyClient, i.e. it hasn't been destroyed yet). So I
think
> this transformation is valid, and preferable to add another boolean state
> ("bound to proxy"), but LMK if I missed something.

Won't be able to de-reference the weak ptr here since its bound to the
compositor thread. For this place, I think if we checked if the local_frame_id_
is null, that should be good enough to early out.

For the other places it is being used only for DCHECKs for calls that should be
made only when a frame sink is bound. I think adding a bool to do this should be
fine and may be more explicit. Wdyt?

[Khushal, 2016-11-08 00:26:51]

https://codereview.chromium.org/2481213003/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2481213003/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:457: DCHECK(bound_to_proxy_);
Hmmm, should we still keep this one? Earlier an unbind would have destroyed the
surface factory so there was no way this callback could run. Is that still the
case now after calling Reset on the factory?

[commit-bot: I haz the power, 2016-11-08 00:29:22]

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

[commit-bot: I haz the power, 2016-11-08 00:29:23]

Dry run: This issue passed the CQ dry run.

[piman, 2016-11-08 00:43:58]

https://codereview.chromium.org/2481213003/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2481213003/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:457: DCHECK(bound_to_proxy_);
On 2016/11/08 00:26:51, Khushal wrote:
> Hmmm, should we still keep this one? Earlier an unbind would have destroyed
the
> surface factory so there was no way this callback could run. Is that still the
> case now after calling Reset on the factory?

Actually, the draw callback is run even after the factory is destroyed. There's
no change relative to that in this CL... Maybe the DCHECK was already invalid
previously?
It seems harmless to remove. If we're not bound, proxy_client_ will be NULL on
the compositor thread and
BlimpCompositorFrameSinkProxyClient::SubmitCompositorFrameAck will not run.

[Khushal, 2016-11-08 00:51:45]

lgtm. Thanks!

https://codereview.chromium.org/2481213003/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2481213003/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:457: DCHECK(bound_to_proxy_);
On 2016/11/08 00:43:58, piman wrote:
> On 2016/11/08 00:26:51, Khushal wrote:
> > Hmmm, should we still keep this one? Earlier an unbind would have destroyed
> the
> > surface factory so there was no way this callback could run. Is that still
the
> > case now after calling Reset on the factory?
> 
> Actually, the draw callback is run even after the factory is destroyed.
There's
> no change relative to that in this CL... Maybe the DCHECK was already invalid
> previously?

Oh yes. Just looked through the code, you're right. Could've failed earlier
anyway. Removing it makes sense.

> It seems harmless to remove. If we're not bound, proxy_client_ will be NULL on
> the compositor thread and
> BlimpCompositorFrameSinkProxyClient::SubmitCompositorFrameAck will not run.

[piman, 2016-11-08 01:39:53]

The CQ bit was checked by piman@chromium.org

[piman, 2016-11-08 01:39:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2481213003/#ps40001
(title: "remove bad DCHECK")

[piman, 2016-11-08 01:40:13]

https://codereview.chromium.org/2481213003/diff/20001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2481213003/diff/20001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:457: DCHECK(bound_to_proxy_);
On 2016/11/08 00:51:45, Khushal wrote:
> On 2016/11/08 00:43:58, piman wrote:
> > On 2016/11/08 00:26:51, Khushal wrote:
> > > Hmmm, should we still keep this one? Earlier an unbind would have
destroyed
> > the
> > > surface factory so there was no way this callback could run. Is that still
> the
> > > case now after calling Reset on the factory?
> > 
> > Actually, the draw callback is run even after the factory is destroyed.
> There's
> > no change relative to that in this CL... Maybe the DCHECK was already
invalid
> > previously?
> 
> Oh yes. Just looked through the code, you're right. Could've failed earlier
> anyway. Removing it makes sense.
> 
> > It seems harmless to remove. If we're not bound, proxy_client_ will be NULL
on
> > the compositor thread and
> > BlimpCompositorFrameSinkProxyClient::SubmitCompositorFrameAck will not run.
> 

Done.

[commit-bot: I haz the power, 2016-11-08 01:40:22]

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-11-08 01:59:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-08 02:07:09]

Description was changed from

==========
Make SurfaceFactory lifetime match frame_sink_id_ registration in
BlimpCompositor

This will allow further simplification/refactoring.

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

to

==========
Make SurfaceFactory lifetime match frame_sink_id_ registration in
BlimpCompositor

This will allow further simplification/refactoring.

BUG=657959

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

[commit-bot: I haz the power, 2016-11-08 02:07:11]

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