Closed Bug 924914 Opened 6 years ago Closed 6 years ago

Add "Relaunch in Windows 8 Touch (Metro)" feature to Desktop Firefox

Categories

(Firefox :: General, defect, P2)

x86_64
Windows 8.1
defect

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: bbondy, Assigned: emtwo)

References

Details

(Keywords: feature, relnote, Whiteboard: [block28][Holly][completed-oak] feature=story c=tbd u=tbd p=5 [ux-wanted])

Attachments

(6 files, 6 obsolete files)

This is dependent on UX, but my suggestion is to add a toolbar button next to the downloads one for switch to Metro. Or use the top left menu bar pre-australis and add a menu item.

To just get the core of the feature working, I'd suggest adding it as a menu item for now until UX gives more clarification.

We should make a call like this in the event handler:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroAppShell.cpp#l200

Specifying the new eRestartTouchEnvironment flag.

We also need to add support for handling that flag inside toolkit code.  The handling of that flag should simply do this:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroAppShell.cpp#l200

p=5
Looking at the v1 backlog, I found bug 915375 which I think sounds about the same as this one. Is this perhaps something we want regardless of whether we share profiles?
Flags: needinfo?(asa)
yep it's pretty much the same, but this bug is for restarting and switching, and the other one would probably just open a second instance, but in the metro environment. I think we should wait for a decision to figure out which one we should close.
See Also: → 915375
Flags: needinfo?(asa)
Summary: Work - Profile switching main task: Change View on Desktop feature to be switch to Desktop feature → Work - Profile switching main task: Add View on Metro feature to Desktop Firefox
Summary: Work - Profile switching main task: Add View on Metro feature to Desktop Firefox → Work - Profile switching second main task: Add View on Metro feature to Desktop Firefox
Sorry my last comment was incorrect. This bug just had the wrong title, it did have the correct description though.  Now the title is also fixed.
Summary: Work - Profile switching second main task: Add View on Metro feature to Desktop Firefox → Story - Profile switching second main task: Add View on Metro feature to Desktop Firefox
Whiteboard: feature=story c=tbd u=tbd p=5
Duplicate of this bug: 915375
Whiteboard: feature=story c=tbd u=tbd p=5 → feature=story c=tbd u=tbd p=5 [ux-wanted]
The current plan is to add a menu item in the orange firefox menu on the top left.
Are you OK with that? We also need a separate task for the UX branch since that menu goes away there.
Flags: needinfo?(ywang)
Assignee: nobody → msamuel
Blocks: metrov1it18
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: feature=story c=tbd u=tbd p=5 [ux-wanted] → [block28] feature=story c=tbd u=tbd p=5 [ux-wanted]
No longer blocks: 924911
Depends on: 924911
Attached patch v1: Switch to Metro (obsolete) — Splinter Review
Attachment #824712 - Flags: feedback?(netzen)
Comment on attachment 824712 [details] [diff] [review]
v1: Switch to Metro

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

Looks good at a first pass, I'd like to do another pass at it after the switch to desktop bug is done though.
I want to make sure that the profile is completely done being used at the point you are launching the metro browser, I think it is.
Attachment #824712 - Flags: feedback?(netzen) → feedback+
Comment on attachment 824712 [details] [diff] [review]
v1: Switch to Metro

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

Couple nits.

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

nit: just align this second line by 2 spaces only.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +411,5 @@
>  <!ENTITY bookmarkThisFrameCmd.label      "Bookmark This Frame">
>  <!ENTITY bookmarkThisFrameCmd.accesskey  "m">
>  <!ENTITY emailPageCmd.label           "Email Link…">
>  <!ENTITY emailPageCmd.accesskey       "E">
> +<!ENTITY switchToMetroCmd.label       "Switch to Metro">

nit: Relaunch in Windows 8 style &brandShortName

::: toolkit/components/startup/nsAppStartup.cpp
@@ +273,5 @@
>  
> +  nsresult retval = NS_OK;
> +  if (mRestart)
> +    retval = NS_SUCCESS_RESTART_APP;
> +  if (mRestartTouchEnvironment)

nit: Check mRestartTouchEnvironment first an then have else if (mRestart)
(In reply to Brian R. Bondy [:bbondy] from comment #5)
> The current plan is to add a menu item in the orange firefox menu on the top
> left.
> Are you OK with that? We also need a separate task for the UX branch since
> that menu goes away there.

Hi Brian,

I just talked to designer who work on Australis, for the new interface, it's likely that this option will be hidden in the "Customize" panel. We will need an icon for the action.

And I've been thinking about use cases for opening links from Classic to Metro. And here is a question: based on your current plan, if a users have multiple tabs open in Classic FX, will switching to Metro FX load all his tabs, or only the current one in the foreground?
Flags: needinfo?(ywang)
Flags: needinfo?(netzen)
(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #9)
> (In reply to Brian R. Bondy [:bbondy] from comment #5)
> > The current plan is to add a menu item in the orange firefox menu on the top
> > left.
> > Are you OK with that? We also need a separate task for the UX branch since
> > that menu goes away there.
> 
> Hi Brian,
> 
> I just talked to designer who work on Australis, for the new interface, it's
> likely that this option will be hidden in the "Customize" panel. We will
> need an icon for the action.

I'd like for it to be present only on windows 8, and it would be better if it was top level on the desktop browser itself (i.e. not in the customize menu). I'd also like that if the browser is not the default, when you pressed it, it would ask for default status first.  This would give us much better discoverability.

Thoughts?

> 
> And I've been thinking about use cases for opening links from Classic to
> Metro. And here is a question: based on your current plan, if a users have
> multiple tabs open in Classic FX, will switching to Metro FX load all his
> tabs, or only the current one in the foreground?

yes all of the tabs, all of the login status will also be preserved.  Also even if you have data filled out in forms that will also be preserved.
Flags: needinfo?(netzen) → needinfo?(ywang)
BTW Marina, the ux branch stuff we'll do in another bug, we're just discussing it here though :)
Also Yuan, whether we make it top level or not, please go ahead with an icon because we'll need it in either case.
(In reply to Brian R. Bondy [:bbondy] from comment #10)
> (In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #9)
> > (In reply to Brian R. Bondy [:bbondy] from comment #5)
> > > The current plan is to add a menu item in the orange firefox menu on the top
> > > left.
> > > Are you OK with that? We also need a separate task for the UX branch since
> > > that menu goes away there.
> > 
> > Hi Brian,
> > 
> > I just talked to designer who work on Australis, for the new interface, it's
> > likely that this option will be hidden in the "Customize" panel. We will
> > need an icon for the action.
> 
> I'd like for it to be present only on windows 8, and it would be better if
> it was top level on the desktop browser itself (i.e. not in the customize
> menu). I'd also like that if the browser is not the default, when you
> pressed it, it would ask for default status first.  This would give us much
> better discoverability.
> 
> Thoughts?

That's the ideal solution. I will keep pushing for that, especially if a Windows 8 version menu panel is possible.
The default choice makes sense.
 

> 
> > 
> > And I've been thinking about use cases for opening links from Classic to
> > Metro. And here is a question: based on your current plan, if a users have
> > multiple tabs open in Classic FX, will switching to Metro FX load all his
> > tabs, or only the current one in the foreground?
> 
> yes all of the tabs, all of the login status will also be preserved.  Also
> even if you have data filled out in forms that will also be preserved.

One more question: How about multiple windows? Open only tabs from the current window?


A hypothesis I have is that the No.1 scenario for switching from Class to Metro is context switching. For example, a users want to keep reading articles he saved on desktop FX when he is on the go (without keyboard attached). 

For people who have lots of tabs open on desktop, opening everything on Metro could be an over-kill.

I am thinking for this use case, it might be better to let users pick what they would like to open in Metro. Show a similar in-content page as "Restore tabs" page on FX desktop, where users can select what to open in Metro(they can also choose to open all). This panel only shows when there are lots of tabs open(over 15?)

Thoughts?
(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #13)
> (In reply to Brian R. Bondy [:bbondy] from comment #10)
> > (In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #9)
> One more question: How about multiple windows? Open only tabs from the
> current window?

It will use session restore so I think it would treat each as its own tab.

> A hypothesis I have is that the No.1 scenario for switching from Class to
> Metro is context switching. For example, a users want to keep reading
> articles he saved on desktop FX when he is on the go (without keyboard
> attached). 

Agreed.

> For people who have lots of tabs open on desktop, opening everything on
> Metro could be an over-kill.

I think we should let session restore handle this. I think when there's too many tabs it will show them as a list instead and allow you to unselect them and then restore.

> I am thinking for this use case, it might be better to let users pick what
> they would like to open in Metro. Show a similar in-content page as "Restore
> tabs" page on FX desktop, where users can select what to open in Metro(they
> can also choose to open all). This panel only shows when there are lots of
> tabs open(over 15?)
> Thoughts?

I'm not opposed to exploring this, but let's not implement it for v1. Let's look at improving that part of things for v2.

For the image for the ux branch customization panel, do you want me to post another bug? Maybe you can fight for it to be auto-promoted by saying it is only for one OS and only for win8 and 8.1. So it is only for a small percentage of the users overall.
Attached patch v2: Switch to Metro (obsolete) — Splinter Review
Attachment #824712 - Attachment is obsolete: true
Attachment #825975 - Flags: review?(netzen)
Comment on attachment 824712 [details] [diff] [review]
v1: Switch to Metro

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

::: toolkit/xre/nsAppRunner.cpp
@@ +124,5 @@
>  #include "nsAppDirectoryServiceDefs.h"
>  #include "nsXULAppAPI.h"
>  #include "nsXREDirProvider.h"
>  #include "nsToolkitCompsCID.h"
> +#include "updatehelper.h"

This needs to be inside of an ifdef:
#if defined(XP_WIN)
#include "updatehelper.h"
#endif

It's only exported on Windows.

@@ +4049,5 @@
>  #endif
>  
>    // Check for an application initiated restart.  This is one that
>    // corresponds to nsIAppStartup.quit(eRestart)
> +  if (rv == NS_SUCCESS_RESTART_APP || rv == NS_SUCCESS_RESTART_METRO_APP) {

It's ok to leave this out of the ifdef as long as you keep it defined everywhere.

@@ +4054,3 @@
>      appInitiatedRestart = true;
> +    if (rv == NS_SUCCESS_RESTART_METRO_APP) {
> +      LaunchDefaultMetroBrowser();

ditto: This should be inside an:
#if defined(MOZ_METRO) && defined(XP_WIN)
if (rv == NS_SUCCESS_RESTART_METRO_APP) {
		LaunchDefaultMetroBrowser();
}
#endif
Attachment #824712 - Attachment is obsolete: false
Comment on attachment 825975 [details] [diff] [review]
v2: Switch to Metro

Oops, I missed the new comments before uploading this
Attachment #825975 - Flags: review?(netzen)
Attached patch Patch 1 - Switch to Metro. rev3. (obsolete) — Splinter Review
Attachment #824712 - Attachment is obsolete: true
Attachment #825975 - Attachment is obsolete: true
Attachment #825991 - Flags: review?(netzen)
Attachment #825991 - Flags: review?(netzen) → review+
https://hg.mozilla.org/projects/oak/rev/deb77824578d
Whiteboard: [block28] feature=story c=tbd u=tbd p=5 [ux-wanted] → [block28][completed-oak] feature=story c=tbd u=tbd p=5 [ux-wanted]
Attached image relaunch_in_windows8.png (obsolete) —
A separate bug will be posted or the UX branch once an icon is received.
Attachment #826183 - Flags: ui-review?(ywang)
Thanks, Marina. It looks good to me.
I will talk to Firefox UX team on Tuesday to finalize the copy. Will keep you posted.
Flags: needinfo?(ywang)
Comment on attachment 825991 [details] [diff] [review]
Patch 1 - Switch to Metro. rev3.

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

::: toolkit/xre/nsAppRunner.cpp
@@ +125,5 @@
>  #include "nsXULAppAPI.h"
>  #include "nsXREDirProvider.h"
>  #include "nsToolkitCompsCID.h"
>  
> +#if defined(XP_WIN)

Looks like we need:
#if defined(XP_WIN) && defined(MOZ_METRO) here
https://tbpl.mozilla.org/?tree=Oak&rev=a384228760da
B2G win builds are failing otherwise.
I'll fix on oak, could you upload a new patch for here? Thanks.
Marina ignore comment 22, I'll post a patch here.

Landed on oak here:
https://hg.mozilla.org/projects/oak/rev/deb77824578d
https://hg.mozilla.org/projects/oak/rev/2569b7424bbd

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
Blocks: 935099
Attached patch Patch 2 - Fix for include guard (obsolete) — Splinter Review
The main patch had a build problem on oak with b2g windows builds. This fixes that build problem.
Attachment #827528 - Flags: review?(jmathies)
Attachment #825991 - Attachment description: v3: Switch to Metro → Patch 1 - Switch to Metro. rev3.
Attachment #827528 - Flags: review?(jmathies) → review+
Comment on attachment 825991 [details] [diff] [review]
Patch 1 - Switch to Metro. rev3.

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +411,5 @@
>  <!ENTITY bookmarkThisFrameCmd.label      "Bookmark This Frame">
>  <!ENTITY bookmarkThisFrameCmd.accesskey  "m">
>  <!ENTITY emailPageCmd.label           "Email Link…">
>  <!ENTITY emailPageCmd.accesskey       "E">
> +<!ENTITY switchToMetroCmd.label       "Relaunch in Windows 8 style &brandShortName;">

drive by comment - "Relaunch in" seems odd to me, maybe use "Switch to"?
Verified as fixed for iteration #18, on Win 8 64-bit, with the latest oak build (build ID: 20131114040200).

When in Desktop mode and pressing the "Relaunch in Windows 8 style Nightly" button, the Metro mode is launched with the exact tabs opened as in the Desktop mode.

Also, when in Metro mode and pressing the "Relaunch in Desktop" button, the Desktop mode is launched with the exact tabs opened as in the Metro mode.
Comment on attachment 826183 [details]
relaunch_in_windows8.png

This is obsolute since we're using the UX design decided on in bug 934029
Attachment #826183 - Attachment is obsolete: true
Attachment #826183 - Flags: ui-review?(ywang)
Comment on attachment 825991 [details] [diff] [review]
Patch 1 - Switch to Metro. rev3.

emtwo mentioned all of this is included in the Australis path, so obsoleting this.
Attachment #825991 - Attachment is obsolete: true
Comment on attachment 827528 [details] [diff] [review]
Patch 2 - Fix for include guard

Included in australis bug.
Attachment #827528 - Attachment is obsolete: true
Patches included in bug 934032, nothing in this bug will land on m-c because astralis landed before us.
Resolution: FIXED → WORKSFORME
Attached patch v4: Switch to Metro (obsolete) — Splinter Review
Patch for pre-australis on Holly.
Re-opening to land patch in Holly
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Whiteboard: [block28][completed-oak] feature=story c=tbd u=tbd p=5 [ux-wanted] → [block28][Holly][completed-oak] feature=story c=tbd u=tbd p=5 [ux-wanted]
Comment on attachment 8334744 [details] [diff] [review]
v4: Switch to Metro

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

::: toolkit/xre/nsAppRunner.cpp
@@ +4057,5 @@
>      appInitiatedRestart = true;
>  
> +#if defined(MOZ_METRO) && defined(XP_WIN)
> +    if (rv == NS_SUCCESS_RESTART_METRO_APP) {
> +      LaunchDefaultMetroBrowser();

Is this code that will already be included in the UX branch patch?
I think so, we need to separate out only the parts for Holly that won't be merged in from m-c.

Ditto other changes in this file.
Moving to fixed since it's already on oak, but we still need to split up the previous patch a per the last comment.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
This patch, or some form close to it will be landing on Holly only. It adds a "Relaunch in Metro" feature to Desktop Firefox's top left menu.  It has a l10n string in it.  Is there anything special we need to do to make sure it will get seen and localized before Holly goes to aurora?  It's possible there are other bugs from other people that have similar UI string changes that apply to Holly only, I just wanted to make sure this edge case is covered.
Flags: needinfo?(jaws)
Flags: needinfo?(gavin.sharp)
(In reply to Brian R. Bondy [:bbondy] from comment #35)
> This patch, or some form close to it will be landing on Holly only. It adds
> a "Relaunch in Metro" feature to Desktop Firefox's top left menu.  It has a
> l10n string in it.  Is there anything special we need to do to make sure it
> will get seen and localized before Holly goes to aurora?  It's possible
> there are other bugs from other people that have similar UI string changes
> that apply to Holly only, I just wanted to make sure this edge case is
> covered.

Please land the strings that you need on m-c with a separate patch and that patch will be merged over to Holly. The rest of your patch can land directly on Holly, but the strings need to be introduced on m-c.
Flags: needinfo?(jaws)
Flags: needinfo?(gavin.sharp)
This patch does not include the string or common code in mc
Attachment #8334744 - Attachment is obsolete: true
Attached patch Added stringSplinter Review
patch for mc with just the added string
Attachment #8339548 - Attachment description: v5: Switch to Metro → [Holly only] v5: Switch to Metro
Attachment #8339549 - Flags: review+
Attachment #8339548 - Flags: review+
Attached image screenshot.png
For iteration #19, I've tried testing this with the latest Holly build installer from ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/holly-win32/1385580442/ , build ID: 20131127074557, on Win 8 64bit, but I couldn't find the "Relaunch in Windows 8 style Nightly" button, as shown in the attached screenshot.

I assume this hasn't been landed on Holly yet.
Flags: needinfo?(netzen)
This work is only on oak right now, it'll be moving soon though.
Flags: needinfo?(netzen)
(In reply to Brian R. Bondy [:bbondy] from comment #40)
> This work is only on oak right now, it'll be moving soon though.

I didn't find the "Relaunch in Windows 8 style Nightly" button, with the latest oak build, from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-oak/ (the oak build comes with Australis)
It'll be in the customization panel for now
Indeed, I could see the button (as shown in the screenshot), after customizing a little. Thanks!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
https://hg.mozilla.org/integration/fx-team/rev/514b65ab771d
Target Milestone: --- → Firefox 28
Blocks: 944563
https://hg.mozilla.org/mozilla-central/rev/514b65ab771d
Status: NEW → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
This patch lightens the icons that on the Panel Menu that are placed on the various toolbars such as the navigation bar, bookmarks bar, etc.

Regression
----------

Good:  20131128150519
http://hg.mozilla.org/integration/fx-team/rev/5a6373ab5fe6

Bad: 20131128171419
http://hg.mozilla.org/integration/fx-team/rev/514b65ab771d


Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Also lightens the Back/Forward buttons.
Not this bug, but bug 934032.
I'll post this info in another new bug and mark it as block bug 934032.
(In reply to Brian R. Bondy [:bbondy] from comment #50)
> Not this bug, but bug 934032.
> I'll post this info in another new bug and mark it as block bug 934032.

I thought that the problem didn't seem to fit this patch even though my regression testing pointed here. Where did I go wrong in regression testing so I don't do it again?
there were 14 patches landed in 1 push. The bug I pointed to was in the same push as this bug was.
I see now. Shouldn't happen again.
No worries, thanks for finding the regression range! :)
Since the new name is finalized, we should use "Relaunch in *Brand* for Windows 8 Touch" for non-Australis desktop UI.
Marina, could you see to it that the strings are updated on Holly and if any are left on m-c?
Verified as fixed, for iteration #19, on Win 8 64-bit, with latest Holly build 2013-12-06.

In desktop mode, I can see the "Relaunch in Windows 8 style Holly" menu item, that launches Metro mode.
Duplicate of this bug: 849690
Depends on: 950986
Adding this to the query on https://wiki.mozilla.org/Features/Release_Tracking
Keywords: feature, relnote
Product: Firefox for Metro → Firefox
Summary: Story - Profile switching second main task: Add View on Metro feature to Desktop Firefox → Add "Relaunch in Windows 8 Touch (Metro)" feature to Desktop Firefox
Version: unspecified → Trunk
Depends on: 968317
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.