932664 - Story - Link clicks and file activations should open in the currently opened browser no matter which environment

Status: VERIFIED FIXED

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

Description

[Brian R. Bondy [:bbondy]]

Currently link clicks from metro always opens in the metro browser, and link clicks from desktop always opens in the desktop browser.

This task is to:
i) Detect if the browser is already open
ii) If so open the link click and file url in that environment
iii) If not do as we did before

p=1

(Marco pls subtract 1 from contingency when you add this in with whiteboard headers. Thanks!)

Comment 1

[Marina Samuel [:emtwo]]

Attachment: v1: Open links in current environment

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 826750 [details] [diff] [review]
v1: Open links in current environment

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

I don't think this will work in general. (I didn't try it yet though).

If I have the metro browser open, and I click on my taskbar icon, it should open Firefox Metro. 

In that case neither of mIsRestartMetroRequest nor mIsRestartDesktopRequest would be set.

Basically this function does a call to:

> EC_HOST_UI_MODE mode;
> if (FAILED(pHost->GetUIMode(&mode))) 

And then later checks that value and if we are already in Desktop (mode is desktop) it chooses Desktop, if we're already in Metro (mode is metro) it chooses Metro.  We want to not use that heuristic if the browser is already open.  I think you'd need add a IsDesktopProcessRunning which mirrors the functionality of IsImmersiveProcessRunning but checks for !immersive. 

if (IsImmersiveProcessRunning(kFirefox) {
  // force metro
  return;
} else if (IsDesktopProcessRunning(kFirefox) {
  // force desktop
  return;
}

Otherwise fall back to the old way of GetUIMMode

Comment 3

[Brian R. Bondy [:bbondy]]

Note that this doesn't play perfectly well with multiple browsers, if a different channel is already open on desktop it would choose desktop. But that's something we can live with for now.

Comment 4

[Marina Samuel [:emtwo]]

Attachment: v2: Open links in current environment

I simplified the if statement but haven't added a 'IsDesktopProcessRunning' function yet. I have a couple of points/questions first:

* I think we still need to check for !mIsRestartMetroRequest and !mIsRestartDesktopRequest in the if statements like in this patch because metrofx could be open while we try to switch to desktop and without these checks, the switch to desktop wouldn't work since we'd force metro.

* You said

> I think you'd need add a IsDesktopProcessRunning which mirrors the
> functionality of IsImmersiveProcessRunning but checks for !immersive. 

do you mean the check on this line? http://dxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CEHHelper.cpp#l78 would be for !immersive instead? If so, is that not the equivalent of the !IsImmersiveProcessRunning() check?

Comment 5

[Brian R. Bondy [:bbondy]]

> * I think we still need to check for !mIsRestartMetroRequest and !mIsRestartDesktopRequest in 
> the if statements like in this patch because metrofx could be open while we try to switch 
> to desktop and without these checks, the switch to desktop wouldn't work since we'd force metro.

Yep you're right since we don't wait for the browser to fully shutdown anymore.


In particular I mean that the new function would do the same as IsImmersiveProcessRunning but instead of doing:

> if (IsImmersiveProcessDynamic(process)) {
>   exists = true;
> }

It would do:

> if (!IsImmersiveProcessDynamic(process)) {
>   exists = true;
> }

See http://dxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CEHHelper.cpp#l96


>  If so, is that not the equivalent of the !IsImmersiveProcessRunning() check

They are not fully equivalent.
For example neither the Desktop nor the Metro browser could be running. 
In which case it would go into the "if !IsImmersiveProcessRunning()" but we wouldn't want it to.

Comment 6

[Brian R. Bondy [:bbondy]]

btw you don't need to duplicate all the code in IsImmersiveProcessRUnning, you can just pass in a parameter to it and rename it to IsProcessRunning(bool bCheckIfMetro)

Comment 7

[Marina Samuel [:emtwo]]

Attachment: v3: Open links in current environment

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 830201 [details] [diff] [review]
v3: Open links in current environment

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

Looks and works great, thanks!

By the way would you mind running mach mercurial-setup once which will effect future patches? This will setup your name and email so that they will be included in the patch files for pushing. Thanks :D

Comment 9

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/projects/oak/rev/de1eaf6e17a0

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 10

[Marina Samuel [:emtwo]]

Attachment: Open links in current environment (rebased on mc with Australis)

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Open links in current environment (rebased on mc with Australis)

Comment 12

[Brian R. Bondy [:bbondy]]

Relanded on post-australis oak:
https://hg.mozilla.org/projects/oak/rev/01d68399af78

Comment 13

[Brian R. Bondy [:bbondy]]

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Manuela Muntean [Away]]

Verified as fixed, for iteration #20, with latest Nightly on Win 8.1 Pro: link clicks from Metro always opens in the Metro browser, and link clicks from Desktop always open in the Desktop browser