Show an InfoBar when trying to start Packaged Apps from Metro mode.

Show an InfoBar when trying to start Packaged Apps from Metro mode.

Platform Apps do not function properly while in Metro mode.

This change intercepts platform app launch requests done
while the browser is in metro mode, redirecting them to an
infobar. If "yes" is chosen, a local pref is stored with the
extension id and Profile that tried to launch it, and a relaunch
of Chrome in Desktop mode is triggered. Upon restart,
apps::AppLaunchOnRestartService is responsible for launching the
selected app, and clearing the pref.

BUG=153426
TEST=With Chrome configured for Windows 8 mode, try to start a
packaged app. This can be done from NTP (if launcher not enabled),
or from a taskbar shortcut, a desktop shortcut, or a pinned start
page tile. InfoBar should display offering to switch to Desktop mode.
If switching, selected packaged app should launch after a brief
delay.

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

[tapted, 2013-03-15 00:31:21]

Hello Mr Wells, please take a look and let me know what you think.

This is the design I settled on, but there are earlier versions.

Patchset 1: throws everything in with app_list_controller...
Patchset 2: no delay, just launch (and get hidden by the browser)
Patchset 3: as described.

https://codereview.chromium.org/12450014/diff/6001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/app_metro_infobar_delegate_win.h (left):

https://codereview.chromium.org/12450014/diff/6001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.h:50: }  //
namespace chrome
jeepers! - my bad from an earlier patch :(. Bad enough to convince me to see
about updating that presubmit check..

[benwells, 2013-03-15 00:39:04]

Overall approach lg. Have not done a detailed review yet, my main question is
about where to put the code.

https://codereview.chromium.org/12450014/diff/6001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/12450014/diff/6001/apps/app_restore_service.c...
apps/app_restore_service.cc:118: void AppRestoreService::LaunchAppWithId(const
std::string& extension_id) {
I would prefer all this in a different PKS dedicated to this purpose. You could
then make it _win.* instead of #if.

https://codereview.chromium.org/12450014/diff/6001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc (right):

https://codereview.chromium.org/12450014/diff/6001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc:77:
CHECK_EQ(mode_ == SHOW_APP_LIST, extension_id_.empty());
Why CHECK not DCHECK?

https://codereview.chromium.org/12450014/diff/6001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc:89:
IDS_WIN8_INFOBAR_DESKTOP_RESTART_FOR_APP_LIST :
indenting

[tapted, 2013-03-15 00:51:16]

On 2013/03/15 00:39:04, benwells wrote:
> my main question is about where to put the code.

I had that question too ;).

https://codereview.chromium.org/12450014/diff/6001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/12450014/diff/6001/apps/app_restore_service.c...
apps/app_restore_service.cc:118: void AppRestoreService::LaunchAppWithId(const
std::string& extension_id) {
On 2013/03/15 00:39:04, benwells wrote:
> I would prefer all this in a different PKS dedicated to this purpose. You
could
> then make it _win.* instead of #if.

I'll try that out.

Although I think the #if is still necessary, except it would go in
StartupBrowserCreatorImpl::ProcessStartupURLs, with some extra logic just below
where AppRestoreService currently is.

https://codereview.chromium.org/12450014/diff/6001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc (right):

https://codereview.chromium.org/12450014/diff/6001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc:77:
CHECK_EQ(mode_ == SHOW_APP_LIST, extension_id_.empty());
On 2013/03/15 00:39:04, benwells wrote:
> Why CHECK not DCHECK?

It has no coverage in debug mode.. (cpu@ earlier suggested this approach for ui
stuff).

https://codereview.chromium.org/12450014/diff/6001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc:89:
IDS_WIN8_INFOBAR_DESKTOP_RESTART_FOR_APP_LIST :
On 2013/03/15 00:39:04, benwells wrote:
> indenting

Done.

[tapted, 2013-03-15 07:04:42]

This [patchset 5] is the approach I'm going to try, to handle the multi-profile
case. (But..er.. it doesn't work yet).

[tapted, 2013-03-18 06:27:14]

Hi Ben, patchset 6 is ready for review. Multi-profile cases all check out. PTAL.

[tapted, 2013-03-18 06:54:31]

Code now has a little bit more necessary paranoia.

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
File apps/app_launch_on_restart_service_win.cc (right):

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
apps/app_launch_on_restart_service_win.cc:74: if
(win8::IsSingleWindowMetroMode()) {
IsMetroProcess from base/win/metro.h is a feasible alternative here, since this
code should never run in ash either. That would remove the DEPS change, but I
think base/win/metro.h might be a temporary home anyway.

[benwells, 2013-03-18 22:16:47]

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
File apps/app_launch_on_restart_service_win.cc (right):

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
apps/app_launch_on_restart_service_win.cc:32:
g_browser_process->profile_manager()->GetProfile(profile_dir);
Why not just send the profile to this function?

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
apps/app_launch_on_restart_service_win.cc:59: if (profile_dir.empty() ||
profile->GetPath().BaseName() != profile_dir)
Could you add a comment why we need the profiles to match? It isn't clear to me.

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
File apps/app_launch_on_restart_service_win.h (right):

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
apps/app_launch_on_restart_service_win.h:18: class AppLaunchOnRestartServiceWin
{
Nit: Please rename this the metro restart service or something metro specific.
Also, is there any reason this is a class? It could equally by a namespace.

https://codereview.chromium.org/12450014/diff/40001/apps/pref_names.h
File apps/pref_names.h (right):

https://codereview.chromium.org/12450014/diff/40001/apps/pref_names.h#newcode14
apps/pref_names.h:14: extern const char kRestartFromMetroWithAppLaunch[];
Can you use one pref which is a dictionary?

Like: RestartAppFromMetro.ProfileDir and RestartAppFromMetro.AppID

https://codereview.chromium.org/12450014/diff/40001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/app_metro_infobar_delegate_win.h (right):

https://codereview.chromium.org/12450014/diff/40001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.h:29: static void
CreateAndActivateMetroForAppList(Profile* profile);
This is a  mouthful. Is the CreateAndActivateMetro some pattern for creating
metro / touch infobars?

[tapted, 2013-03-19 02:49:05]

I found another mistake, but after doing some more tracing I think this handles
all the multi-profile cases now. (Note that incognito windows are not included
on a relaunch.)

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
File apps/app_launch_on_restart_service_win.cc (right):

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
apps/app_launch_on_restart_service_win.cc:32:
g_browser_process->profile_manager()->GetProfile(profile_dir);
On 2013/03/18 22:16:47, benwells wrote:
> Why not just send the profile to this function?

Yeah, this might be overly paranoid. I wasn't sure how safe it is to pass around
raw pointers to delayed tasks during startup. E.g. this example in
chrome_browser_main.cc uses a string:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

But there are a few examples of passing around the pointer too (e.g.
app_list_controller_win ;). I'll change it.

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
apps/app_launch_on_restart_service_win.cc:59: if (profile_dir.empty() ||
profile->GetPath().BaseName() != profile_dir)
On 2013/03/18 22:16:47, benwells wrote:
> Could you add a comment why we need the profiles to match? It isn't clear to
me.

Done.

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
File apps/app_launch_on_restart_service_win.h (right):

https://codereview.chromium.org/12450014/diff/40001/apps/app_launch_on_restar...
apps/app_launch_on_restart_service_win.h:18: class AppLaunchOnRestartServiceWin
{
On 2013/03/18 22:16:47, benwells wrote:
> Nit: Please rename this the metro restart service or something metro specific.

Done.

> Also, is there any reason this is a class? It could equally by a namespace.

class-style felt a bit more 'natural' for grouping the functions together, and
matching up the filename + classname. But I'm not really a fan of the
creating-a-class-just-with-static-functions model either. I'll try out a
namespace to see how it looks.

https://codereview.chromium.org/12450014/diff/40001/apps/pref_names.h
File apps/pref_names.h (right):

https://codereview.chromium.org/12450014/diff/40001/apps/pref_names.h#newcode14
apps/pref_names.h:14: extern const char kRestartFromMetroWithAppLaunch[];
On 2013/03/18 22:16:47, benwells wrote:
> Can you use one pref which is a dictionary?
> 
> Like: RestartAppFromMetro.ProfileDir and RestartAppFromMetro.AppID

Yeah, I thought of doing that, but realised it would add a lot more code...
edit: And then I started doing it and turned around to complain to you :P

Now, with better names (I think? Consistent, at least - WDYT?).

https://codereview.chromium.org/12450014/diff/40001/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/app_metro_infobar_delegate_win.h (right):

https://codereview.chromium.org/12450014/diff/40001/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.h:29: static void
CreateAndActivateMetroForAppList(Profile* profile);
On 2013/03/18 22:16:47, benwells wrote:
> This is a  mouthful. Is the CreateAndActivateMetro some pattern for creating
> metro / touch infobars?

Most info bars just have a static 'Create()'. This one has a significant
side-effect that it also activates metro mode Chrome. And it was originally
called just 'Activate()' but didn't have the constructory feel in review at
http://crrev.com/12385013

https://codereview.chromium.org/12450014/diff/40001/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/12450014/diff/40001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:636:
apps::AppLaunchOnRestartServiceWin::HandleStartupForProfile(profile_);
This function is only called if process_startup is true, so it was actually in
the wrong place :(

https://codereview.chromium.org/12450014/diff/49019/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/12450014/diff/49019/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:562: if
(base::win::GetVersion() >= base::win::VERSION_WIN8) {
I added this -- it's mostly an aesthetic change, but after adding a better
comment it seemed silly not to have it.

[benwells, 2013-03-20 06:54:50]

lgtm

[tapted, 2013-03-20 07:03:48]

+sky for OWNERS in c/b/ui/ [ startup/startup_browser_creator_impl.cc ]

link:
https://codereview.chromium.org/12450014/diff/61001/chrome/browser/ui/startup...

[sky, 2013-03-20 14:30:00]

https://codereview.chromium.org/12450014/diff/61001/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/12450014/diff/61001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:562: if
(base::win::GetVersion() >= base::win::VERSION_WIN8) {
Is it possible to move this into startup_browser_creator_win?

[tapted, 2013-03-21 09:47:59]

On 2013/03/20 14:30:00, sky wrote:
>
https://codereview.chromium.org/12450014/diff/61001/chrome/browser/ui/startup...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
> 
>
https://codereview.chromium.org/12450014/diff/61001/chrome/browser/ui/startup...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:562: if
> (base::win::GetVersion() >= base::win::VERSION_WIN8) {
> Is it possible to move this into startup_browser_creator_win?

I investigated this pretty thoroughly. startup_browser_creator_win isn't really
ready for this kind of flexibility. The way it is currently used is pretty close
to this change. E.g., in a different file, at [1]. You can see my investigation
to add the required flexibility in patch set 11 -- it's a pretty big refactor
(removes some dead code I discovered too). However, I think it's orthogonal to
this change. I'm happy to pursue it in a follow-up (I've added a TODO, and will
move patchset 11 to a new CL).

WDYT?


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

[sky, 2013-03-21 15:34:02]

LGTM

[commit-bot: I haz the power, 2013-03-21 22:14:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/12450014/90001

[Peter Kasting, 2013-03-21 22:26:06]

Please run all infobar-related reviews by me.

Some problems here:

* Making the infobar delegate destructor public
* Multiple creation functions that just pass through to an internal one, instead
of simply publicizing that one directly
* The create function is not called "Create()" like with all other infobars
* Adding logging (we eschew logging because it bloats the binary and we rarely
have the means to collect the log data, this should usually be either some kind
of DCHECK or simply handle the case if it is supposed to be possible)
* Existing code uses CHECK (should only be for security issues and tracking down
bugs, otherwise use DCHECK)

The UI in general of opening a blank tab to show an infobar also seems highly
questionable.  Why aren't we using some better UI like a dialog box?

[tfarina, 2013-03-21 23:02:12]

On Thu, Mar 21, 2013 at 7:26 PM,  <pkasting@chromium.org> wrote:
> Please run all infobar-related reviews by me.
>
Peter, as suggested elsewhere, I think by Ilya, the best way to
accomplish this is to set up a watchlist for all infobars files in
//WATCHLISTS

-- 
Thiago

[tapted, 2013-03-21 23:33:45]

Thanks for catching this. PTAL.

On 2013/03/21 22:26:06, Peter Kasting wrote:
> Please run all infobar-related reviews by me.
> 
> Some problems here:
> 
> * Making the infobar delegate destructor public

Done.

> * Multiple creation functions that just pass through to an internal one,
instead
> of simply publicizing that one directly

Fixed.

> * The create function is not called "Create()" like with all other infobars

Done.

> * Adding logging (we eschew logging because it bloats the binary and we rarely
> have the means to collect the log data, this should usually be either some
kind
> of DCHECK or simply handle the case if it is supposed to be possible)
> * Existing code uses CHECK (should only be for security issues and tracking
down
> bugs, otherwise use DCHECK)

Both Fixed. Changed to NOTREACHED(); return;
CHECK_EQ -> DCHECK_EQ

> The UI in general of opening a blank tab to show an infobar also seems highly
> questionable.  Why aren't we using some better UI like a dialog box?

This approach was run by Glen -- see http://crbug.com/165383#c15 . The very
first
version to handle this problem (before that) was simply to rip Chrome out of
metro
mode, but it didn't pass review, and had other problems besides (see below).

One advantage of the infobar approach is that it gives the user the opportunity
to clean up anything they are doing in other tabs before the process relaunch
required for taking Chrome out of Metro mode. And, if Chrome is configured for
Metro mode but the user is in Windows Desktop mode, there is no way for Chrome
to show any dialog without activating itself in Metro mode (or going behind the
user to reconfigure Chrome for desktop mode).

Metro mode also has the problem that any non-modal dialogs are easy to lose --
there is no way to alt-tab to them once they lose focus there.

Another complication is that a Chrome app can be launched either from the new
tab page, or a desktop shortcut, or a pinned taskbar shortcut, or a start page
tile. This is the reason for the new, blank tab. The added context also means
that the user doesn't have to find their app again from wherever they tired to
launch it (e.g. after cleaning up other tabs).

[Peter Kasting, 2013-03-22 19:48:21]

Thanks, infobar changes LGTM.

https://codereview.chromium.org/12450014/diff/104001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc (right):

https://codereview.chromium.org/12450014/diff/104001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc:33: if
(!win8::IsSingleWindowMetroMode() ||
Nit: Simpler, more similar to constructor, and complies with the "don't handle
DCHECK failure" style guide rule:

  DCHECK(win8::IsSingleWindowMetroMode());
  DCHECK_EQ(mode_ == SHOW_APP_LIST, extension_id_.empty());

https://codereview.chromium.org/12450014/diff/104001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc:92: if (button ==
BUTTON_CANCEL) {
Nit: This function can be collapsed by using ?: the way you did in
GetMessageText().

[tapted, 2013-03-24 22:02:46]

https://codereview.chromium.org/12450014/diff/104001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc (right):

https://codereview.chromium.org/12450014/diff/104001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc:33: if
(!win8::IsSingleWindowMetroMode() ||
On 2013/03/22 19:48:21, Peter Kasting wrote:
> Nit: Simpler, more similar to constructor, and complies with the "don't handle
> DCHECK failure" style guide rule:
> 
>   DCHECK(win8::IsSingleWindowMetroMode());
>   DCHECK_EQ(mode_ == SHOW_APP_LIST, extension_id_.empty());

Done.

https://codereview.chromium.org/12450014/diff/104001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/app_metro_infobar_delegate_win.cc:92: if (button ==
BUTTON_CANCEL) {
On 2013/03/22 19:48:21, Peter Kasting wrote:
> Nit: This function can be collapsed by using ?: the way you did in
> GetMessageText().

Done.

[commit-bot: I haz the power, 2013-03-24 22:03:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/12450014/117001

[commit-bot: I haz the power, 2013-03-25 00:23:35]

Change committed as 190306