Quit the message loop by default in ShellConnectionLost when ApplicationRunner is used

Quit the message loop by default in ShellConnectionLost when ApplicationRunner is used

BUG=

[Ben Goodger (Google), 2016-03-18 14:53:40]

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

[Ben Goodger (Google), 2016-03-18 14:53:41]

This makes shutdown work "automatically" for the vast majority of mojo apps.

[sky, 2016-03-18 16:20:37]

https://codereview.chromium.org/1814223002/diff/40001/mojo/shell/public/cpp/l...
File mojo/shell/public/cpp/lib/application_runner.cc (right):

https://codereview.chromium.org/1814223002/diff/40001/mojo/shell/public/cpp/l...
mojo/shell/public/cpp/lib/application_runner.cc:22:
base::MessageLoop::current()->QuitWhenIdle();
If the messageloop is nested this quits the topmost runloop, which may not be
what you want. Instead create a RunLoop on 69 and call RunUntilIdle on it here.

[Ben Goodger (Google), 2016-03-18 17:38:37]

Maybe I'm misunderstanding what you're saying but what you propose here
doesn't work - apps stay running.

Can you clarify, or can I land this as it's a strict improvement over
what's there?

-Ben

On Fri, Mar 18, 2016 at 9:20 AM, <sky@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1814223002/diff/40001/mojo/shell/public/cpp/l...
> File mojo/shell/public/cpp/lib/application_runner.cc (right):
>
>
>
https://codereview.chromium.org/1814223002/diff/40001/mojo/shell/public/cpp/l...
> mojo/shell/public/cpp/lib/application_runner.cc:22:
> base::MessageLoop::current()->QuitWhenIdle();
> If the messageloop is nested this quits the topmost runloop, which may
> not be what you want. Instead create a RunLoop on 69 and call
> RunUntilIdle on it here.
>
> https://codereview.chromium.org/1814223002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-03-18 17:39:35]

I'll take a look after you land. LGTM

[Ben Goodger (Google), 2016-03-18 17:44:28]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2016-03-18 17:44:29]

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

[commit-bot: I haz the power, 2016-03-18 17:45:05]

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

[commit-bot: I haz the power, 2016-03-18 19:32:44]

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

[commit-bot: I haz the power, 2016-03-18 19:32:45]

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

[Ben Goodger (Google), 2016-03-19 04:25:08]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2016-03-19 04:25:09]

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

[commit-bot: I haz the power, 2016-03-19 04:25:23]

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

[commit-bot: I haz the power, 2016-03-19 05:53:09]

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

[commit-bot: I haz the power, 2016-03-19 05:53:10]

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

[sky, 2016-03-21 19:23:24]

I need this for browser_tests, so I'm going to copy it off to another patch and
land it.

[sky, 2016-03-21 19:55:41]

On 2016/03/21 19:23:24, sky wrote:
> I need this for browser_tests, so I'm going to copy it off to another patch
and
> land it.

The patch I copied is here: https://codereview.chromium.org/1819063002/ .