[win8] Force browsers created with Chrome in Metro mode to be on the Ash desktop.

[win8] Force browsers created while Chrome is in Metro mode to be on the Ash desktop.

When a user has configured Chrome to run in Metro mode, all browser
windows should open in the Ash desktop, regardless of where the request
came from. This includes browser desktop shortcuts, v1 browser-hosted
apps, OAuth windows, links from packaged apps, and DevTools windows.
Currently there are many code paths that result in a browser created in
Windows Desktop mode.

Also, if a browser is created in Ash, Metro mode does not always
activate. This may cause a user to think no window was created.

This CL adds BrowserWindow::AdjustHostDesktopType(..) to represent the
current policy decision that browser windows should only open on the Ash
desktop on Windows, when Chrome is running in Metro mode.

The policy is enforced by ensuring all constructed Browser instances are
created with a verified host desktop, which will be TYPE_ASH whenever the
ash shell is instantiated on Windows.

Then, when a browser window gains focus in the Ash desktop, if the metro
viewer process is not already the foreground window, switch to Metro
mode.

BUG=315385, 315894, 319173
TEST=Have Chrome configured in Metro mode. Then:
1. Open Chrome via the Desktop mode App Launcher.
2. Open Chrome via the context menu on the Chrome icon on the Desktop
mode App Launcher.
3. Open Chrome with a profile-badged desktop shortcut (twice).
4. (in Metro Mode) open an undocked Inspector window.
5. Open a v1 hosted app (e.g. Drive) from the Desktop mode app launcher.
6. After showing the Desktop app launcher and closing it, open a v1
hosted app desktop shortcut
===> All of these should open windows in Ash, and switch to Metro mode.

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

[tapted, 2013-11-14 11:38:12]

Hi cpu & sky. Please take a look. I think this is the policy we want to enforce
in Windows 8. However many things currently slip through.

I'm a bit nervous about the change to browser.cc - please let me know your
thoughts. The alternative I have is the "whackamole" approach, which I've
started in crrev/72303002/. That other CL doesn't yet even cover fixing the
codepaths I know about, but it gives an idea of the scope of the problem.

One merit of the approach here, in this CL, is that it centralizes the current
policy decision that Browser windows are not spread between Desktop and Ash, the
way they were initially in m32. Whereas the whackamole approach starts to spread
this knowledge over the codebase.

[sky, 2013-11-14 15:28:41]

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
File chrome/browser/ui/host_desktop.h (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
chrome/browser/ui/host_desktop.h:49: HostDesktopType
GetVerifiedHostDesktopForBrowser(HostDesktopType desktop_type);
This doesn't feel like the right place for this code. From your description the
setting applies only to browsers, right? Some where closer to browser is a
better place.

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame_ash_win.cc (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash_win.cc:29:
base::MessageLoop::current()->PostTask(
How come the delay here?

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame_ash_win.h (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash_win.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_FRAME_ASH_WIN_H_
I think you should go with ashwin, like we have aurawin.

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/native_browser_frame_factory_aura.cc (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/native_browser_frame_factory_aura.cc:20: #if
defined(OS_WIN)
Oy, what a sea of ifdefs. I think it's time for aurawin, aurax11 and chromeos

[Shrikant Kelkar, 2013-11-14 23:54:22]

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
File chrome/browser/ui/host_desktop.cc (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
chrome/browser/ui/host_desktop.cc:76: return chrome::HOST_DESKTOP_TYPE_ASH;
I doubt if this will work when we have both ASH and Native Desktop instances
running?

[tapted, 2013-11-15 00:11:51]

(no new code yet - need to ActivateMetroChrome on myself to my slow win8
build/machine, but some answers to questions)

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
File chrome/browser/ui/host_desktop.cc (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
chrome/browser/ui/host_desktop.cc:76: return chrome::HOST_DESKTOP_TYPE_ASH;
On 2013/11/14 23:54:23, Shrikant Kelkar wrote:
> I doubt if this will work when we have both ASH and Native Desktop instances
> running?

Yes that's correct. This function is to capture the current policy that we do
not have both instances running. E.g. the Desktop app launcher has a "Chrome"
app. With policy="both" that should open a browser on the desktop. With
policy="metro OR desktop" that should open a browser in Ash.

Is there a timeline in mind for switching back to "both"?

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
File chrome/browser/ui/host_desktop.h (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
chrome/browser/ui/host_desktop.h:49: HostDesktopType
GetVerifiedHostDesktopForBrowser(HostDesktopType desktop_type);
On 2013/11/14 15:28:42, sky wrote:
> This doesn't feel like the right place for this code. From your description
the
> setting applies only to browsers, right? Some where closer to browser is a
> better place.

I'll try it out in browser_commands.h . (I originally had it in an anonymous
namespace in browser.cc, but I think longer-term it shouldn't just be
browser.cc's call, so moved it out so other things can use it it.)

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame_ash_win.cc (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash_win.cc:29:
base::MessageLoop::current()->PostTask(
On 2013/11/14 15:28:42, sky wrote:
> How come the delay here?

There's a comment on chrome::ActivateMetroChrome() about this (I'll add a
comment here too). ActivateMetroChrome can't be called nested in another
::SendMessage(), and I was able to trigger this via a desktop shortcut coming in
via ProcessCommandLineAlreadyRunning

[tapted, 2013-11-15 12:47:11]

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
File chrome/browser/ui/host_desktop.h (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/host_d...
chrome/browser/ui/host_desktop.h:49: HostDesktopType
GetVerifiedHostDesktopForBrowser(HostDesktopType desktop_type);
On 2013/11/15 00:11:51, tapted wrote:
> On 2013/11/14 15:28:42, sky wrote:
> > This doesn't feel like the right place for this code. From your description
> the
> > setting applies only to browsers, right? Some where closer to browser is a
> > better place.
> 
> I'll try it out in browser_commands.h . (I originally had it in an anonymous
> namespace in browser.cc, but I think longer-term it shouldn't just be
> browser.cc's call, so moved it out so other things can use it it.)

Done.

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame_ash_win.cc (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash_win.cc:29:
base::MessageLoop::current()->PostTask(
On 2013/11/15 00:11:51, tapted wrote:
> On 2013/11/14 15:28:42, sky wrote:
> > How come the delay here?
> 
> There's a comment on chrome::ActivateMetroChrome() about this (I'll add a
> comment here too). ActivateMetroChrome can't be called nested in another
> ::SendMessage(), and I was able to trigger this via a desktop shortcut coming
in
> via ProcessCommandLineAlreadyRunning

Done.

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame_ash_win.h (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ash_win.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_FRAME_ASH_WIN_H_
On 2013/11/14 15:28:42, sky wrote:
> I think you should go with ashwin, like we have aurawin.

Done. (true - this was abusing that "win" \implies "aura && ash". We could also
call it "aurawin" and abuse "win && aura" implies "ash". But "ashwin" means no
abuse :)

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/native_browser_frame_factory_aura.cc (right):

https://codereview.chromium.org/58853004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/native_browser_frame_factory_aura.cc:20: #if
defined(OS_WIN)
On 2013/11/14 15:28:42, sky wrote:
> Oy, what a sea of ifdefs. I think it's time for aurawin, aurax11 and chromeos

Done. Fluctuated a lot with the various permutations - see what you think of
what I came up with.

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame_ashwin.cc (right):

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_ashwin.cc:30: // ::SendMessage().
(comment added here, otherwise unchanged from patchset 4)

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/native_browser_frame_factory.h (right):

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/native_browser_frame_factory.h:36: static bool
ShouldCreateForAshDesktop(BrowserView* browser_view);
Note: the current approach leaves this undefined on chromeos, and on use_ash==0.
Alternative is a header like native_browser_frame_factory_nonchromeosash.h or
_ashwinanddesktoplinux.h. Or an #ifdef here, but it's not strictly needed..

[sky, 2013-11-15 17:31:26]

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:336:
host_desktop_type_(chrome::GetVerifiedHostDesktopForBrowser(
Since this is platform specific, how about a factory on BrowserWindow, eg
BrowserWindow::AdjustHostDesktopType(...) ? That way you're not adding a
depending on ash in the non ash directories.

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/native_browser_frame_factory.h (right):

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/native_browser_frame_factory.h:36: static bool
ShouldCreateForAshDesktop(BrowserView* browser_view);
On 2013/11/15 12:47:11, tapted wrote:
> Note: the current approach leaves this undefined on chromeos, and on
use_ash==0.
> Alternative is a header like native_browser_frame_factory_nonchromeosash.h or
> _ashwinanddesktoplinux.h. Or an #ifdef here, but it's not strictly needed..

Is there a reason not to move it to native_browser_frame_factory.cc and always
compile it?

[tapted, 2013-11-18 11:13:47]

PTAL

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:336:
host_desktop_type_(chrome::GetVerifiedHostDesktopForBrowser(
On 2013/11/15 17:31:27, sky wrote:
> Since this is platform specific, how about a factory on BrowserWindow, eg
> BrowserWindow::AdjustHostDesktopType(...) ? That way you're not adding a
> depending on ash in the non ash directories.

Done (I think!) + updated CL description.

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/native_browser_frame_factory.h (right):

https://codereview.chromium.org/58853004/diff/800001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/native_browser_frame_factory.h:36: static bool
ShouldCreateForAshDesktop(BrowserView* browser_view);
On 2013/11/15 17:31:27, sky wrote:
> On 2013/11/15 12:47:11, tapted wrote:
> > Note: the current approach leaves this undefined on chromeos, and on
> use_ash==0.
> > Alternative is a header like native_browser_frame_factory_nonchromeosash.h
or
> > _ashwinanddesktoplinux.h. Or an #ifdef here, but it's not strictly needed..
> 
> Is there a reason not to move it to native_browser_frame_factory.cc and always
> compile it?

It needs to pull in some ash-specific files - if that's OK I can merge the
definition from native_browser_frame_factory_desktop_ash.cc into
native_browser_frame_factory.cc. I've now added the missing definition there to
compare (Done.), but it is just NOTREACHED(). I kept the definition out
initially because, without it, the NOTREACHED can be a link error rather than a
runtime error (but maybe that's more confusing).

[sky, 2013-11-18 14:59:04]

LGTM

[tapted, 2013-11-18 22:32:31]

shirkant (or cpu?): does this all look good at a kind of conceptual level?

Some of these bugs recently got upgraded as M32 release blockers.. this patch is
bit scary merge-wise, but I think it's what we need.

[shrikant_google.com, 2013-11-18 23:53:36]

Looks okay to me for now. Given that we want to force single mode (metro or
native) for this release.


On Mon, Nov 18, 2013 at 2:32 PM, <tapted@chromium.org> wrote:

> shirkant (or cpu?): does this all look good at a kind of conceptual level?
>
> Some of these bugs recently got upgraded as M32 release blockers.. this
> patch is
> bit scary merge-wise, but I think it's what we need.
>
> https://codereview.chromium.org/58853004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-11-19 00:45:18]

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

[commit-bot: I haz the power, 2013-11-19 04:00:44]

Change committed as 235906