services/shell: Fix running the connection-lost callback.

services/shell: Fix running the connection-lost callback.

The connection may have already been lost by the time the callback is set.
Run the callback synchronously in such cases.

BUG=594600
Committed: https://crrev.com/d9862587e3ab64d4065b61d0d2a0ca8ad21b8f05
Cr-Commit-Position: refs/heads/master@{#390249}

[sadrul, 2016-04-27 20:00:37]

sadrul@chromium.org changed reviewers:
+ rockot@chromium.org, sky@chromium.org

[sadrul, 2016-04-27 20:00:38]

https://codereview.chromium.org/1920033005/diff/1/content/common/mojo/mojo_sh...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/1920033005/diff/1/content/common/mojo/mojo_sh...
content/common/mojo/mojo_shell_connection_impl.cc:134:
shell_connection_->set_connection_lost_closure(closure);
Alternatively, ShellConnection::set_connection_lost_closure could become
SetConnectionLostClosure() and do this instead.

[sky, 2016-04-27 21:18:55]

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

[sky, 2016-04-27 21:18:56]

+ben

https://codereview.chromium.org/1920033005/diff/1/content/common/mojo/mojo_sh...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/1920033005/diff/1/content/common/mojo/mojo_sh...
content/common/mojo/mojo_shell_connection_impl.cc:134:
shell_connection_->set_connection_lost_closure(closure);
On 2016/04/27 20:00:38, sadrul wrote:
> Alternatively, ShellConnection::set_connection_lost_closure could become
> SetConnectionLostClosure() and do this instead.

I would add this to ShellConnection as it seems easy to miss and I suspect
others will want this behavior too.

[Ben Goodger (Google), 2016-04-27 21:21:55]

On 2016/04/27 21:18:56, sky wrote:
> +ben
> 
>
https://codereview.chromium.org/1920033005/diff/1/content/common/mojo/mojo_sh...
> File content/common/mojo/mojo_shell_connection_impl.cc (right):
> 
>
https://codereview.chromium.org/1920033005/diff/1/content/common/mojo/mojo_sh...
> content/common/mojo/mojo_shell_connection_impl.cc:134:
> shell_connection_->set_connection_lost_closure(closure);
> On 2016/04/27 20:00:38, sadrul wrote:
> > Alternatively, ShellConnection::set_connection_lost_closure could become
> > SetConnectionLostClosure() and do this instead.
> 
> I would add this to ShellConnection as it seems easy to miss and I suspect
> others will want this behavior too.

+1

[sadrul, 2016-04-27 21:57:52]

Description was changed from

==========
mojo/content: Fix running the connection-lost callback.

The connection may have already been lost by the time the callback is set.
Run the callback synchronously in such cases.

BUG=594600
==========

to

==========
services/shell: Fix running the connection-lost callback.

The connection may have already been lost by the time the callback is set.
Run the callback synchronously in such cases.

BUG=594600
==========

[sadrul, 2016-04-27 22:11:19]

https://codereview.chromium.org/1920033005/diff/1/content/common/mojo/mojo_sh...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/1920033005/diff/1/content/common/mojo/mojo_sh...
content/common/mojo/mojo_shell_connection_impl.cc:134:
shell_connection_->set_connection_lost_closure(closure);
On 2016/04/27 21:18:56, sky wrote:
> On 2016/04/27 20:00:38, sadrul wrote:
> > Alternatively, ShellConnection::set_connection_lost_closure could become
> > SetConnectionLostClosure() and do this instead.
> 
> I would add this to ShellConnection as it seems easy to miss and I suspect
> others will want this behavior too.

Done.

[Ben Goodger (Google), 2016-04-27 22:29:20]

services/shell lgtm with nits

https://codereview.chromium.org/1920033005/diff/40001/services/shell/public/c...
File services/shell/public/cpp/lib/shell_connection.cc (right):

https://codereview.chromium.org/1920033005/diff/40001/services/shell/public/c...
services/shell/public/cpp/lib/shell_connection.cc:27:
should_run_connection_lost_closure_(false) {
init in the header

https://codereview.chromium.org/1920033005/diff/40001/services/shell/public/c...
File services/shell/public/cpp/shell_connection.h (right):

https://codereview.chromium.org/1920033005/diff/40001/services/shell/public/c...
services/shell/public/cpp/shell_connection.h:98: bool
should_run_connection_lost_closure_;
= false;

[sadrul, 2016-04-27 22:45:11]

https://codereview.chromium.org/1920033005/diff/40001/services/shell/public/c...
File services/shell/public/cpp/lib/shell_connection.cc (right):

https://codereview.chromium.org/1920033005/diff/40001/services/shell/public/c...
services/shell/public/cpp/lib/shell_connection.cc:27:
should_run_connection_lost_closure_(false) {
On 2016/04/27 22:29:20, Ben Goodger (Google) wrote:
> init in the header

Done.

https://codereview.chromium.org/1920033005/diff/40001/services/shell/public/c...
File services/shell/public/cpp/shell_connection.h (right):

https://codereview.chromium.org/1920033005/diff/40001/services/shell/public/c...
services/shell/public/cpp/shell_connection.h:98: bool
should_run_connection_lost_closure_;
On 2016/04/27 22:29:20, Ben Goodger (Google) wrote:
> = false;

Done.

[sadrul, 2016-04-27 23:54:49]

A review on the change in content/common/mojo/embedded_application_runner.cc
before I put this in CQ?

[Ken Rockot(use gerrit already), 2016-04-27 23:57:00]

lgtm as non-owner - ben can stamp

[sadrul, 2016-04-28 00:00:14]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-04-28 00:00:14]

The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1920033005/#ps60001
(title: ".")

[commit-bot: I haz the power, 2016-04-28 00:01:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1920033005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1920033005/60001

[commit-bot: I haz the power, 2016-04-28 00:07:17]

Description was changed from

==========
services/shell: Fix running the connection-lost callback.

The connection may have already been lost by the time the callback is set.
Run the callback synchronously in such cases.

BUG=594600
==========

to

==========
services/shell: Fix running the connection-lost callback.

The connection may have already been lost by the time the callback is set.
Run the callback synchronously in such cases.

BUG=594600
==========

[commit-bot: I haz the power, 2016-04-28 00:07:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-30 17:14:51]

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

[commit-bot: I haz the power, 2016-04-30 17:53:12]

Description was changed from

==========
services/shell: Fix running the connection-lost callback.

The connection may have already been lost by the time the callback is set.
Run the callback synchronously in such cases.

BUG=594600
==========

to

==========
services/shell: Fix running the connection-lost callback.

The connection may have already been lost by the time the callback is set.
Run the callback synchronously in such cases.

BUG=594600

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