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

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 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! :-)

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

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 ?

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

was missing one change.

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.)

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

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

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 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.

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 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?

See also bug 947988

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.

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

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 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 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.

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 on attachment 8346056 [details] [diff] [review] backout-1b037416a394 Review of attachment 8346056 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Jared

Took out the Australis bits.

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 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