961587 - new profile has "Windows 8 Touch" bookmark folder

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Bookmarks, defect, P1)

Description

[Cameron McCormack (:heycam)]

Attachment: screen shot

In a new profile, within the bookmarks menu is a folder named "Windows 8 Touch" that contains the usual default bookmarks that the "Mozilla Firefox" folder does.  I don't think we should have such a folder.

See the attached screen shot.

Comment 1

[Matt Brubeck (:mbrubeck)]

The folder is added intentionally when upgrading to a Metro-enabled Firefox build, as part of bug 939092.  However, it should be empty until you have added bookmarks while running in "Windows 8 Touch" mode. (Perhaps we should not create the folder at all until such a bookmark has been added...)  It's definitely a bug that it contains all the default bookmarks.

Comment 2

[Marina Samuel [:emtwo]]

Hi Cameron, I've seen a similar issue to what you saw but a while back, bug 945842. Are you using Windows 8? What version of Firefox did you spot the issue on? It's possible that you had a build with that bug before it was fixed but the fix didn't remove an existing folder with those bookmarks. Thanks!

Comment 3

[Cameron McCormack (:heycam)]

Yes it was on Windows 8.  That was a nightly Firefox which would have been from around the time I filed this bug.  I've had that Nightly installed and updating infrequently.  It's possible I guess that a run of an older build caused the newer build (even with a freshly created profile) to show the bad bookmarks?

Comment 4

[Marina Samuel [:emtwo]]

Attachment: v1: Only create Windows 8 Touch smart bookmark once a user has intialized their Metro bookmark list.

Hi Marco, I'm not sure who would be the best reviewer for this, but bugzilla suggested you as a reviewer :) Would you mind taking a look please?  Feel free to re-assign if there is a better candidate. Thanks!

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8381769 [details] [diff] [review]
v1: Only create Windows 8 Touch smart bookmark once a user has intialized their Metro bookmark list.

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

Sorry I figure the comments below may be a little bit unclear, so I'll try to sum up:
- bump version to 7
- convert uri: to a getter in the metro query, it will return a valid uri only if conditions are satisfied (metroRoot exists)
- use smartBookmark.uri validity to decide whether to remove an existing query or to create a new one
- create the smart bookmark when you create the root, in metro code (since this code can't dinamically create one on a condition happening)

::: browser/components/nsBrowserGlue.js
@@ +1624,5 @@
>      // When adding a new smart bookmark below, its newInVersion property must
>      // 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.
> +    let SMART_BOOKMARKS_VERSION = 6;

sorry, but I don't like the version changing dinamically, what happens when we introduce version 8 and further? it will be confusing and error-prone.

I'd suggest to just bump the version to 7 here and then modify the queries definition later

@@ +1647,5 @@
> +
> +    // The Windows 8 Touch Folder should only show up after a user has opened MetroFx at least once.
> +    if (metroBookmarksRoot.length > 0 && !hasWin8SmartBookmark) {
> +      SMART_BOOKMARKS_VERSION = 7;
> +    }

rather than doing this hack, you may create the smart bookmark in the same code point where you create the root. The important part is that you annotate it with "Places/SmartBookmark".

you should still add it to the smart queries list, as it is now, since this list is used to maintain and update the queries in future version bumps, so if it would be missing we'd consider it dead and un-annotate it.

@@ +1713,5 @@
>                                  "&maxResults=" + MAX_RESULTS +
>                                  "&excludeQueries=1"),
>              parent: PlacesUtils.bookmarksMenuFolderId,
>              position: menuIndex++,
> +            newInVersion: 7

you should not remove it from the list based on metroBookmarksRoot.length > 0, otherwise the code later will find the smart bookmark in existing profiles who hit this bug, won't find it in the list, and will un-annotate it.
What you may do is leave the code here as-is, make uri a getter, if it can't find the metro root it should return null. Later in the for loop that creates the queries check smartBookmark.uri, if it's null just continue to the next query.

as a bonus, you may also use the same method to remove the old bogus queries. In the smartBookmarkItemIds.forEach loop, check smartBookmark.uri, if it's invalid remove the existing item, we know the old version can't be valid if the new is not valid.

Comment 6

[Marina Samuel [:emtwo]]

Attachment: v2: Only create Windows 8 Touch smart bookmark once a user has intialized their Metro bookmark list.

This is my understanding of your review. I agree this seems a lot better, thanks!

Comment 7

[Marina Samuel [:emtwo]]

I'll get a different reviewer for the Metro bit once you confirm this looks like what you had in mind, Marco

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8383332 [details] [diff] [review]
v2: Only create Windows 8 Touch smart bookmark once a user has intialized their Metro bookmark list.

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

yes, more or less, some comments following

::: browser/components/nsBrowserGlue.js
@@ +1691,5 @@
>              newInVersion: 1
>            },
>          };
>  
> +        let metroBookmarksRoot = PlacesUtils.annotations.getItemsWithAnnotation('metro/bookmarksRoot', {});

you don't need to do this here...

@@ +1696,4 @@
>          if (Services.sysinfo.getProperty("hasWindowsTouchInterface")) {
>            smartBookmarks.Windows8Touch = {
>              title: bundle.GetStringFromName("windows8TouchTitle"),
> +            uri: metroBookmarksRoot.length > 0 ?

...rather here make

get uri() {
  let metroBookmarksRoot...
  if (root) {
    return newURI
  }
  return null;
}

@@ +1708,2 @@
>              parent: PlacesUtils.bookmarksMenuFolderId,
> +            position: PlacesUtils.bookmarks.DEFAULT_INDEX,

this will append-to/move-to-the-end-of the bookmarks menu, is it an expected change? it will also move existing queries, if the menu is long it may end up out of view.

@@ +1724,5 @@
>              smartBookmark.position = PlacesUtils.bookmarks.getItemIndex(itemId);
> +            if (!smartBookmark.uri) {
> +              PlacesUtils.bookmarks.removeItem(smartBookmark.itemId);
> +              delete smartBookmarks.queryId;
> +            }

move this just after smartBookmark definition, and return inside the if, we don't need to do all of the rest (you will have to use itemId instead of smartBookmark.itemId, clearly).
On the other side what may happen if here you keep the previous index, is that in future the query could be moved to the position specified here. I'm fine either way, your call.

::: browser/metro/components/BrowserStartup.js
@@ +73,5 @@
> +      yield BookmarkJSONUtils.importFromURL("chrome://browser/locale/bookmarks.json", false);
> +
> +      // Create the new smart bookmark.
> +      const MAX_RESULTS = 10;
> +      let bundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");

you should not need to add the string to a further bundle, you may use PlacesUIUtils.getString("windows8TouchTitle") (notice this is a different module from PlacesUtils), otherwise you should remove the old string from places.properties and make nsBrowserGlue use the new string in browser.properties... In the end you should have the string only in one place.
The choice depends on hwo much metro browser code is trying to detach from the other components in browser.

Comment 9

[Marco Bonardo [:mak]]

> On the other side what may happen if here you keep the previous index, is that in
> future the query could be moved to the position specified here. I'm fine either 
> way, your call.

This comment was referred to the DEFAULT_INDEX change , I wrongly posted it in the next section :(

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to Marco Bonardo [:mak] from comment #8)
> you should not need to add the string to a further bundle, you may use
> PlacesUIUtils.getString("windows8TouchTitle") (notice this is a different
> module from PlacesUtils), otherwise you should remove the old string from
> places.properties and make nsBrowserGlue use the new string in
> browser.properties...

Metro Firefox doesn't include code from /browser/components or /browser/locales (it's a separate app, like Fennec), so we can't have a shared string there.

If we want to share the string, we could move it to /toolkit/locales/en-US/chrome/places/places.properties, if that's okay with you.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> Metro Firefox doesn't include code from /browser/components or
> /browser/locales (it's a separate app, like Fennec), so we can't have a
> shared string there.
> 
> If we want to share the string, we could move it to
> /toolkit/locales/en-US/chrome/places/places.properties, if that's okay with
> you.

I see, I'd be fine to take it in toolkit, it's not a big deal.

Comment 12

[Marina Samuel [:emtwo]]

Attachment: v3: Only create Windows 8 Touch smart bookmark once a user has intialized their Metro bookmark list.

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8384817 [details] [diff] [review]
v3: Only create Windows 8 Touch smart bookmark once a user has intialized their Metro bookmark list.

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

r=mbrubeck for the /browser/metro part.

Comment 14

[Marina Samuel [:emtwo]]

Hey Marco. Was wondering if you'd be able to get to this today since it's our last chance to uplift to beta? Would be nice to have this bug fixed before going into release. Thanks!

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Yes, we are going to build with our final beta today - let's get a nomination here for uplift with a breakdown of risk/reward.  Is this a metro-only fix?  That will make it more likely to be ok for uplift.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

We're going to have to leave this out of FF28 based on timing, feel free to nominate for Aurora uplift in the coming week if low risk so this gets into FF29 beta cycle for wider user reach.

Comment 17

[Marco Mucci [:MarcoM]]

Removing release blocking tags and tagging for a FF29 release.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8384817 [details] [diff] [review]
v3: Only create Windows 8 Touch smart bookmark once a user has intialized their Metro bookmark list.

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

Sorry for late, this week has been a little bit surprising...

r=me with the following fixed

::: browser/components/nsBrowserGlue.js
@@ +1729,5 @@
>            if (queryId in smartBookmarks) {
>              let smartBookmark = smartBookmarks[queryId];
> +            if (!smartBookmark.uri) {
> +              PlacesUtils.bookmarks.removeItem(itemId);
> +              delete smartBookmarks.queryId;

this delete should not be needed since later you continue in the for loop, so you should either do this delete, or continue later.

@@ +1756,5 @@
>                !smartBookmark.itemId)
>              continue;
>  
> +          if (!smartBookmark.uri) {
> +            menuIndex--; // Update menuIndex for deciding where to place a separator later.

ugh, this is indeed a little bit bogus, since the smartBookmark with invalid uri may be on a toolbar...

what about changing all position properties to be getters like get position() menuIndex++ (or toolbarIndex++)

So it will be increased only on use.

::: browser/components/places/tests/unit/head_bookmarks.js
@@ +67,2 @@
>  const SMART_BOOKMARKS_ON_TOOLBAR = 1;
> +const SMART_BOOKMARKS_ON_MENU =  3; // Takes in count the additional separator.

should be "takes into account" (my past fault likely)

Comment 19

[Marina Samuel [:emtwo]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb89890bd6c7

Comment 21

[Dominic]

Has this resolved the bug whereby if you sync your bookmarks in Firefox Touch-based version (in the non-Metro interface, I don't even know if you can sync from the Touch interface), and you are syncing to a non-touch version of Firefox running on another computer, you don't see any of the sync'd bookmarks in the Touch/Metro interface (though they appear fine in the windowed interface)?

Practically this makes Firefox Touch useless as an upgrade, for me anyway :-(