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

VERIFIED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: phlsa, Assigned: emtwo)

Tracking

(Blocks: 1 bug)

28 Branch
Firefox 28
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

Created attachment 8340432 [details]
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: 942658
Whiteboard: [Australis:P1] → [Australis:P1][block28]
Blocks: 924860, 942658
No longer depends on: 942658
(Assignee)

Comment 2

4 years ago
Created attachment 8340548 [details] [diff] [review]
v1: Don't show metro button on non-windows 8

Missed wrapping these checks right here.
Attachment #8340548 - Flags: review?(netzen)

Updated

4 years ago
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)
(Assignee)

Comment 4

4 years ago
Created attachment 8340561 [details] [diff] [review]
v2: Don't show metro button on non-windows 8
Attachment #8340548 - Attachment is obsolete: true
Attachment #8340561 - Flags: review?(netzen)
Attachment #8340561 - Flags: review?(netzen) → review+

Comment 5

4 years ago
(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)

Comment 6

4 years ago
(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.

Comment 7

4 years ago
Also we need to kill off the touch bookmarks folder as well. I'm seeing it on Win7.
(Assignee)

Comment 8

4 years ago
Thanks for pointing this out, I'll make those changes.
Flags: needinfo?(msamuel)
(Assignee)

Comment 9

4 years ago
Created attachment 8341478 [details] [diff] [review]
v3: Don't show metro button on non-windows 8
Attachment #8340561 - Attachment is obsolete: true
Attachment #8341478 - Flags: review?(gijskruitbosch+bugs)

Comment 10

4 years ago
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-

Comment 11

4 years ago
(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.)

Comment 12

4 years ago
(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)

Comment 13

4 years ago
(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)
(Assignee)

Comment 14

4 years ago
Created attachment 8341920 [details] [diff] [review]
v4: Don't show metro button on non-windows 8

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 15

4 years ago
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+
(Assignee)

Comment 16

4 years ago
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
Last Resolved: 4 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

Updated

4 years ago
Priority: -- → P1

Updated

4 years ago
Depends on: 947988

Updated

4 years ago
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

Comment 19

4 years ago
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.