945842 - "Firefox Touch" Folder should only be for Win8

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Comment 1

[Marina Samuel [:emtwo]]

Bug 939092 did not prevent this folder from being created on other Windows versions.

Comment 2

[Marina Samuel [:emtwo]]

Attachment: v1: Show Metro bookmarks folder in Windows 8 only

Hi Gijs, this patch is working as expected but the function isWin8OrHigher() is duplicated code, both in the two files here and in CustomizableWidgets.jsm. Do you know if there is a universal spot I can put this function or some way to avoid the code duplication? Thanks!

Comment 3

[:Gijs (he/him)]

Comment on attachment 8343284 [details] [diff] [review]
v1: Show Metro bookmarks folder in Windows 8 only

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

Actually, I would think adding it to the sysinfo interface is probably the most sensible... something like "hasWindowsTouchInterface" (where really I would have preferred "hasMetro" but I hear we can't call it that anymore?). That could cache the value as well, making it faster, too! :-)

Comment 4

[Marina Samuel [:emtwo]]

Attachment: v2: Show Metro bookmarks folder in Windows 8 only

Added a sysinfo property.

Still need to test/run a try build but seems to be working as expected.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8343802 [details] [diff] [review]
v2: Show Metro bookmarks folder in Windows 8 only

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

I'm not a good reviewer for the nsSystemInfo bits; you should get from an XPCOM person. However, generally this looks alright, although to store a boolean value you should probably be using SetPropertyAsBool rather than the string variant. That'll simplify the reading of the property as well, as you can just leave out the === "true" bits everywhere.

::: browser/components/nsBrowserGlue.js
@@ +1657,5 @@
> +        };
> +
> +        if (Services.sysinfo.getProperty("hasWindowsTouchInterface") === "true") {
> +          smartBookmarks.Windows8Touch =
> +          {

Nit: brace on the previous line

::: xpcom/base/nsSystemInfo.cpp
@@ +185,5 @@
> +    double versionDouble = atof(NS_ConvertUTF16toUTF8(version).get());
> +
> +    rv = SetPropertyAsACString(NS_ConvertASCIItoUTF16("hasWindowsTouchInterface"),
> +      nsDependentCString(osName.EqualsASCII("Windows_NT") && versionDouble >= 6.2 ?
> +      "true" : "false"));

I am totally not qualified to review these bits, I'm afraid. However, I *can* tell you that, nit: you should probably be checking rv for success here.

Separately, I see that you're using strings here... why not use SetPropertyAsBool ?

Comment 6

[Marina Samuel [:emtwo]]

Attachment: v3: Show Metro bookmarks folder in Windows 8 only

Hi Aaron, would you be able to review the nsSystemInfo.cpp changes in this patch please? Thanks!

Comment 7

[Marina Samuel [:emtwo]]

Attachment: v4: Show Metro bookmarks folder in Windows 8 only

was missing one change.

Comment 8

[Matt Brubeck (:mbrubeck)]

Nominating for tracking-firefox28 since we would like to get this landed before Aurora 28 is unthrottled.  (Otherwise, users of Win7 and earlier may update to Aurora 28 builds without this fix and will end up with the Metro bookmarks folder added to their bookmarks.)

Comment 9

[Marina Samuel [:emtwo]]

A try build from the patch in comment 4:
https://tbpl.mozilla.org/?tree=Try&rev=64e16610e17f

Comment 10

[Marina Samuel [:emtwo]]

A try build from the patch in comment 4:
https://tbpl.mozilla.org/?tree=Try&rev=64e16610e17f

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 8343892 [details] [diff] [review]
v4: Show Metro bookmarks folder in Windows 8 only

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

::: xpcom/base/nsSystemInfo.cpp
@@ +185,5 @@
> +    double versionDouble = atof(NS_ConvertUTF16toUTF8(version).get());
> +
> +    rv = SetPropertyAsBool(NS_ConvertASCIItoUTF16("hasWindowsTouchInterface"),
> +      osName.EqualsASCII("Windows_NT") && versionDouble >= 6.2);
> +    NS_ENSURE_SUCCESS(rv, rv);

I think we want to check for this property not only that it's win 8 but also that it's a win8 enabled build.
So I think we should be storing false if either of XP_WIN or MOZ_METRO is not defined.

Also we don't need any of the code for the os name since we can just put that inside an XP_WIN define check.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8343892 [details] [diff] [review]
v4: Show Metro bookmarks folder in Windows 8 only

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

This code makes a property called "hasWindowsTouchInterface", but it's really checking for >=Win8 and the code using it stores the result in a variable named that way. What's the point? 

The current code will also set that variable on non-Windows builds.

Comment 13

[Brian R. Bondy [:bbondy]]

Having such a property has some value, for example it would simplify and remove ugly ifdefs in code like this:
http://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/advanced.js#48

Comment 14

[Marina Samuel [:emtwo]]

Attachment: v5: Show Metro bookmarks folder in Windows 8 only

Comment 15

[Brian R. Bondy [:bbondy]]

Comment on attachment 8344766 [details] [diff] [review]
v5: Show Metro bookmarks folder in Windows 8 only

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

::: browser/components/nsBrowserGlue.js
@@ +1591,5 @@
>      // be set to the version it has been added in, we will compare its value
>      // to users' smartBookmarksVersion and add new smart bookmarks without
>      // recreating old deleted ones.
> +    const SMART_BOOKMARKS_VERSION =
> +      Services.sysinfo.getProperty("hasWindowsTouchInterface") ? 5 : 4;

Can we just leave this at one number? Maybe 6?

@@ +1670,5 @@
>              parent: PlacesUtils.bookmarksMenuFolderId,
>              position: menuIndex++,
>              newInVersion: 5
> +          }
> +        }

semicolon

::: browser/components/places/tests/unit/head_bookmarks.js
@@ +63,5 @@
>  }
>  
>  // Smart bookmarks constants.
> +let isWin8OrHigher = Services.sysinfo.getProperty("hasWindowsTouchInterface");
> +const SMART_BOOKMARKS_VERSION = isWin8OrHigher ? 5 : 4;

ditto: why do we need a specific version number difference here?

Comment 16

[Brian R. Bondy [:bbondy]]

See also bug 947988

Comment 17

[Marina Samuel [:emtwo]]

Attachment: v6: Show Metro bookmarks folder in Windows 8 only

I think those version checks might be useful to avoid the scenario where someone is using a Mac and they are on version 6, they won't have a windows button. But if somehow they copy over or sync their pref that has the version as 6 to a win8 machine, it may not show the metro folder since there would be no version update.

I believe the opposite is true too. If you are on a win8 machine and have the metro folder on version 6, there would be no version change if you switched to a Mac and I think the folder would remain.

However, even if we addressed this versioning of the folders issue, I think sync may still copy that folder over, depending on how it works.

I do think these are edge cases and I can file a separate bug for them. I only had it that way before because it helped while testing.

Comment 18

[Brian R. Bondy [:bbondy]]

Do we sync that pref? I don't think we need to worry about or support copying the profile over different OS.

Comment 19

[Marina Samuel [:emtwo]]

I just tried to change that pref and perform a sync and it looks like it doesn't get synced so I think this should be ok.

Comment 20

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/1b037416a394

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b037416a394

Comment 22

[Marina Samuel [:emtwo]]

Comment on attachment 8344840 [details] [diff] [review]
v6: Show Metro bookmarks folder in Windows 8 only

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 939092
User impact if declined: Users on non-Win8 machines will see a Metro-specific bookmarks folder which can be confusing.
Testing completed (on m-c, etc.): Tested mc build from https://tbpl.mozilla.org/?rev=e15cbe188aec to contain the Metro folder on a win8 machine and to not contain it on a Mac
Risk to taking this patch (and alternatives if risky): Low risk, it passed all tests on try/fx-team/mc and the manual testing and changes don't affect code besides what's being tested.
String or IDL/UUID changes made by this patch: none

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8344840 [details] [diff] [review]
v6: Show Metro bookmarks folder in Windows 8 only

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

This patch cannot move to Aurora as-is as it touches a non-existant CustomizableWidgets.jsm file on mozilla-aurora.

Since this patch touched an Australis file, the commit message should have included "[Australis]" in the message so it would be backed out upon merging to Holly. From looking at the patch though, it seems that you may have wanted to keep the other changes in Holly/Aurora. I will put up a patch for your review that will back out the CustomizableWidgets.jsm changes for Holly/Aurora.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: backout-1b037416a394

This patch undoes the changes to CustomizableWidgets.jsm so it can be properly backed out on Holly.

Please note that you will have to update your Aurora patch to not include the CustomizableWidgets.jsm changes.

Comment 25

[Marina Samuel [:emtwo]]

Comment on attachment 8346056 [details] [diff] [review]
backout-1b037416a394

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

Thanks Jared

Comment 26

[Marina Samuel [:emtwo]]

Attachment: v7: Show Metro bookmarks folder in Windows 8 only (for Aurora)

Took out the Australis bits.

Comment 27

[Marina Samuel [:emtwo]]

Attachment: v7: Show Metro bookmarks folder in Windows 8 only (for Aurora)

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8346080 [details] [diff] [review]
v7: Show Metro bookmarks folder in Windows 8 only (for Aurora)

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

rs=me as long as this is the same patch that bbondy r+'d. I see that this patch no longer includes the CustomizableWidgets.jsm changes. Thanks!

Comment 29

[Marina Samuel [:emtwo]]

Comment on attachment 8346080 [details] [diff] [review]
v7: Show Metro bookmarks folder in Windows 8 only (for Aurora)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 939092
User impact if declined: Users on non-Win8 machines will see a Metro-specific bookmarks folder which can be confusing.
Testing completed (on m-c, etc.): Tested mc build from https://tbpl.mozilla.org/?rev=e15cbe188aec to contain the Metro folder on a win8 machine and to not contain it on a Mac
Risk to taking this patch (and alternatives if risky): Low risk, it passed all tests on try/fx-team/mc and the manual testing and changes don't affect code besides what's being tested.
String or IDL/UUID changes made by this patch: none

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ce333a03499