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

VERIFIED FIXED in Firefox 28

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: bbondy, Assigned: emtwo)

Tracking

unspecified
Firefox 28
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [completed-oak][block28][completed-oak] feature=story c=tbd u=tbd p=1)

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

6 years ago
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!)
Whiteboard: [block28] feature=story c=tbd u=tbd p=1
Assignee

Updated

6 years ago
Assignee: nobody → msamuel
Blocks: metrov1it18
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Assignee

Comment 1

6 years ago
Attachment #826750 - Flags: review?(netzen)
Reporter

Comment 2

6 years ago
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
Attachment #826750 - Flags: review?(netzen) → review-
Reporter

Comment 3

6 years ago
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.
Assignee

Comment 4

6 years ago
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?
Attachment #826750 - Attachment is obsolete: true
Flags: needinfo?(netzen)
Reporter

Comment 5

6 years ago
> * 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.
Flags: needinfo?(netzen)
Reporter

Comment 6

6 years ago
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)
Blocks: metrov1it19
No longer blocks: metrov1it18
Assignee

Comment 7

6 years ago
Attachment #827551 - Attachment is obsolete: true
Attachment #830201 - Flags: review?(netzen)
Reporter

Comment 8

6 years ago
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
Attachment #830201 - Flags: review?(netzen) → review+
Reporter

Comment 9

6 years ago
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [block28] feature=story c=tbd u=tbd p=1 → [block28][completed-oak] feature=story c=tbd u=tbd p=1
Target Milestone: --- → Firefox 28
Reporter

Updated

6 years ago
Blocks: 935099
Reporter

Updated

6 years ago
Attachment #830201 - Attachment is obsolete: true
Reporter

Comment 12

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

Updated

6 years ago
Whiteboard: [block28][completed-oak] feature=story c=tbd u=tbd p=1 → [completed-oak][block28][completed-oak] feature=story c=tbd u=tbd p=1
Reporter

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter

Updated

6 years ago
Status: REOPENED → NEW
https://hg.mozilla.org/mozilla-central/rev/ef5ef338cfcf
Status: NEW → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.