Bug 1312494 - Fix up queries that point to the wrong folder in `_ensureMobileQuery`. r=tcsc

This patch also reports validation errors for left pane queries on the
server, and removes incorrect mobile query migration code.

MozReview-Commit-ID: FT7kAZJapqE

--HG--
extra : rebase_source : 1aa2a14a245bc0bb2a044d951a4c5d274b9d9f13

services/sync/modules/bookmark_validator.js

@@ -12,6 +12,15 @@ Cu.import("resource://gre/modules/Task.jsm");
 
 this.EXPORTED_SYMBOLS = ["BookmarkValidator", "BookmarkProblemData"];
 
+const LEFT_PANE_ROOT_ANNO = "PlacesOrganizer/OrganizerFolder";
+const LEFT_PANE_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
+
+// Indicates if a local bookmark tree node should be excluded from syncing.
+function isNodeIgnored(treeNode) {
+  return treeNode.annos && treeNode.annos.some(anno => anno.name == LEFT_PANE_ROOT_ANNO ||
+                                                       anno.name == LEFT_PANE_QUERY_ANNO);
+}
+
 /**
  * Result of bookmark validation. Contains the following fields which describe
  * server-side problems unless otherwise specified.
@@ -202,7 +211,7 @@ class BookmarkValidator {
     function traverse(treeNode) {
       let guid = PlacesSyncUtils.bookmarks.guidToSyncId(treeNode.guid);
       let itemType = 'item';
-      treeNode.ignored = PlacesUtils.annotations.itemHasAnnotation(treeNode.id, PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);
+      treeNode.ignored = isNodeIgnored(treeNode);
       treeNode.id = guid;
       switch (treeNode.type) {
         case PlacesUtils.TYPE_X_MOZ_PLACE:

services/sync/modules/engines/bookmarks.js

@@ -1278,8 +1278,12 @@ BookmarksTracker.prototype = {
       PlacesUtils.annotations.setItemAnnotation(query, PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO, 1, 0,
                                   PlacesUtils.annotations.EXPIRE_NEVER, SOURCE_SYNC);
     }
-    // Make sure the existing title is correct
+    // Make sure the existing query URL and title are correct
     else {
+      if (!PlacesUtils.bookmarks.getBookmarkURI(mobile[0]).equals(queryURI)) {
+        PlacesUtils.bookmarks.changeBookmarkURI(mobile[0], queryURI,
+                                                SOURCE_SYNC);
+      }
       let queryTitle = PlacesUtils.bookmarks.getItemTitle(mobile[0]);
       if (queryTitle != title) {
         PlacesUtils.bookmarks.setItemTitle(mobile[0], title, SOURCE_SYNC);

services/sync/tests/unit/test_bookmark_tracker.js

@@ -1517,6 +1517,16 @@ add_task(function* test_mobile_query() {
     queryInfo = yield PlacesUtils.bookmarks.fetch(queryGuid);
     equal(queryInfo.title, "Mobile Bookmarks", "Should fix query title");
 
+    _("Point query to different folder");
+    yield PlacesUtils.bookmarks.update({
+      guid: queryGuid,
+      url: "place:folder=BOOKMARKS_MENU",
+    });
+    tracker._ensureMobileQuery();
+    queryInfo = yield PlacesUtils.bookmarks.fetch(queryGuid);
+    equal(queryInfo.url.href, `place:folder=${PlacesUtils.mobileFolderId}`,
+      "Should fix query URL to point to mobile root");
+
     _("We shouldn't track the query or the left pane root");
     yield verifyTrackedCount(0);
     do_check_eq(tracker.score, 0);

services/sync/tests/unit/test_bookmark_validator.js

@@ -239,6 +239,66 @@ add_test(function test_cswc_differences() {
   run_next_test();
 });
 
+add_test(function test_cswc_serverUnexpected() {
+  let {server, client} = getDummyServerAndClient();
+  client.children.push({
+    "guid": "dddddddddddd",
+    "title": "",
+    "id": 2000,
+    "annos": [{
+      "name": "places/excludeFromBackup",
+      "flags": 0,
+      "expires": 4,
+      "value": 1
+    }, {
+      "name": "PlacesOrganizer/OrganizerFolder",
+      "flags": 0,
+      "expires": 4,
+      "value": 7
+    }],
+    "type": "text/x-moz-place-container",
+    "children": [{
+      "guid": "eeeeeeeeeeee",
+      "title": "History",
+      "annos": [{
+        "name": "places/excludeFromBackup",
+        "flags": 0,
+        "expires": 4,
+        "value": 1
+      }, {
+        "name": "PlacesOrganizer/OrganizerQuery",
+        "flags": 0,
+        "expires": 4,
+        "value": "History"
+      }],
+      "type": "text/x-moz-place",
+      "uri": "place:type=3&sort=4"
+    }]
+  });
+  server.push({
+    id: 'dddddddddddd',
+    parentid: 'places',
+    parentName: '',
+    title: '',
+    type: 'folder',
+    children: ['eeeeeeeeeeee']
+  }, {
+    id: 'eeeeeeeeeeee',
+    parentid: 'dddddddddddd',
+    parentName: '',
+    title: 'History',
+    type: 'query',
+    bmkUri: 'place:type=3&sort=4'
+  });
+
+  let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
+  equal(c.clientMissing.length, 0);
+  equal(c.serverMissing.length, 0);
+  equal(c.serverUnexpected.length, 2);
+  deepEqual(c.serverUnexpected, ["dddddddddddd", "eeeeeeeeeeee"]);
+  run_next_test();
+});
+
 function validationPing(server, client, duration) {
   return wait_for_ping(function() {
     // fake this entirely

toolkit/components/places/Database.cpp

@@ -92,7 +92,6 @@
 
 #define MOBILE_ROOT_GUID "mobile______"
 #define MOBILE_ROOT_ANNO "mobile/bookmarksRoot"
-#define MOBILE_QUERY_ANNO "MobileBookmarks"
 
 // We use a fixed title for the mobile root to avoid marking the database as
 // corrupt if we can't look up the localized title in the string bundle. Sync
@@ -1909,22 +1908,6 @@ Database::MigrateV35Up() {
     if (NS_FAILED(rv)) return rv;
   }
 
-  // Delete any left pane queries that point to the old folder. We'll
-  // automatically create one for the new mobile root during the next sync.
-  // Other queries like `place:folder={folderId}` might become invalid; we
-  // ignore them because they're unlikely to exist, and rewriting query URLs
-  // is tedious.
-  nsTArray<int64_t> queryIds;
-  rv = GetItemsWithAnno(NS_LITERAL_CSTRING(MOBILE_QUERY_ANNO),
-                        nsINavBookmarksService::TYPE_BOOKMARK,
-                        queryIds);
-  if (NS_FAILED(rv)) return rv;
-
-  for (uint32_t i = 0; i < queryIds.Length(); ++i) {
-    rv = DeleteBookmarkItem(queryIds[i]);
-    if (NS_FAILED(rv)) return rv;
-  }
-
   return NS_OK;
 }
 

toolkit/components/places/tests/migration/test_current_from_v34.js

@@ -80,20 +80,8 @@ function insertMobileFolder(db) {
   });
 }
 
-function insertMobileQuery(db, parentGuid, folderId) {
-  return db.executeTransaction(function* () {
-    let item = yield* insertItem(db, {
-      type: TYPE_BOOKMARK,
-      parentGuid,
-      url: `place:folder=${folderId}`,
-    });
-    yield* insertAnno(db, item.id, "MobileBookmarks", 0);
-    return item;
-  });
-}
-
-var mobileId, mobileGuid, fxGuid, queryGuid;
-var dupeMobileId, dupeMobileGuid, tbGuid, dupeQueryGuid;
+var mobileId, mobileGuid, fxGuid;
+var dupeMobileId, dupeMobileGuid, tbGuid;
 
 add_task(function* setup() {
   yield setupPlacesDatabase("places_v34.sqlite");
@@ -120,15 +108,6 @@ add_task(function* setup() {
     parentGuid: dupeMobileGuid,
   }));
 
-  // Add queries that point to the old mobile folders. These should be
-  // deleted so that Sync can create a new one.
-  do_print("Insert query for mobile folder");
-  ({ guid: queryGuid} = yield insertMobileQuery(db, "toolbar_____", mobileId));
-
-  do_print("Insert query for second mobile folder");
-  ({ guid: dupeQueryGuid} = yield insertMobileQuery(db, "menu________",
-                                                    dupeMobileId));
-
   yield db.close();
 });
 
@@ -160,18 +139,3 @@ add_task(function* test_mobile_root() {
   deepEqual(annoItemIds, [mobileRootId],
     "Only mobile root should have mobile anno");
 });
-
-add_task(function* test_mobile_queries() {
-  let mobileRootId = PlacesUtils.promiseItemId(
-    PlacesUtils.bookmarks.mobileGuid);
-
-  let query = yield PlacesUtils.bookmarks.fetch(queryGuid);
-  ok(!query, "Query should be removed");
-
-  let dupeQuery = yield PlacesUtils.bookmarks.fetch(dupeQueryGuid);
-  ok(!dupeQuery, "Dupe query should be removed");
-
-  let annoQueryIds = PlacesUtils.annotations.getItemsWithAnnotation(
-    "MobileBookmarks", {});
-  deepEqual(annoQueryIds, [], "All mobile query annos should be removed");
-});