Closed Bug 944740 Opened 11 years ago Closed 11 years ago

[Australis] Don't show »Metro Mode« button on non-Windows-8 platforms

Categories

(Firefox :: Toolbars and Customization, defect, P1)

28 Branch
x86
All
defect

Tracking

()

VERIFIED FIXED
Firefox 28

People

(Reporter: phlsa, Assigned: emtwo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P1][block28] feature=defect c=tbd u=tbd p=1)

Attachments

(2 files, 3 obsolete files)

Attached image Metro button on OS X
The Metro Mode button only makes sense on Windows 8 and should not be displayed on other platforms and older Windows versions.
Summary: [Australis] [Mac] Don't show »Metro Mode« button on non-Windows-8 platforms → [Australis] Don't show »Metro Mode« button on non-Windows-8 platforms
Whiteboard: [Australis:P1]
There's some code to hide it, but I guess there's a bug in it.
p=1
Assignee: nobody → msamuel
Status: NEW → ASSIGNED
Depends on: metrov1it20
Whiteboard: [Australis:P1] → [Australis:P1][block28]
No longer depends on: metrov1it20
Missed wrapping these checks right here.
Attachment #8340548 - Flags: review?(netzen)
Whiteboard: [Australis:P1][block28] → [Australis:P1][block28] feature=defect c=tbd u=tbd p=1
Comment on attachment 8340548 [details] [diff] [review]
v1: Don't show metro button on non-windows 8

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

Oops I think on non windows platforms you'll have 2:
 }, {

in a row
Attachment #8340548 - Flags: review?(netzen)
Attachment #8340548 - Attachment is obsolete: true
Attachment #8340561 - Flags: review?(netzen)
Attachment #8340561 - Flags: review?(netzen) → review+
(In reply to Marina Samuel [:emtwo] from comment #4)
> Created attachment 8340561 [details] [diff] [review]
> v2: Don't show metro button on non-windows 8

How does this patch stop the button showing up on Win7 and other earlier Windows versions? Presumably when running nightly builds MOZ_METRO was defined at build time, so the button will be in the widgets file. Looks to me like you should add it dynamically at runtime rather than putting things behind #ifdefs.
Flags: needinfo?(msamuel)
(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Marina Samuel [:emtwo] from comment #4)
> > Created attachment 8340561 [details] [diff] [review]
> > v2: Don't show metro button on non-windows 8
> 
> How does this patch stop the button showing up on Win7 and other earlier
> Windows versions? Presumably when running nightly builds MOZ_METRO was
> defined at build time, so the button will be in the widgets file. Looks to
> me like you should add it dynamically at runtime rather than putting things
> behind #ifdefs.

It doesn't. We'll need to incorporate an os version check in here as well somehow.
Also we need to kill off the touch bookmarks folder as well. I'm seeing it on Win7.
Thanks for pointing this out, I'll make those changes.
Flags: needinfo?(msamuel)
Attachment #8340561 - Attachment is obsolete: true
Attachment #8341478 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8341478 [details] [diff] [review]
v3: Don't show metro button on non-windows 8

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

r- because this check should really be in CustomizableWidgets.jsm, behind an XP_WIN ifdef as it is now, but with the version check added.

Note also that because you're changing when this item is in the panel (used to be always on Windows, now only on Win8), some of the tests you adapted in the earlier bug will start failing in the new situation. You should check that once you have a new patch, the patch passes all the CustomizableUI tests on a Win7 or WinXP machine, and then probably push to try to be safe.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +212,5 @@
>      });
>    },
>  
> +  _isInWin8: function() {
> +    let sysInfo = Services.sysinfo;

Nit: No need for the intermediate.

@@ +217,5 @@
> +    let osName = sysInfo.getProperty("name");
> +    let version = sysInfo.getProperty("version");
> +
> +    // Windows 8 is version >= 6.2
> +    return osName == "Windows_NT" && version >= 6.2;

Versions aren't really numbers (and the getProperty call returns a string). While this probably works, the correct way to do this is checking the result of:

Services.vc.compare(version, "6.2")

See: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIVersionComparator

@@ +224,5 @@
>    _defineBuiltInWidgets: function() {
>      //XXXunf Need to figure out how to auto-add new builtin widgets in new
>      //       app versions to already customized areas.
>      for (let widgetDefinition of CustomizableWidgets) {
> +      if (widgetDefinition.id == "switch-to-metro-button" && !this._isInWin8()) {

Adding if statements into this loop is really icky. Please just take the item out of the fixed array in CustomizableWidgets.jsm, and .push() it on iff we're on Windows 8. Then that check only needs to happen once (instead of twice, like here), and for the default placements you can just check if gPalette.has("switch-to-metro-button").
Attachment #8341478 - Flags: review?(gijskruitbosch+bugs) → review-
(Also, naming nit: you're naming the utility function "isInWin8", but it'll return true for win > 8. So please update the naming to reflect that.)
(In reply to Jim Mathies [:jimm] from comment #7)
> Also we need to kill off the touch bookmarks folder as well. I'm seeing it
> on Win7.

Is this related or should this be its own bug?
Flags: needinfo?(jmathies)
(In reply to :Gijs Kruitbosch from comment #12)
> (In reply to Jim Mathies [:jimm] from comment #7)
> > Also we need to kill off the touch bookmarks folder as well. I'm seeing it
> > on Win7.
> 
> Is this related or should this be its own bug?

Should probably be in a separate bug.
Flags: needinfo?(jmathies)
Addressed comments. The tests were fine because all the tests were checking for the metro button existing in the menu panel and the code was correctly only placing the button in the panel in win8. This bug is just for ensuring it doesn't appear in the palette when not in win8.

try build:
https://tbpl.mozilla.org/?tree=Try&rev=42518adea04b
Attachment #8341478 - Attachment is obsolete: true
Attachment #8341920 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8341920 [details] [diff] [review]
v4: Don't show metro button on non-windows 8

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

Looks good, thanks!
Attachment #8341920 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/integration/fx-team/rev/03a55dd19083
Whiteboard: [Australis:P1][block28] feature=defect c=tbd u=tbd p=1 → [fixed-in-fx-team][Australis:P1][block28] feature=defect c=tbd u=tbd p=1
https://hg.mozilla.org/mozilla-central/rev/03a55dd19083
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][Australis:P1][block28] feature=defect c=tbd u=tbd p=1 → [Australis:P1][block28] feature=defect c=tbd u=tbd p=1
Target Milestone: --- → Firefox 28
Priority: -- → P1
Depends on: 947988
QA Contact: jbecerra
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.1; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 5.2; rv:28.0) Gecko/20100101 Firefox/28.0

Verified on latest nightly (build ID: 20131209053402).
The "metro mode" button is no longer shown on Windows 7, Windows XP, Ubuntu 12.04 and Mac OS X 10.8.5.
Status: RESOLVED → VERIFIED
Argh, the button also hides on Windows 8.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: