[win8] Show a prompt before relaunching Chrome in Windows 8 mode while packaged apps are running.

[win8] Show a prompt before relaunching Chrome in Windows 8 mode while packaged apps are running.

Introduces ShellWindowRegistry::IsShellWindowRegisteredInAnyProfile to
help determine if packaged apps are currently running. Intercepts the
IDC_WIN8_METRO_RESTART command and shows a modal dialog box if packaged
apps are running to confirm that the user is OK with their packaged
apps closing. Ensures apps with a relaunch handler are not relaunched
when restarting in metro mode.

BUG=222297
TEST=Launch a packaged app in desktop mode and select 'Relaunch Chrome
in Windows 8 mode' from the hotdog menu. A prompt should appear
confirming that your Chrome apps will close if you continue. If no
packaged apps are running, no prompt should appear.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192240

[tapted, 2013-03-20 05:51:37]

benwells for initial review.

Showing a dialog was initially pulling a lot of dependencies into
browser_command_controller.cc, so I ended up making a new file for it. In
spirit, it's somewhat similar to profile_error_dialog.cc . But then maybe some
extra dependencies are OK, and the PackagedAppsAreRunning() can be ditched (or
moved elsewhere.. or maybe it should be moved elsewhere anyway..). WDYT?

https://codereview.chromium.org/12566039/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12566039/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:15129: +      To relaunch in Windows 8 mode
your Chrome apps will be closed. Continue?
note: This is a "Chrome" in generated_resources (rather than
google_chrome_strings) because it refers to "Chrome apps" - not sure if
"Chromium apps" would make sense here.

[benwells, 2013-03-20 07:06:18]

looks pretty good overall.

https://codereview.chromium.org/12566039/diff/1/chrome/browser/extensions/she...
File chrome/browser/extensions/shell_window_registry.h (right):

https://codereview.chromium.org/12566039/diff/1/chrome/browser/extensions/she...
chrome/browser/extensions/shell_window_registry.h:100: static int
GetShellWindowCountAllProfiles(int window_type_mask);
Should this just return a bool? Seems like things which just want to know if
there are any, not the actual count.

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/apps_running_switch_to_metro_win.cc (right):

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/apps_running_switch_to_metro_win.cc:5: #include
"chrome/browser/ui/extensions/apps_running_switch_to_metro_win.h"
This file name is verbose. Maybe apps_metro_handler_win?

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/apps_running_switch_to_metro_win.cc:16: bool
PackagedAppsAreRunning() {
This doesn't feel right here. Maybe as a static method in ShellWindowRegistry?
Or just use ShellWindowRegistry at the caller?

[koz (OOO until 15th September), 2013-03-20 20:42:15]

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/browser_com...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/browser_com...
chrome/browser/ui/browser_command_controller.cc:91: class SwitchToMetroUIHandler
lol, I made the same mistake you did yesterday when parsing this. I'm thinking
"what changed???"

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/apps_running_switch_to_metro_win.h (right):

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/apps_running_switch_to_metro_win.h:16: // closing,
in order to relaunch to metro mode.
nit: += "Returns true if the user clicks OK."

[tapted, 2013-03-21 10:31:14]

(sorry there's a tiny bit of rebase mixed in.)

https://codereview.chromium.org/12566039/diff/1/chrome/browser/extensions/she...
File chrome/browser/extensions/shell_window_registry.h (right):

https://codereview.chromium.org/12566039/diff/1/chrome/browser/extensions/she...
chrome/browser/extensions/shell_window_registry.h:100: static int
GetShellWindowCountAllProfiles(int window_type_mask);
On 2013/03/20 07:06:18, benwells wrote:
> Should this just return a bool? Seems like things which just want to know if
> there are any, not the actual count.

Done.

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/apps_running_switch_to_metro_win.cc (right):

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/apps_running_switch_to_metro_win.cc:5: #include
"chrome/browser/ui/extensions/apps_running_switch_to_metro_win.h"
On 2013/03/20 07:06:18, benwells wrote:
> This file name is verbose. Maybe apps_metro_handler_win?

Done.

Also, random thought - WDYT about moving the info bar delegate into this file as
well?

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/apps_running_switch_to_metro_win.cc:16: bool
PackagedAppsAreRunning() {
On 2013/03/20 07:06:18, benwells wrote:
> This doesn't feel right here. Maybe as a static method in ShellWindowRegistry?
> Or just use ShellWindowRegistry at the caller?

Done. But see what you think.. I was trying to avoid pulling too much stuff into
browser_command_controller.cc ...

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/apps_running_switch_to_metro_win.h (right):

https://codereview.chromium.org/12566039/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/apps_running_switch_to_metro_win.h:16: // closing,
in order to relaunch to metro mode.
On 2013/03/20 20:42:15, koz wrote:
> nit: += "Returns true if the user clicks OK."

Done. .. although there is no OK/Cancel message box in Chrome yet. Hopefully
UI-review are "OK" with yes/no.

[benwells, 2013-03-21 23:44:06]

lgtm with dud function decl reoved.

i would slightly prefer to have a ShellWindowRegistry function (or chrome::
function in shell_window_registry) for PackagedAppsAreRunning. The function was
fine before, it just didn't feel right where it was.

https://codereview.chromium.org/12566039/diff/10001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/apps_metro_handler_win.h (right):

https://codereview.chromium.org/12566039/diff/10001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/apps_metro_handler_win.h:13: bool
PackagedAppsAreRunning();
This isn't used now.

[tapted, 2013-03-22 06:15:56]

koz/benwells could you take another look. It didn't feel right having logic for
whether apps should be restored in startup_browser_creator_impl -- felt like
something AppsRestoreService should be able to do ;). WDYT?

Note that the DEPS change is also in http://crrev.com/12450014

https://codereview.chromium.org/12566039/diff/10001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/apps_metro_handler_win.h (right):

https://codereview.chromium.org/12566039/diff/10001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/apps_metro_handler_win.h:13: bool
PackagedAppsAreRunning();
On 2013/03/21 23:44:06, benwells wrote:
> This isn't used now.

Done.

[benwells, 2013-03-22 06:42:43]

Slgtm

[benwells, 2013-03-22 06:43:57]

Slgtm

[benwells, 2013-03-22 06:44:49]

Slgtm

[koz_google.com, 2013-03-22 06:46:07]

I'm not a big fan of this logic being here - it makes this class hard to
test as now you can't hit all the flows through here without controlling
the platform you're on and the return value of
win8::IsSingleWindowMetroMode().

How about adding a static function that we can give |is_browser_restart|
and it will tell us |should_restore_apps|? The policy we have for when apps
should be restarted is static (and platform dependent) and needs have
nothing to do with a running instance of AppRestoreService.

James


On Fri, Mar 22, 2013 at 5:15 PM, <[email protected]> wrote:

> koz/benwells could you take another look. It didn't feel right having
> logic for
> whether apps should be restored in startup_browser_creator_impl -- felt
> like
> something AppsRestoreService should be able to do ;). WDYT?
>
> Note that the DEPS change is also in http://crrev.com/12450014
>
>
>
> https://codereview.chromium.**org/12566039/diff/10001/**
>
chrome/browser/ui/extensions/**apps_metro_handler_win.h<https://codereview.chromium.org/12566039/diff/10001/chrome/browser/ui/extensions/apps_metro_handler_win.h>
> File chrome/browser/ui/extensions/**apps_metro_handler_win.h (right):
>
> https://codereview.chromium.**org/12566039/diff/10001/**
>
chrome/browser/ui/extensions/**apps_metro_handler_win.h#**newcode13<https://codereview.chromium.org/12566039/diff/10001/chrome/browser/ui/extensions/apps_metro_handler_win.h#newcode13>
> chrome/browser/ui/extensions/**apps_metro_handler_win.h:13: bool
> PackagedAppsAreRunning();
> On 2013/03/21 23:44:06, benwells wrote:
>
>> This isn't used now.
>>
>
> Done.
>
>
https://codereview.chromium.**org/12566039/<https://codereview.chromium.org/1...
>

[tapted, 2013-03-22 09:23:03]

On 2013/03/22 06:46:07, koz_google.com wrote:
> I'm not a big fan of this logic being here - it makes this class hard to
> test as now you can't hit all the flows through here without controlling
> the platform you're on and the return value of
> win8::IsSingleWindowMetroMode().
> 
> How about adding a static function that we can give |is_browser_restart|
> and it will tell us |should_restore_apps|? The policy we have for when apps
> should be restarted is static (and platform dependent) and needs have
> nothing to do with a running instance of AppRestoreService.

You make an excellent point. tryjobs might have told me off if they were
functioning :/

Let me know what you think of patchset 4.

[tapted, 2013-03-28 03:09:17]

+sky for owners in c/b/ui

benwells: UI review were OK with the modal dialog, but wanted a cancel button
rather than Yes/No, so I've added that option to simple_message_box.h for views
and win. PTAL. After our chat, I've also added tests as friends, rather than
exposing that function in AppsRestoreService, but I'm not sure which is nicer.
WDYT?

For the message box, other platforms have a sample implementation in patch set
6, but I didn't want to add unused code, so I switched them to call
NOTIMPLEMENTED() in patch set 7.

[benwells, 2013-03-28 03:38:58]

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.cc:456:
window()->GetNativeWindow())) {
Can we put this whole block into apps_metro_handler_win? i.e. a function like
chrome::CheckAppsForSwitchToMetro(window) which handles the check and the
prompt? Feels a bit yucky like this.

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/apps_metro_handler_win.h (right):

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/apps_metro_handler_win.h:14: // Yes.
nitty nit: Yes -> OK.

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/simple_message_box_win.cc (right):

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/views/s...
chrome/browser/ui/views/simple_message_box_win.cc:33: return (result == IDNO ||
result == IDCANCEL) ?
Maybe (up to you) it would be clearer to use positive test / result. i.e. if yes
or ok return yes, otherwise no.

[tapted, 2013-03-28 06:21:20]

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.cc:456:
window()->GetNativeWindow())) {
On 2013/03/28 03:38:58, benwells wrote:
> Can we put this whole block into apps_metro_handler_win? i.e. a function like
> chrome::CheckAppsForSwitchToMetro(window) which handles the check and the
> prompt? Feels a bit yucky like this.

Done.

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/apps_metro_handler_win.h (right):

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/apps_metro_handler_win.h:14: // Yes.
On 2013/03/28 03:38:58, benwells wrote:
> nitty nit: Yes -> OK.

Done. Also updated to say the new function only prompts when apps are running
otherwise just returns true.

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/views/s...
File chrome/browser/ui/views/simple_message_box_win.cc (right):

https://codereview.chromium.org/12566039/diff/79001/chrome/browser/ui/views/s...
chrome/browser/ui/views/simple_message_box_win.cc:33: return (result == IDNO ||
result == IDCANCEL) ?
On 2013/03/28 03:38:58, benwells wrote:
> Maybe (up to you) it would be clearer to use positive test / result. i.e. if
yes
> or ok return yes, otherwise no.

Yeah, I suppose the previous way was like that so that it didn't have to name
'OK' in the check, but that argument no longer holds.

Done.

[koz (OOO until 15th September), 2013-04-01 15:56:12]

https://codereview.chromium.org/12566039/diff/86001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/12566039/diff/86001/apps/app_restore_service....
apps/app_restore_service.cc:56: PerformStartupTasks(should_restore_apps);
What I had in mind was that AppRestoreService still exposes HandleStartup()
publicly, and callers can figure out for themselves whether they want to restore
apps or not. The function you have here would be a static and public member of
AppRestoreService, so clients might call it like this:

AppRestoreService* restore_service = /*...*/;
restore_service->HandleStartup(AppRestoreService::ShouldRestoreApps(is_browser_restart));

Requiring friend classes to test is a code smell, IMHO, and besides, it's
AppRestoreService's responsibility to know how to restore apps, not when -
that's what its public interface is for.

I'd rather this functionality not go in AppRestoreService at all, as this
defines Chrome's policy for when to restart apps, which has nothing to do with
the act of actually restoring apps. An analogous situation is figuring out
startup URLs. Chrome startup policy determines which URLs we need to launch and
other code launches them. I think it's ok to delegate this logic to a static
helper function within AppRestoreService, but really it's the call site's
responsibility and so probably makes more sense there.

[tapted, 2013-04-02 11:38:30]

On 2013/03/28 03:09:17, tapted wrote:
> UI review were OK with the modal dialog, but wanted a cancel button
> rather than Yes/No, so I've added that option to simple_message_box.h for
views
> and win.

(replying to myself..) Another option is something like
update_recommended_message_box.{h,cc} [1]. Feels like overkill for this though.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/12566039/diff/86001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/12566039/diff/86001/apps/app_restore_service....
apps/app_restore_service.cc:56: PerformStartupTasks(should_restore_apps);
On 2013/04/01 15:56:12, koz wrote:
> What I had in mind was that AppRestoreService still exposes HandleStartup()
> publicly, and callers can figure out for themselves whether they want to
restore
> apps or not. The function you have here would be a static and public member of
> AppRestoreService, so clients might call it like this:
> 
> AppRestoreService* restore_service = /*...*/;
>
restore_service->HandleStartup(AppRestoreService::ShouldRestoreApps(is_browser_restart));

Done.

> Requiring friend classes to test is a code smell, IMHO, and besides, it's
> AppRestoreService's responsibility to know how to restore apps, not when -
> that's what its public interface is for.

Yeah - I didn't really like the friends either. I guess the rationale for the
encapsulation was that attempting to restore apps in Metro mode is essentially
an error. extensions::LaunchPlatformApp(..) checks for Metro and balks, but the
extensions::RestartPlatformAppWithFileEntries(..) used here does not. Making
PerformStartupTasks private avoided exposing that error state to the public
interface of AppRestoreService. But that's getting academic.

> I'd rather this functionality not go in AppRestoreService at all, as this
> defines Chrome's policy for when to restart apps, which has nothing to do with
> the act of actually restoring apps. An analogous situation is figuring out
> startup URLs. Chrome startup policy determines which URLs we need to launch
and
> other code launches them. I think it's ok to delegate this logic to a static
> helper function within AppRestoreService, but really it's the call site's
> responsibility and so probably makes more sense there.

I'm working on it ;). I'm going to follow up in http://crrev.com/12545064 to
rein in all the "stuff apps need to do during startup" and make it more
cohesive. But it's a big refactor to decouple apps and a few other things from
startup_browser_creator* and make it clear, logical and testable.

E.g. AppsRestoreService::HandleStartup(..) is (and was) called from
StartupBrowserCreatorImpl::ProcessStartupURLs(..) which sounds quirky, and will
be something that I can address in the refactor.

[koz (OOO until 15th September), 2013-04-02 20:16:52]

lgtm

https://codereview.chromium.org/12566039/diff/86001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/12566039/diff/86001/apps/app_restore_service....
apps/app_restore_service.cc:56: PerformStartupTasks(should_restore_apps);
On 2013/04/02 11:38:30, tapted wrote:
> On 2013/04/01 15:56:12, koz wrote:
> > What I had in mind was that AppRestoreService still exposes HandleStartup()
> > publicly, and callers can figure out for themselves whether they want to
> restore
> > apps or not. The function you have here would be a static and public member
of
> > AppRestoreService, so clients might call it like this:
> > 
> > AppRestoreService* restore_service = /*...*/;
> >
>
restore_service->HandleStartup(AppRestoreService::ShouldRestoreApps(is_browser_restart));
> 
> Done.
> 
> > Requiring friend classes to test is a code smell, IMHO, and besides, it's
> > AppRestoreService's responsibility to know how to restore apps, not when -
> > that's what its public interface is for.
> 
> Yeah - I didn't really like the friends either. I guess the rationale for the
> encapsulation was that attempting to restore apps in Metro mode is essentially
> an error. extensions::LaunchPlatformApp(..) checks for Metro and balks, but
the
> extensions::RestartPlatformAppWithFileEntries(..) used here does not. Making
> PerformStartupTasks private avoided exposing that error state to the public
> interface of AppRestoreService. But that's getting academic.

Yeah, I see what you mean.

> 
> > I'd rather this functionality not go in AppRestoreService at all, as this
> > defines Chrome's policy for when to restart apps, which has nothing to do
with
> > the act of actually restoring apps. An analogous situation is figuring out
> > startup URLs. Chrome startup policy determines which URLs we need to launch
> and
> > other code launches them. I think it's ok to delegate this logic to a static
> > helper function within AppRestoreService, but really it's the call site's
> > responsibility and so probably makes more sense there.
> 
> I'm working on it ;). I'm going to follow up in http://crrev.com/12545064 to
> rein in all the "stuff apps need to do during startup" and make it more
> cohesive. But it's a big refactor to decouple apps and a few other things from
> startup_browser_creator* and make it clear, logical and testable.
> 
> E.g. AppsRestoreService::HandleStartup(..) is (and was) called from
> StartupBrowserCreatorImpl::ProcessStartupURLs(..) which sounds quirky, and
will
> be something that I can address in the refactor.

Awesome! Death to crazy messy startup logic :-)

[tapted, 2013-04-03 09:09:58]

sky, can I trouble you for OWNERS in:

- c/b/ui/browser_command_controller.cc 
- c/b/ui/simple_message_box.h
- c/b/ui/startup/startup_browser_creator_impl.cc
- c/b/ui/views/simple_message_box_views.cc
- c/b/ui/views/simple_message_box_win.cc

and also small changes to

- c/b/ui/cocoa/simple_message_box_mac.mm
- c/b/ui/gtk/simple_message_box_gtk.cc
- chrome/chrome_browser_ui.gypi

[sky, 2013-04-03 13:46:30]

https://codereview.chromium.org/12566039/diff/100001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/12566039/diff/100001/chrome/browser/ui/browse...
chrome/browser/ui/browser_command_controller.cc:454: new SwitchToMetroUIHandler;
Document who owns this.

https://codereview.chromium.org/12566039/diff/100001/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):

https://codereview.chromium.org/12566039/diff/100001/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:649:
should_restore_apps = true;
Where is the chromeos specific bits handled now?

[tapted, 2013-04-03 22:03:23]

https://codereview.chromium.org/12566039/diff/100001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/12566039/diff/100001/chrome/browser/ui/browse...
chrome/browser/ui/browser_command_controller.cc:454: new SwitchToMetroUIHandler;
On 2013/04/03 13:46:31, sky wrote:
> Document who owns this.

Done.

https://codereview.chromium.org/12566039/diff/100001/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):

https://codereview.chromium.org/12566039/diff/100001/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:649:
should_restore_apps = true;
On 2013/04/03 13:46:31, sky wrote:
> Where is the chromeos specific bits handled now?

This moved to the static function in AppRestoreService, at
https://codereview.chromium.org/12566039/diff/100001/apps/app_restore_service...

[sky, 2013-04-03 23:12:19]

LGTM

[commit-bot: I haz the power, 2013-04-04 01:19:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12566039/108001

[commit-bot: I haz the power, 2013-04-04 08:32:43]

Change committed as 192240