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
--- 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");
-});