cc: Stop calling DetachFromClient if bind failed.

cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/8fa1509c2848015f60d931090d6da9720a98fec2
Cr-Commit-Position: refs/heads/master@{#424310}

[danakj, 2016-10-10 22:00:05]

Description was changed from

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
BUG=606056
==========

to

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-10-10 22:04:37]

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

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

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

[danakj, 2016-10-10 22:05:42]

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

[danakj, 2016-10-10 22:05:43]

enne: overall
boliu: content
khushal: blimp
sky: services

[danakj, 2016-10-10 22:08:48]

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

[danakj, 2016-10-10 22:08:56]

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

https://codereview.chromium.org/2404993002/diff/40001/cc/output/compositor_fr...
cc/output/compositor_frame_sink.h:39: // An interface for submitting
CompositorFrames to a display compositor
sky: You pointed out comments were missing/bad so here's a few improvements. LMK
if you think it can be better.

[commit-bot: I haz the power, 2016-10-10 22:09:16]

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

[danakj, 2016-10-10 22:13:04]

https://codereview.chromium.org/2404993002/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/renderer_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2404993002/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/renderer_compositor_frame_sink.cc:126: if (!HasClient())
This looks "interesting" but AFAICT
DetachFromClient->RemoveHandlerOnCompositorThread implies that this method will
not be called again, as the message filter uses the map (on this same thread) to
find this class to call OnMessageReceived but it'll be removed from it.

[enne (OOO), 2016-10-10 22:27:11]

https://codereview.chromium.org/2404993002/diff/40001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2404993002/diff/40001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor_frame_sink.cc:66: FROM_HERE,
base::Bind(&BlimpCompositorFrameSinkProxy::UnbindProxyClient,
Is this a change in behavior, where this wouldn't have been called previously?

[danakj, 2016-10-10 22:35:25]

https://codereview.chromium.org/2404993002/diff/40001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2404993002/diff/40001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor_frame_sink.cc:66: FROM_HERE,
base::Bind(&BlimpCompositorFrameSinkProxy::UnbindProxyClient,
On 2016/10/10 22:27:10, enne wrote:
> Is this a change in behavior, where this wouldn't have been called previously?

Yeah because it was in a dead branch. But it was meant to be called. So.. I
think this is right. This cleans up stuff in the blimp display compositor, which
is also cleaned up in its destructor, but would have leaked on context
loss/recovry.

[enne (OOO), 2016-10-10 22:38:39]

Ok ok, lgtm!

[boliu, 2016-10-10 22:40:41]

lgtm, although I don't own content/renderer in general

[danakj, 2016-10-10 22:44:22]

On 2016/10/10 22:40:41, boliu wrote:
> lgtm, although I don't own content/renderer in general

ya np, enne does what you don't for this patch.

[commit-bot: I haz the power, 2016-10-10 22:53:25]

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

[commit-bot: I haz the power, 2016-10-10 22:53:26]

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

[Khushal, 2016-10-10 22:55:56]

lgtm.

https://codereview.chromium.org/2404993002/diff/40001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2404993002/diff/40001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor_frame_sink.cc:66: FROM_HERE,
base::Bind(&BlimpCompositorFrameSinkProxy::UnbindProxyClient,
On 2016/10/10 22:35:25, danakj wrote:
> On 2016/10/10 22:27:10, enne wrote:
> > Is this a change in behavior, where this wouldn't have been called
previously?
> 
> Yeah because it was in a dead branch. But it was meant to be called. So.. I
> think this is right. This cleans up stuff in the blimp display compositor,
which
> is also cleaned up in its destructor, but would have leaked on context
> loss/recovry.

Thanks for the cleanup. Now that we call Detach only when the bind is
successful, this simplifies it a lot.

[danakj, 2016-10-10 23:09:08]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-10-10 23:09:08]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
boliu@chromium.org, khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2404993002/#ps60001
(title: "detachinternal: fix-blimp-unittest-send-swapacks")

[commit-bot: I haz the power, 2016-10-10 23:09:24]

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

[commit-bot: I haz the power, 2016-10-10 23:09:26]

This CL has an open dependency (Issue 2402173002 Patch 180001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[danakj, 2016-10-10 23:09:33]

Patchset #4 (id:60001) has been deleted

[danakj, 2016-10-10 23:18:18]

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

[commit-bot: I haz the power, 2016-10-10 23:18:40]

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

[danakj, 2016-10-11 00:26:23]

Description was changed from

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-10-11 00:26:47]

TBR=sky for services/ but please LMK if u have any feedback on the comment
changes esp

[commit-bot: I haz the power, 2016-10-11 00:38:27]

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

[commit-bot: I haz the power, 2016-10-11 00:38:28]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-10-11 00:40:58]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-10-11 00:40:59]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
boliu@chromium.org, khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2404993002/#ps80001
(title: "detachinternal: rebase")

[commit-bot: I haz the power, 2016-10-11 00:41:23]

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

[commit-bot: I haz the power, 2016-10-11 00:48:28]

Description was changed from

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-11 00:48:30]

Committed patchset #4 (id:80001)

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

Description was changed from

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/8fa1509c2848015f60d931090d6da9720a98fec2
Cr-Commit-Position: refs/heads/master@{#424310}
==========

[commit-bot: I haz the power, 2016-10-11 00:51:52]

Patchset 4 (id:??) landed as
https://crrev.com/8fa1509c2848015f60d931090d6da9720a98fec2
Cr-Commit-Position: refs/heads/master@{#424310}

[sky, 2016-10-11 02:07:59]

Description was changed from

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/8fa1509c2848015f60d931090d6da9720a98fec2
Cr-Commit-Position: refs/heads/master@{#424310}
==========

to

==========
cc: Stop calling DetachFromClient if bind failed.

And remove DetachFromClientInternal in the process. This removes any
branching happening in DetachFromClient subclass overrides based on
bind happening or not.

R=enne@chromium.org, khushalsagar@chromium.org, boliu@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/8fa1509c2848015f60d931090d6da9720a98fec2
Cr-Commit-Position: refs/heads/master@{#424310}
==========

[sky, 2016-10-11 02:07:59]

sky@chromium.org changed reviewers:
+ fsamuel@chromium.org
- sky@chromium.org

[sky, 2016-10-11 02:08:14]

sky->fsamuel