Closed
Bug 932664
Opened 11 years ago
Closed 11 years ago
Story - Link clicks and file activations should open in the currently opened browser no matter which environment
Categories
(Firefox for Metro Graveyard :: General, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 28
People
(Reporter: bbondy, Assigned: emtwo)
References
Details
(Whiteboard: [completed-oak][block28][completed-oak] feature=story c=tbd u=tbd p=1)
Attachments
(1 file, 4 obsolete files)
4.78 KB,
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
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!)
Updated•11 years ago
|
Whiteboard: [block28] feature=story c=tbd u=tbd p=1
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → msamuel
Updated•11 years ago
|
Reporter | ||
Comment 2•11 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•11 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•11 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•11 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•11 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)
Updated•11 years ago
|
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #827551 -
Attachment is obsolete: true
Attachment #830201 -
Flags: review?(netzen)
Reporter | ||
Comment 8•11 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•11 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: 11 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
Assignee | ||
Comment 10•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Attachment #830201 -
Attachment is obsolete: true
Reporter | ||
Comment 11•11 years ago
|
||
Attachment #8333976 -
Attachment is obsolete: true
Attachment #8334649 -
Flags: review+
Reporter | ||
Comment 12•11 years ago
|
||
Relanded on post-australis oak: https://hg.mozilla.org/projects/oak/rev/01d68399af78
Reporter | ||
Updated•11 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•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Updated•11 years ago
|
Status: REOPENED → NEW
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ef5ef338cfcf
Status: NEW → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 15•11 years ago
|
||
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.
Description
•