Bug 1312494 - Fix up queries that point to the wrong folder in `_ensureMobileQuery`. r=tcsc draft
authorKit Cambridge <kit@yakshaving.ninja>
Mon, 24 Oct 2016 12:06:00 -0700
changeset 428950 1aa2a14a245bc0bb2a044d951a4c5d274b9d9f13
parent 428736 c845bfd0accb7e0c29b41713255963b08006e701
child 534855 ce760b1a4c730f450de4a954348f2340ee464f40
push id33438
push userkcambridge@mozilla.com
push dateMon, 24 Oct 2016 20:37:23 +0000
reviewerstcsc
bugs1312494
milestone52.0a1
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
services/sync/modules/bookmark_validator.js
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_tracker.js
services/sync/tests/unit/test_bookmark_validator.js
toolkit/components/places/Database.cpp
toolkit/components/places/tests/migration/test_current_from_v34.js
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -7,16 +7,25 @@
 const Cu = Components.utils;
 
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
 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.
  *
  * - missingIDs (number): # of objects with missing ids
  * - duplicates (array of ids): ids seen more than once
  * - parentChildMismatches (array of {parent: parentid, child: childid}):
  *   instances where the child's parentid and the parent's children array
@@ -197,17 +206,17 @@ class BookmarkValidator {
   createClientRecordsFromTree(clientTree) {
     // Iterate over the treeNode, converting it to something more similar to what
     // the server stores.
     let records = [];
     let recordsByGuid = new Map();
     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:
           let query = null;
           if (treeNode.annos && treeNode.uri.startsWith("place:")) {
             query = treeNode.annos.find(({name}) =>
               name === PlacesSyncUtils.bookmarks.SMART_BOOKMARKS_ANNO);
           }
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -1273,18 +1273,22 @@ BookmarksTracker.prototype = {
     // Add the mobile bookmarks query if it doesn't exist
     else if (mobile.length == 0) {
       let query = PlacesUtils.bookmarks.insertBookmark(all[0], queryURI, -1, title, /* guid */ null, SOURCE_SYNC);
       PlacesUtils.annotations.setItemAnnotation(query, ORGANIZERQUERY_ANNO, MOBILE_ANNO, 0,
                                   PlacesUtils.annotations.EXPIRE_NEVER, SOURCE_SYNC);
       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);
       }
       let rootTitle =
         PlacesUtils.bookmarks.getItemTitle(PlacesUtils.mobileFolderId);
       if (rootTitle != title) {
         PlacesUtils.bookmarks.setItemTitle(PlacesUtils.mobileFolderId, title,
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -1512,16 +1512,26 @@ add_task(function* test_mobile_query() {
       title: "renamed query",
     });
     tracker._ensureMobileQuery();
     let rootInfo = yield PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.mobileGuid);
     equal(rootInfo.title, "Mobile Bookmarks", "Should fix root title");
     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);
   } finally {
     _("Clean up.");
     yield cleanup();
   }
 });
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -234,16 +234,76 @@ add_test(function test_cswc_differences(
     let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
     equal(c.clientMissing.length, 0);
     equal(c.serverMissing.length, 0);
     deepEqual(c.differences, [{id: 'cccccccccccc', differences: ['type']}]);
   }
   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
     Svc.Obs.notify("weave:service:sync:start");
     Svc.Obs.notify("weave:engine:sync:start", null, "bookmarks");
     Svc.Obs.notify("weave:engine:sync:finish", null, "bookmarks");
     let data = {
       duration, // We fake this just so that we can verify it's passed through.
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -87,17 +87,16 @@
 #define PLACES_BUNDLE "chrome://places/locale/places.properties"
 
 // Livemarks annotations.
 #define LMANNO_FEEDURI "livemark/feedURI"
 #define LMANNO_SITEURI "livemark/siteURI"
 
 #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
 // sets the title to the localized version when it creates the left pane query.
 #define MOBILE_ROOT_TITLE "mobile"
 
 using namespace mozilla;
 
@@ -1904,32 +1903,16 @@ Database::MigrateV35Up() {
     rv = moveStmt->Execute();
     if (NS_FAILED(rv)) return rv;
 
     // Delete the old folder.
     rv = DeleteBookmarkItem(folderIds[i]);
     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;
 }
 
 nsresult
 Database::GetItemsWithAnno(const nsACString& aAnnoName, int32_t aItemType,
                            nsTArray<int64_t>& aItemIds)
 {
   nsCOMPtr<mozIStorageStatement> stmt;
--- a/toolkit/components/places/tests/migration/test_current_from_v34.js
+++ b/toolkit/components/places/tests/migration/test_current_from_v34.js
@@ -75,30 +75,18 @@ function insertMobileFolder(db) {
       type: TYPE_FOLDER,
       parentGuid: "root________",
     });
     yield* insertAnno(db, item.id, "mobile/bookmarksRoot", 1);
     return item;
   });
 }
 
-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");
   // Setup database contents to be migrated.
   let path = OS.Path.join(OS.Constants.Path.profileDir, DB_FILENAME);
   let db = yield Sqlite.openConnection({ path });
 
   do_print("Create mobile folder with bookmarks");
@@ -115,25 +103,16 @@ add_task(function* setup() {
   do_print("Create second mobile folder with different bookmarks");
   ({ id: dupeMobileId, guid: dupeMobileGuid } = yield insertMobileFolder(db));
   ({ guid: tbGuid } = yield insertBookmark(db, {
     type: TYPE_BOOKMARK,
     url: "http://getthunderbird.com",
     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();
 });
 
 add_task(function* database_is_valid() {
   // Accessing the database for the first time triggers migration.
   Assert.equal(PlacesUtils.history.databaseStatus,
                PlacesUtils.history.DATABASE_STATUS_UPGRADED);
 
@@ -155,23 +134,8 @@ add_task(function* test_mobile_root() {
 
   let mobileRootId = PlacesUtils.promiseItemId(
     PlacesUtils.bookmarks.mobileGuid);
   let annoItemIds = PlacesUtils.annotations.getItemsWithAnnotation(
     PlacesUtils.MOBILE_ROOT_ANNO, {});
   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");
-});