924911 - Story - Profile switching main task: Change View on Desktop feature to be switch to Desktop feature

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Brian R. Bondy [:bbondy]]

Change the button for view on desktop to be Switch to Desktop (exact wording depends on UX).

Add a new uint32_t value here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/startup/public/nsIAppStartup.idl?from=nsIAppStartup.idl#l1 called:
const uint32_t eRestartTouchEnvironment = 0x80;

And add a new attribute for restartingTouchEnvironment: 
readonly attribute boolean restartingTouchEnvironment

Add code to implement that property around here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/startup/nsAppStartup.cpp?from=GetRestarting#l522

---

We already have code for handling re-launching Firefox for updates here:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroAppShell.cpp#l200

We need to update WinLaunchDeferredMetroFirefox() to take a parameter of bool aInMetro.  If it is specified inside WinLaunchDeferredMetroFirefox we should set parameters of executeCommand->SetParameters(L"--metro-restart"); otherwise we should set parameters of executeCommand->SetParameters(L"--desktop-restart");

We should add a similar function call here, and strip out the old code:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/nsWinMetroUtils.cpp#l325
Inside that code we should also save to session store and ensure that it will be used next time. (A special pref or something being set might be needed to force it for the next firefox launch, but I'm not sure)

We also need to update the interface for launchInDesktop to not take any parameters.

Inside the little COM object that we use for launching firefox deferred, we already have support for:
--metro-restart
We need to also add support for:
--desktop-restart
To do this just follow along with this code:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp#l153
But add a new flag for mIsRestartDesktopRequest

Node in this bug only the restart in desktop feature will be used, there is another bug which is a dependency of bug 924860 which will use the flag added here for restarting in Metro from Desktop.

p=8

Comment 1

[Marina Samuel [:emtwo]]

Attachment: WIP: Switch to desktop

A few notes:

1) The biggest thing still missing from this patch is that switching to desktop will only restore your session if you choose "restore previous session" so I believe that part is what you mentioned in your comment that I still need to look into.

2) The only way I was able to get windows to switch from metro to desktop mode was by using ShellExecuteEx()  in nsWinMetroUtils::LaunchInDesktop() and I couldn't figure out why. If I did appStartup->Quit(nsIAppStartup::eForceQuit | nsIAppStartup::eRestart); without ShellExecuteEx(), for example, this would open up Desktop Firefox but I would have to manually switch from Metro to Desktop to see it.

3) nsWinMetroUtils::LaunchInDesktop() was also being used to open up downloaded files by passing in a URI for their location. Does it sound reasonable to keep the content of LaunchInDesktop() as a separate function for this purpose?

4) I didn't do anything with the restarting touch variables because it sounds like they'll be used for bug 924914 (correct me if I'm wrong)

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 821928 [details] [diff] [review]
WIP: Switch to desktop

Review of attachment 821928 [details] [diff] [review]:
-----------------------------------------------------------------

Great progress so far! Thanks for the work in progress patch, I think posting WIPs like this will help us deliver fast :D

> 1) The biggest thing still missing from this patch is that switching to
> desktop will only restore your session if you choose "restore previous
> session" so I believe that part is what you mentioned in your comment that I
> still need to look into.


Yep let's handle that in a new bug later.
Also I'm surprised it works at all because they are using said-to-be incompatible session restore formats.

> 2) The only way I was able to get windows to switch from metro to desktop
> mode was by using ShellExecuteEx()  in nsWinMetroUtils::LaunchInDesktop()
> and I couldn't figure out why. If I did
> appStartup->Quit(nsIAppStartup::eForceQuit | nsIAppStartup::eRestart);
> without ShellExecuteEx(), for example, this would open up Desktop Firefox
> but I would have to manually switch from Metro to Desktop to see it.

See review notes below.

> 3) nsWinMetroUtils::LaunchInDesktop() was also being used to open up
> downloaded files by passing in a URI for their location. Does it sound
> reasonable to keep the content of LaunchInDesktop() as a separate function
> for this purpose?

Also noticed that before seeing this comment, covered it in the review comments.

> 4) I didn't do anything with the restarting touch variables because it
> sounds like they'll be used for bug 924914 (correct me if I'm wrong)

See review comments, I have a suggested use.

::: browser/metro/base/content/appbar.js
@@ +150,5 @@
>        });
>    },
>  
>    onViewOnDesktop: function() {
> +    Services.metro.launchInDesktop();

See note below in nsWinMetroUtils::LaunchInDesktop() or what to do here.

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +153,5 @@
>  
>      if (_wcsicmp(aParameters, kMetroRestartCmdLine) == 0) {
>        mIsRestartMetroRequest = true;
> +    } else if (_wcsicmp(aParameters, kDesktopRestartCmdLine) == 0) {
> +      mIsDesktopRequest = true;

nit: Call this mIsRestartDesktopRequest to be consistent with mIsRestartMetroRequest.

We'll need to use mIsDesktopRequest as well in this file. I'm sure you played with it previously but as you said it wasn't working properly. 

On this line:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp#l701

We'll need to change it to: 
if (mIsRestartMetroRequest || mIsRestartDesktopRequest) {

That alone probably won't be enough to work.
I have a couple ideas to try:
1. Try to force the value here when the mIsRestartDesktopRequest is set.
2. If that doesn't work, copy the code from DelayedExecuteThread into a new DelayedExecuteDesktopThread, and call ShellExecute directly there.  You can find ShellExecute code in this file already, or in the launchInDesktop code.

::: widget/windows/winrt/MetroAppShell.cpp
@@ +201,5 @@
>  
>        nsCOMPtr<nsIAppStartup> appStartup (do_GetService(NS_APPSTARTUP_CONTRACTID));
>        bool restarting;
>        if (appStartup && NS_SUCCEEDED(appStartup->GetRestarting(&restarting)) && restarting) {
> +        if (!WinLaunchDeferredMetroFirefox(true)) {

Instead of hardcoding true here, you should be passing the out parameter returned from 

bool restartingInMetro;

- if (!WinLaunchDeferredMetroFirefox(true)) {
+ if (!WinLaunchDeferredMetroFirefox(NS_SUCCEEDED(appStartup->GetRestartingTouchEnvironment(&restartingInMetro)) && restartingInMetro)) {

::: widget/windows/winrt/nsWinMetroUtils.cpp
@@ +321,5 @@
>   * Launches the specified application with the specified arguments and
>   * switches to Desktop mode if in metro mode.
>  */
>  NS_IMETHODIMP
> +nsWinMetroUtils::LaunchInDesktop()

Let's leave LaunchInDesktop alone with it's parameters. It is used for launching default handlers for downloads here:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/base/content/downloads.js#l108

Instead just update this code:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/base/content/appbar.js#l160

And in that code directly instead of calling launchInDesktop just call:
let appStartup = Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(Components.interfaces.nsIAppStartup);
appStartup.quit(Components.interfaces.nsIAppStartup.eAttemptQuit |
                Components.interfaces.nsIAppStartup.eRestart);

Note that we aren't passing Components.interfaces.nsIAppStartup.eRestartTouchEnvironment here on purpose because we want it to restart in desktop instead of in metro.  In the bug for switch to Metro from Desktop, we'll use eRestartTouchEnvironment.

We should be using Components.interfaces.nsIAppStartup.eRestartTouchEnvironment here in the context of this bug though:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/base/content/flyoutpanels/AboutFlyoutPanel.js#l276

@@ +329,5 @@
> +  nsCOMPtr<nsIAppStartup> appStartup =
> +    do_GetService(NS_APPSTARTUP_CONTRACTID);
> +  if (appStartup) {
> +    appStartup->Quit(nsIAppStartup::eForceQuit);
> +  }

As per above, remove all of this code and restore the old parameters passed to ShellExecute's sinfo structure.

::: widget/windows/winrt/nsWinMetroUtils.h
@@ +8,5 @@
>  #include "nsIWinMetroUtils.h"
>  #include "nsString.h"
> +#include "MetroAppShell.h"
> +#include "nsToolkitCompsCID.h"
> +#include "nsIAppStartup.h"

nit: It's better to put includes in the .cpp for faster build times unless it's needed, but in this case it's not needed in the header.

Comment 3

[Brian R. Bondy [:bbondy]]

> I have a couple ideas to try:
> 1. Try to force the value here when the mIsRestartDesktopRequest is set.

Forgot the link: 
http://dxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp#l260

Comment 4

[Marina Samuel [:emtwo]]

Attachment: WIP: Switch to desktop

Addressed comments. I tried both proposed approaches you suggested to switch to desktop but they don't seem to be working. Does this look like what you were proposing?

If so, I think a possible reason why this may not be working is that a restart does not fully close Firefox and since Firefox is already 'open' it doesn't switch to desktop to show the newly opened app. This can be seen with a download. Clicking 'Show in Files' for a completed download will not switch to desktop mode if the Files app is already open at the directory where the downloaded file is.

If that's the case, I think we either want to quit and re-open instead of 'eRestart' or somehow change the restart code to take this scenario into account.

What do you think?

Comment 5

[Brian R. Bondy [:bbondy]]

(In reply to Marina Samuel [:emtwo] from comment #4)
> Created attachment 823383 [details] [diff] [review]
> WIP: Switch to desktop
> 
> Addressed comments. I tried both proposed approaches you suggested to switch
> to desktop but they don't seem to be working. Does this look like what you
> were proposing?

I'll take a look momentarily.

> 
> If so, I think a possible reason why this may not be working is that a
> restart does not fully close Firefox and since Firefox is already 'open' it
> doesn't switch to desktop to show the newly opened app. This can be seen
> with a download. Clicking 'Show in Files' for a completed download will not
> switch to desktop mode if the Files app is already open at the directory
> where the downloaded file is.
> 
> If that's the case, I think we either want to quit and re-open instead of
> 'eRestart' or somehow change the restart code to take this scenario into
> account.
> 
> What do you think?

That doesn't sound right though, we shouldn't be even trying to execute the Desktop browser until the metro one is closed. The code that does that is here:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp?from=CommandExecuteHandler.cpp#l668

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 823383 [details] [diff] [review]
WIP: Switch to desktop

Review of attachment 823383 [details] [diff] [review]:
-----------------------------------------------------------------

I think the problem is actually here:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp?from=CommandExecuteHandler.cpp#l609

It should be:
sinfo.fMask = SEE_MASK_FLAG_LOG_USAGE;


> Should a Metro style enabled desktop browser wish to launch a desktop app and have that app come to the 
> foreground (for example, downloading and launching a file such as a PDF reader), it may do so by using 
> ShellExecuteEx() and specifying the SEE_MASK_FLAG_LOG_USAGE flag in the fMask field. If this flag is 
> not specified the desktop app will launch, but will not be able to come to the foreground.   
> There is no automatic switching between the Metro style experience and the desktop.

We've never had to actually do this before from the CEH, so that's just a bug there.

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +267,1 @@
>        Log(L"No mUnkSite.");

nit: This log message doesn't make sense here for the case of mIsRestartDesktopRequest. Please split out the checks.

@@ +415,3 @@
>    bool mTargetIsFileSystemLink;
> +  static bool mTargetIsDefaultBrowser;
> +  static bool mTargetIsBrowser;

We shouldn't make all of this static. Instead:

CExecuteCommandVerb::LaunchDesktopBrowser() should call into a new function and pass that new function all the needed information.

You'd also call that new function from the Desktop thread.

Note: You can create a struct with needed members for the thread, and pass that to the thread through the void* param.  The thread will be called with that pointer which you can cast back to the struct pointer and access its members.

Comment 7

[Marina Samuel [:emtwo]]

Attachment: WIP #3: Switch to desktop

Made suggested changes. Still doesn't seem to be switching to desktop :( I tried to adjust some other parameters as well but with no success. I'll continue to try to fix this but if you have any more suggestions please let me know.

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 824048 [details] [diff] [review]
WIP #3: Switch to desktop

Review of attachment 824048 [details] [diff] [review]:
-----------------------------------------------------------------

A couple nits, let me take a look at this locally, but in the meantime, after the nits let's start on bug 924914

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +646,5 @@
> +{
> +  CStringW mBrowserPath, mVerb, mTarget, mParameters;
> +  bool mTargetIsDefaultBrowser, mTargetIsBrowser, mAutoSetRequestMet;
> +
> +};

nit: empty line should be removed after the 3 bools.

@@ +713,5 @@
>  
> +DWORD WINAPI
> +DelayedExecuteDesktopThread(LPVOID param)
> +{
> +  RestartDesktopThreadParams &desktopThreadParams(*(RestartDesktopThreadParams*)param);

nit: Let's treat this as a pointer instead of a reference, and then delete it before the function returns but after you've used it.

Comment 9

[Marina Samuel [:emtwo]]

Attachment: WIP #4: Switch to desktop

nits addressed

Comment 10

[Marina Samuel [:emtwo]]

Attachment: WIP #5: Switch to desktop

There was a flaw in logic that resulted in a restart instead of shut down. Fixed here.

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 824174 [details] [diff] [review]
WIP #5: Switch to desktop

Review of attachment 824174 [details] [diff] [review]:
-----------------------------------------------------------------

Still looking for a solution. In the worst case I think we can just launch a dummy http page, and it would work (Similar to how view on desktop feature works today, but it uses a real http page). But I want to spend a bit more time trying to find a better solution.  I remember fighting with a similar issue when I first implemented the view on desktop feature.

Put some nits that can be implemented for now.  Also pls continue with the view on metro feature, you shouldn't run into the same issue there because we currently launch the metro browser from the desktop updater.exe and it works after updates.

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +583,5 @@
>  /*
>   * Desktop launch - Launch the destop browser to display the current
>   * target using shellexecute.
>   */
> +void LaunchDesktopBrowserWithParams(CStringW& browserPath, CStringW& mVerb, CStringW& mTarget, CStringW& mParameters,

nit: these parameters should be named aBrowserPath, aVerb, aTarget, ...
The 'm' prefix is used for class and struct members.

@@ +616,2 @@
>    seinfo.hwnd   = nullptr;
> +  seinfo.lpVerb = L"open";

nit: use aVerb here
Also initialize mVerb inside CExecuteCommandVerb() to L"open" so that it gets set for the case of the thread code path where Initailize() is not called.

@@ +718,5 @@
> +  CoInitialize(NULL);
> +
> +  CComPtr<IApplicationActivationManager> activateMgr;
> +  if (FAILED(PrepareActivationManager(activateMgr))) {
> +      Log(L"Warning: Could not prepare activation manager");

This activateMgr block isn't needed btw since we're just executing ShellExecute. Also you can get rid of CoInitialize and CoUninitialize.

Comment 12

[Brian R. Bondy [:bbondy]]

Hold off on the activateMgr comments btw until we find the best path forward in case we have to move away from ShellExecute.

Comment 13

[Brian R. Bondy [:bbondy]]

OK so we have a few options but it looks like we have to basically get Windows to do the activation for us from within CommandExecuteHandler. 

We won't be changing much by the way, all of the existing code stays in place.

In general, what we'll do is after we use CommandExecuteHandler to wait for ourselves to close in the thread you added, we'll tell Windows *go ahead* (Via the method described below).  And Windows will make a new CommandExecuteHandler to carry out the operation and environment switch. 

Since we know we're in Metro mode when this code executes, we know we are the default HTTP handlers. (We could alternatively go after an lnk file or an HTML file URL, but HTTP handler is best here I think.)

So instead of ShellExecuting on the browser path from within that new thread, instead shellExecute on http://-desktop.

Remember to uncomment this line in CEHHelper.h for debugging:
//#define SHOW_CONSOLE 1

Then add some extra code to force the value returned from this function:
// IExecuteCommandApplicationHostEnvironment
IFACEMETHODIMP GetValue(AHE_TYPE *aLaunchType)
So Inside GetValue if the url passed is http://-desktop then return desktop.

I had you add code previously there for mIsRestartDesktopRequest but I realize now that it is not hit when we run he command execute handler ourselves. (You can leave that code as is).

So to summarize the whole process:
i) we call some js to restart firefox in desktop mode from within Metro Firefox. 
ii) That triggers firefox metro to start closing
iii) and it calls into the command execute handler to wait for metro firefox to finish closing in a thread you added.
iv) Once that thread ends, we will execute http://-desktop
v) That will cause Windows to do an environment switch and create a new Command Execute Handler to carry out the operation.
vi) Windows will call into the command execute handler and ask it which environment to choose. The Commnd Execute Handler is allowed to pick now because it considers this a user initiated action. It gets this info from IExecuteCommandApplicationHostEnvironment::GetValue in which you'll return AHE_DESKTOP because http://-desktop was passed.
vii) A switch to desktop should happen

Step iv) and beyond is the new part.

That should get things working. We may need to add code to filter out the http://-desktop URL that will be passed on the second instantiation of the CEH. Otherwise desktop firefox will try to open up that URL.

Comment 14

[Brian R. Bondy [:bbondy]]

Some changes discussed on IRC today with Marina:
[15:51:00] <emtwo> ok
[15:51:45] <bbondy> and go to this line if (!WinLaunchDeferredMetroFirefox(restartingInMetro)) {
[15:51:52] <bbondy> instead of calling that let's just do this:
[15:51:57] <bbondy> if (restartingInMetro) { 
[15:52:12] <bbondy>   WinLaunchDeferredMetroFirefox(true);
[15:52:13] <bbondy> }
[15:52:17] <bbondy> else {
[15:52:55] <bbondy> ShellExecute with the seinfo.fMask = SEE_MASK_FLAG_LOG_USAGE; and specifying http://-desktop
[15:52:57] <bbondy> }
[15:53:32] <bbondy> Then in CommandLineExecuteHandler, continue on step v) of Comment 13
[15:54:42] <emtwo> ok cool, I'll try that
[15:55:24] <bbondy> that simplifies the code path a bit, the main difference is now we don't launch Command execute handler manually to wait for a clean shutdown, we just tell windows to launch it early.  Because at this point the profile should no longer be in use.
[15:56:29] <bbondy> thanks sorry about the confusion, I didn't realize until recently that we only need to wait for profile to be unlocked not program to be unused.
[15:56:43] <bbondy> the way described in the last comment would still work, but this way will have better perf
[15:56:44] <emtwo> its ok, no problem
[15:56:50] <emtwo> yea i understand
[15:58:31] <emtwo> I mean, this bug has the highest point value from all the shared profile bugs so I was expecting it to take some time
[15:58:35] <emtwo> though it actually doesn't seem too bad
[15:58:55] <bbondy> with updates we had to wait for the program to be completely closed because the relaunch to metro wont' work from metro unless it is completely closed

Comment 15

[Marina Samuel [:emtwo]]

Attachment: v1: Switch to desktop

There's some unused code (e.g. --desktop-restart is not used here) but I kept it since as you mentioned, it could be useful later on. And it works now :)

Comment 16

[Marina Samuel [:emtwo]]

Attachment: v2: Switch to desktop

Add a check to ignore ShellExecute params only if the target is "http://-desktop"

Comment 17

[Brian R. Bondy [:bbondy]]

Comment on attachment 825383 [details] [diff] [review]
v2: Switch to desktop

Review of attachment 825383 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, we're just about there.
One more pass and we should be good.

- Let's always show the menu item now, to do this get rid of the http / https scheme check here:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/base/content/appbar.js#l130
 
- Let's update the text to "Relaunch in Desktop" to make it clear that a close/open will happen here:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/locales/en-US/chrome/browser.dtd#l19

::: browser/metro/base/content/appbar.js
@@ +151,5 @@
>    },
>  
>    onViewOnDesktop: function() {
> +    let appStartup = Components.classes["@mozilla.org/toolkit/app-startup;1"]
> +      .getService(Components.interfaces.nsIAppStartup);

nit: put the . on the previous line, but keep the indenting the same otherwise.

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +43,5 @@
>  static const WCHAR* kFirefoxExe = L"firefox.exe";
>  static const WCHAR* kMetroFirefoxExe = L"firefox.exe";
>  static const WCHAR* kDefaultMetroBrowserIDPathKey = L"FirefoxURL";
>  static const WCHAR* kMetroRestartCmdLine = L"--metro-restart";
> +static const WCHAR* kDesktopRestartCmdLine = L"--desktop-restart";

yep this is good mirrored functionality to have and we should keep it.

@@ +617,5 @@
> +  seinfo.lpVerb = aVerb;
> +  seinfo.lpFile = aBrowserPath;
> +  seinfo.nShow  = SW_SHOWNORMAL;
> +
> +  if (_wcsicmp(aTarget, L"http://-desktop/") != 0) {

nit: Add a comment here about why we're doing this: i.e. something like: Relaunch in Desktop mode uses a special URL to trick Windows into switching environments. We shouldn't actually try to open this URL.

@@ +649,5 @@
> +{
> +  CStringW mBrowserPath, mVerb, mTarget, mParameters;
> +  bool mTargetIsDefaultBrowser, mTargetIsBrowser, mAutoSetRequestMet;
> +};
> +

nit: This struct can be removed since the thread is removed now.

::: toolkit/components/startup/nsAppStartup.cpp
@@ +436,5 @@
>      if (obsService) {
>        NS_NAMED_LITERAL_STRING(shutdownStr, "shutdown");
>        NS_NAMED_LITERAL_STRING(restartStr, "restart");
>        obsService->NotifyObservers(nullptr, "quit-application",
> +        (mRestart || mRestartTouchEnvironment) ? restartStr.get() : shutdownStr.get());

nit: keep line length for this code <= 80 chars.

@@ +562,5 @@
>  
>  NS_IMETHODIMP
> +nsAppStartup::GetRestartingTouchEnvironment(bool *aResult)
> +{
> +  *aResult = mRestartTouchEnvironment;

nit: before this line add
NS_ENSURE_ARG_POINTER(aResult);

::: toolkit/components/startup/public/nsIAppStartup.idl
@@ +121,5 @@
>       */
>      const uint32_t eRestartx86_64 = 0x40;
>  
>      /**
> +     * Restart the application in metro after quitting. The application will be

nit: Let's avoid the name metro here and say a touch optimized environment to keep it generic in case other OS ever want to do something similar.

@@ +166,5 @@
>       */
>      readonly attribute boolean wasRestarted;
>  
> +    /**
> +     * True if the application is being restarted in metro

nit: in a touch optimized environment (such as Metro)

@@ +168,5 @@
>  
> +    /**
> +     * True if the application is being restarted in metro
> +     */
> +    readonly attribute boolean restartingTouchEnvironment;

Whenever an idl file is changed you need to also update the uuid
So just change this line:
[scriptable, uuid(6799abed-4721-4f51-9304-d1a2ea1df5d5)]

You can get a new guid from guidgen or simply with mach uuid >out.txt and then copy it out.

::: widget/windows/winrt/MetroAppShell.cpp
@@ +200,5 @@
>        rv = nsBaseAppShell::Run();
>        mozilla::widget::StopAudioSession();
>  
>        nsCOMPtr<nsIAppStartup> appStartup (do_GetService(NS_APPSTARTUP_CONTRACTID));
> +      bool restartingInMetro, restarting;

nit: Init these both to false.

@@ +201,5 @@
>        mozilla::widget::StopAudioSession();
>  
>        nsCOMPtr<nsIAppStartup> appStartup (do_GetService(NS_APPSTARTUP_CONTRACTID));
> +      bool restartingInMetro, restarting;
> +      if (appStartup && NS_SUCCEEDED(appStartup->GetRestartingTouchEnvironment(&restartingInMetro)) &&

nit: Only check for GetRestartingTouchEnvironment && restartingInMetro here and not GetRestarting && restarting.

@@ +203,5 @@
>        nsCOMPtr<nsIAppStartup> appStartup (do_GetService(NS_APPSTARTUP_CONTRACTID));
> +      bool restartingInMetro, restarting;
> +      if (appStartup && NS_SUCCEEDED(appStartup->GetRestartingTouchEnvironment(&restartingInMetro)) &&
> +          NS_SUCCEEDED(appStartup->GetRestarting(&restarting)) &&
> +          (restartingInMetro || restarting)) {

Replace: (restartingInMetro || restarting)
with: restartingInMetro 
here.

@@ +207,5 @@
> +          (restartingInMetro || restarting)) {
> +        if (restartingInMetro) {
> +          WinLaunchDeferredMetroFirefox(true);
> +        } else {
> +          SHELLEXECUTEINFOW sinfo;

There's a bit of a race condition here, but not in the above case of WinLaunchDeferredMetroFirefox(true);

The reason is because WinLaunchDeferredMetroFirefox waits for a close first before relaunching. 

Move this code, below. See next note.

@@ +224,5 @@
>  
>        // This calls XRE_metroShutdown() in xre. This will also destroy
>        // MessagePump.
>        sMetroApp->ShutdownXPCOM();
>  

We computed the bool restarting above (which is good because we can't do it anymore since xpcom is shut down). 

Add here:
if (restarting) {
  Do the shell execute code here.
}

Comment 18

[Marina Samuel [:emtwo]]

Attachment: v3: Switch to desktop

Comment 19

[Brian R. Bondy [:bbondy]]

Comment on attachment 825905 [details] [diff] [review]
v3: Switch to desktop

Review of attachment 825905 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for oak for now until we're sure it's safe to land on m-c.
I'll land everything on oak myself soonish btw.

::: widget/windows/winrt/MetroAppShell.cpp
@@ +207,5 @@
> +          restartingInMetro) {
> +        WinLaunchDeferredMetroFirefox(true);
> +      }
> +
> +      if (!appStartup || !NS_SUCCEEDED(appStartup->GetRestarting(&restarting))) {

nit: NS_FAILED instead of !NS_SUCCEEDED

Comment 20

[Brian R. Bondy [:bbondy]]

Comment on attachment 825905 [details] [diff] [review]
v3: Switch to desktop

Review of attachment 825905 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/MetroAppShell.cpp
@@ +207,5 @@
> +          restartingInMetro) {
> +        WinLaunchDeferredMetroFirefox(true);
> +      }
> +
> +      if (!appStartup || !NS_SUCCEEDED(appStartup->GetRestarting(&restarting))) {

nit: NS_FAILED instead of !NS_SUCCEEDED

@@ +220,5 @@
>        // once winrt cleans up this thread.
>        sMetroApp->CoreExit();
> +
> +
> +      if (restarting) {

sorry one more nit, put this below shutdownXPOM but above CoreExit

Comment 21

[Marina Samuel [:emtwo]]

Attachment: v4: Switch to desktop

nits addressed

Comment 22

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/projects/oak/rev/4f198b9933b7

Comment 23

[Marina Samuel [:emtwo]]

Attachment: relaunch_in_desktop.png

Comment 24

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

(In reply to Marina Samuel [:emtwo] from comment #23)
> Created attachment 826181 [details]
> relaunch_in_desktop.png

Thanks, Marina. It looks good to me.
I will talk to Firefox UX team on Tuesday to finalize the copy. Will keep you posted.

Comment 25

[Brian R. Bondy [:bbondy]]

Landed on oak here:
https://hg.mozilla.org/projects/oak/rev/4f198b9933b7

Bug 935099 will track the landing on m-c.
When it lands on m-c a new comment will be added here as well with the m-c changeset.
This is being done so we can still use scrumbugs efficiently. 
See bug 935099 for further details.

Comment 26

[Manuela Muntean [Away]]

Attachment: oak_screenshot.png

While trying to verify this bug for iteration #18, I've tried the following, on a Win 8 32-bit machine: 

1) installed the .exe from ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/oak-win32/1384354004/ and made it the default browser

2) launched it in Metro mode, but didn't find the Switch to Desktop button; I can only see the 3 buttons shown in the attached screenshot

3) I've also noticed that, in Metro mode, the oak builds don't behave the same as the Nightly builds do:

- for example, if in desktop mode I pin 3 tabs, when launching in Metro mode, I can see the same 3 tabs, and viceversa

- if the oak build is opened in desktop mode, I can't launch it in Metro mode (at least not until I close the one in desktop mode)

Could anyone please offer guidance for this particular type of bugs that are related to shared profiles and use oak builds? What is the procedure here?

I can see another 4 similar bugs that need verification during iteration #18: 924914, 924995, 924894, 931846.

Comment 27

[Brian R. Bondy [:bbondy]]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #26)
> Created attachment 832137 [details]
> oak_screenshot.png
> 
> While trying to verify this bug for iteration #18, I've tried the following,
> on a Win 8 32-bit machine: 
> 
> 1) installed the .exe from
> ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/oak-win32/
> 1384354004/ and made it the default browser

Try using the latest nightly builds for testing please.

> 2) launched it in Metro mode, but didn't find the Switch to Desktop button;
> I can only see the 3 buttons shown in the attached screenshot

Relaunch in Desktop is the button you're looking for.
The text was changed to Relaunch.

> 3) I've also noticed that, in Metro mode, the oak builds don't behave the
> same as the Nightly builds do:
> 
> - for example, if in desktop mode I pin 3 tabs, when launching in Metro
> mode, I can see the same 3 tabs, and viceversa

Do you mean you fovorite 3 tabs? Where do you see them in each case? Do you see the tabs open or do you see them in the bookmarks bar?
A relaunch in Desktop should preserve these tabs. 

> - if the oak build is opened in desktop mode, I can't launch it in Metro
> mode (at least not until I close the one in desktop mode)

That is expected, I covered this info in bug 924860 Comment 6.
But since then you can use the latest nightly oak build and it should work as expected.
Please read this to see how the feature should work in general:
http://www.brianbondy.com/blog/id/155/shared-profiles-for-metro-firefox-and-desktop-firefox


> Could anyone please offer guidance for this particular type of bugs that are
> related to shared profiles and use oak builds? What is the procedure here?

Should be covered with the above info, let me know if it is not.

Comment 28

[Brian R. Bondy [:bbondy]]

(In reply to Brian R. Bondy [:bbondy] from comment #27)
> (In reply to Manuela Muntean [:Manuela] [QA] from comment #26)
> > Created attachment 832137 [details]
> > oak_screenshot.png
> > 
> > While trying to verify this bug for iteration #18, I've tried the following,
> > on a Win 8 32-bit machine: 
> > 
> > 1) installed the .exe from
> > ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/oak-win32/
> > 1384354004/ and made it the default browser
> 
> Try using the latest nightly builds for testing please.

I mean the latest nightly Oak builds by the way in the context of these bugs.
Link for the latest nightly oak build is in the above comment (bug 924860 comment 7)

Comment 29

[Manuela Muntean [Away]]

Thank you Brian for the info! 

With the latest Nightly Oak builds mentioned in bug 924860 comment 7, I get the following results:

- on Win 8 64-bit

1) if I open Nightly in desktop mode, I can't also start it in Metro mode, from the Start Screen (it doesn't start)

2) if I open Nightly in metro mode, open 3 tabs and then press the "Relaunch in Desktop" button, Nightly opens in desktop mode, but with only "about:home" tab opened (neither one of the 3 tabs opened in metro mode gets/remains open in desktop mode)

3) if I open Nightly in desktop mode, load 3 tabs and close it, after opening it in metro mode, all 3 tabs are opened

Is this the intended behavior? (I assume point 2) isn't)

Comment 30

[Brian R. Bondy [:bbondy]]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #29)
> Thank you Brian for the info! 
> 
> With the latest Nightly Oak builds mentioned in bug 924860 comment 7, I get
> the following results:
> 
> - on Win 8 64-bit
> 
> 1) if I open Nightly in desktop mode, I can't also start it in Metro mode,
> from the Start Screen (it doesn't start)

It should go to the desktop one, but perhaps you're using an older build.

> 
> 2) if I open Nightly in metro mode, open 3 tabs and then press the "Relaunch
> in Desktop" button, Nightly opens in desktop mode, but with only
> "about:home" tab opened (neither one of the 3 tabs opened in metro mode
> gets/remains open in desktop mode)

That's fine and not in the builds yet.

> 
> 3) if I open Nightly in desktop mode, load 3 tabs and close it, after
> opening it in metro mode, all 3 tabs are opened

Sounds like there's probably an unclean shutdown happening, what if you open Desktop again after closing it, what happens?

Comment 31

[Manuela Muntean [Away]]

> > With the latest Nightly Oak builds mentioned in bug 924860 comment 7, I get
> > the following results:
> > 
> > - on Win 8 64-bit
> > 
> > 1) if I open Nightly in desktop mode, I can't also start it in Metro mode,
> > from the Start Screen (it doesn't start)
> 
> It should go to the desktop one, but perhaps you're using an older build.

I've retested this with the latest oak build (build ID: 20131114040200) on the same Win 8 64-bit machine, and indeed, if in this situation I want to launch metro mode I can't, because it shifts the focus again to the desktop one. (I got the same behavior yesterday as well, with the build from 2013-11-13)  Today I installed the oak build in C:\Program Files and yesterday on the Desktop. (perhaps this affected the behavior somehow)

> > 
> > 2) if I open Nightly in metro mode, open 3 tabs and then press the "Relaunch
> > in Desktop" button, Nightly opens in desktop mode, but with only
> > "about:home" tab opened (neither one of the 3 tabs opened in metro mode
> > gets/remains open in desktop mode)
> 
> That's fine and not in the builds yet.

I've retested this today, and I get the exact results, with the oak build installed in C:\Program Files

> > 3) if I open Nightly in desktop mode, load 3 tabs and close it, after
> > opening it in metro mode, all 3 tabs are opened
> 
> Sounds like there's probably an unclean shutdown happening, what if you open
> Desktop again after closing it, what happens?

I've cleaned my PC (uninstalled all Nightly builds) and after a fresh install (this time, of the oak build in C:\Program Files), this issue isn't reproducible anymore.  If I open Desktop again after closing it, only the "about:home" tab shows, which I assume is intended. 


How soon can these types of bugs marked as "verified"? I assume that only after they have landed on m-c and also verified there, right?

Comment 32

[Brian R. Bondy [:bbondy]]

> I've retested this with the latest oak build (build ID: 20131114040200) on the same Win 8 64-bit 
> machine, and indeed, if in this situation I want to launch metro mode I can't, because it shifts the
> focus again to the desktop one

That is expected functionality, also I mentioned that in the blog as how it works originally. We hope to add some options around here in the future to control this behavior.

Comment 33

[Brian R. Bondy [:bbondy]]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #31)
> > > 2) if I open Nightly in metro mode, open 3 tabs and then press the "Relaunch
> > > in Desktop" button, Nightly opens in desktop mode, but with only
> > > "about:home" tab opened (neither one of the 3 tabs opened in metro mode
> > > gets/remains open in desktop mode)
> > 
> > That's fine and not in the builds yet.
> 

That fix hasn't landed yet. So you can ignore that for now, but retest that in a later iteration.

> I've retested this today, and I get the exact results, with the oak build
> installed in C:\Program Files
> 
> > > 3) if I open Nightly in desktop mode, load 3 tabs and close it, after
> > > opening it in metro mode, all 3 tabs are opened
> > 
> > Sounds like there's probably an unclean shutdown happening, what if you open
> > Desktop again after closing it, what happens?
> 
> I've cleaned my PC (uninstalled all Nightly builds) and after a fresh
> install (this time, of the oak build in C:\Program Files), this issue isn't
> reproducible anymore.  If I open Desktop again after closing it, only the
> "about:home" tab shows, which I assume is intended. 


I think it was just due to a shutdown crash, which causes tabs to be reloaded on the next startup. I think it's unrelated to this work.

> How soon can these types of bugs marked as "verified"? I assume that only
> after they have landed on m-c and also verified there, right?

Yep let's hold off on changing the overall status until they land on m-c.

Comment 34

[Marina Samuel [:emtwo]]

Attachment: Switch to desktop (rebased on mc with Australis)

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: Switch to desktop (rebased on mc with Australis)

Comment 36

[Brian R. Bondy [:bbondy]]

Relanded on post-australis rebased oak:
https://hg.mozilla.org/projects/oak/rev/e35a9c37bbb8

Comment 37

[Manuela Muntean [Away]]

Verified as fixed, for iteration #19, with the latest Nightly Oak build (build ID: 20131127114543) on Win 8 64-bit: if I open Nightly in desktop mode, I can't also start it in Metro mode; also the viceversa situation is showing.

Comment 38

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/fx-team/rev/ccfc7f55f776

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ccfc7f55f776

Comment 40

[Manuela Muntean [Away]]

Also verified as fixed, for iteration #19, using the latest Nightly 2013-12-05 on Win 8 64-bit: if I open Nightly in desktop mode, I can't also start it in Metro mode; also the viceversa situation is showing.