--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -847,19 +847,19 @@ function updateBookmark(info, item, newP
if (info.hasOwnProperty("url")) {
// Ensure a page exists in moz_places for this URL.
yield maybeInsertPlace(db, info.url);
// Update tuples for the update query.
tuples.set("url", { value: info.url.href
, fragment: "fk = (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url)" });
}
+ let newIndex = info.hasOwnProperty("index") ? info.index : item.index;
if (newParent) {
// For simplicity, update the index regardless.
- let newIndex = info.hasOwnProperty("index") ? info.index : item.index;
tuples.set("position", { value: newIndex });
if (newParent.guid == item.parentGuid) {
// Moving inside the original container.
// When moving "up", add 1 to each index in the interval.
// Otherwise when moving down, we subtract 1.
// Only the parent needs a sync change, which is handled in
// `setAncestorsLastModified`.
@@ -931,16 +931,26 @@ function updateBookmark(info, item, newP
yield db.executeCached(
`UPDATE moz_bookmarks
SET ${Array.from(tuples.keys()).map(v => tuples.get(v).fragment || `${v} = :${v}`).join(", ")}
WHERE guid = :guid
`, Object.assign({ guid: info.guid },
[...tuples.entries()].reduce((p, c) => { p[c[0]] = c[1].value; return p; }, {})));
if (newParent) {
+ if (newParent.guid == item.parentGuid) {
+ // Mark all affected separators as changed
+ // Also bumps the change counter if the item itself is a separator
+ const startIndex = Math.min(newIndex, item.index);
+ yield adjustSeparatorsSyncCounter(db, newParent._id, startIndex, syncChangeDelta);
+ } else {
+ // Mark all affected separators as changed
+ yield adjustSeparatorsSyncCounter(db, item._parentId, item.index, syncChangeDelta);
+ yield adjustSeparatorsSyncCounter(db, newParent._id, newIndex, syncChangeDelta);
+ }
// Remove the Sync orphan annotation from reparented items. We don't
// notify annotation observers about this because this is a temporary,
// internal anno that's only used by Sync.
yield db.executeCached(
`DELETE FROM moz_items_annos
WHERE anno_attribute_id = (SELECT id FROM moz_anno_attributes
WHERE name = :orphanAnno) AND
item_id = :id`,
@@ -1010,16 +1020,19 @@ function insertBookmark(item, parent) {
:index, :title, :date_added, :last_modified, :guid,
:syncChangeCounter, :syncStatus)
`, { url: item.hasOwnProperty("url") ? item.url.href : "nonexistent",
type: item.type, parent: parent._id, index: item.index,
title: item.title, date_added: PlacesUtils.toPRTime(item.dateAdded),
last_modified: PlacesUtils.toPRTime(item.lastModified), guid: item.guid,
syncChangeCounter: syncChangeDelta, syncStatus });
+ // Mark all affected separators as changed
+ yield adjustSeparatorsSyncCounter(db, parent._id, item.index + 1, syncChangeDelta);
+
if (hasExistingGuid) {
// Remove stale tombstones if we're reinserting an item.
yield db.executeCached(
`DELETE FROM moz_bookmarks_deleted WHERE guid = :guid`,
{ guid: item.guid });
}
if (isTagging) {
@@ -1242,16 +1255,19 @@ function removeBookmark(item, options) {
yield db.executeCached(
`UPDATE moz_bookmarks SET position = position - 1 WHERE
parent = :parentId AND position > :index
`, { parentId: item._parentId, index: item.index });
let syncChangeDelta =
PlacesSyncUtils.bookmarks.determineSyncChangeDelta(options.source);
+ // Mark all affected separators as changed
+ yield adjustSeparatorsSyncCounter(db, item._parentId, item.index, syncChangeDelta);
+
if (isUntagging) {
// If we're removing a tag entry, increment the change counter for all
// bookmarks with the tagged URL.
yield PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
db, item.url, syncChangeDelta);
}
// Write a tombstone for the removed item.
@@ -1762,8 +1778,27 @@ function addSyncChangesForBookmarksInFol
return db.execute(`
UPDATE moz_bookmarks SET
syncChangeCounter = syncChangeCounter + :syncChangeDelta
WHERE type = :type AND
fk = (SELECT fk FROM moz_bookmarks WHERE parent = :parent)
`,
{ syncChangeDelta, type: Bookmarks.TYPE_BOOKMARK, parent: folder._id });
}
+
+function adjustSeparatorsSyncCounter(db, parentId, startIndex, syncChangeDelta) {
+ if (!syncChangeDelta) {
+ return Promise.resolve();
+ }
+
+ return db.executeCached(`
+ UPDATE moz_bookmarks
+ SET syncChangeCounter = syncChangeCounter + :delta
+ WHERE parent = :parent AND position >= :start_index
+ AND type = :item_type
+ `,
+ {
+ delta: syncChangeDelta,
+ parent: parentId,
+ start_index: startIndex,
+ item_type: Bookmarks.TYPE_SEPARATOR
+ });
+}
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -334,16 +334,50 @@ nsNavBookmarks::AdjustIndices(int64_t aF
rv = stmt->Execute();
NS_ENSURE_SUCCESS(rv, rv);
return NS_OK;
}
+nsresult
+nsNavBookmarks::AdjustSeparatorsSyncCounter(int64_t aFolderId,
+ int32_t aStartIndex,
+ int64_t aSyncChangeDelta)
+{
+ MOZ_ASSERT(aStartIndex >= 0, "Bad start position");
+ if (!aSyncChangeDelta) {
+ return NS_OK;
+ }
+
+ nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
+ "UPDATE moz_bookmarks SET syncChangeCounter = syncChangeCounter + :delta "
+ "WHERE parent = :parent AND position >= :start_index "
+ "AND type = :item_type "
+ );
+ NS_ENSURE_STATE(stmt);
+ mozStorageStatementScoper scoper(stmt);
+
+ nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("delta"), aSyncChangeDelta);
+ NS_ENSURE_SUCCESS(rv, rv);
+ rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("parent"), aFolderId);
+ NS_ENSURE_SUCCESS(rv, rv);
+ rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("start_index"), aStartIndex);
+ NS_ENSURE_SUCCESS(rv, rv);
+ rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("item_type"), TYPE_SEPARATOR);
+ NS_ENSURE_SUCCESS(rv, rv);
+
+ rv = stmt->Execute();
+ NS_ENSURE_SUCCESS(rv, rv);
+
+ return NS_OK;
+}
+
+
NS_IMETHODIMP
nsNavBookmarks::GetPlacesRoot(int64_t* aRoot)
{
*aRoot = mRoot;
return NS_OK;
}
@@ -509,16 +543,20 @@ nsNavBookmarks::InsertBookmarkInDB(int64
bool isTagging = aGrandParentId == mTagsRoot;
if (isTagging) {
// If we're tagging a bookmark, increment the change counter for all
// bookmarks with the URI.
rv = AddSyncChangesForBookmarksWithURI(aURI, syncChangeDelta);
NS_ENSURE_SUCCESS(rv, rv);
}
+ // Mark all affected separators as changed
+ rv = AdjustSeparatorsSyncCounter(aParentId, aIndex + 1, syncChangeDelta);
+ NS_ENSURE_SUCCESS(rv, rv);
+
// Add a cache entry since we know everything about this bookmark.
BookmarkData bookmark;
bookmark.id = *_itemId;
bookmark.guid.Assign(_guid);
if (aTitle.IsVoid()) {
bookmark.title.SetIsVoid(true);
}
else {
@@ -700,16 +738,20 @@ nsNavBookmarks::RemoveItem(int64_t aItem
NS_ENSURE_SUCCESS(rv, rv);
}
bookmark.lastModified = RoundedPRNow();
rv = SetItemDateInternal(LAST_MODIFIED, syncChangeDelta,
bookmark.parentId, bookmark.lastModified);
NS_ENSURE_SUCCESS(rv, rv);
+ // Mark all affected separators as changed
+ rv = AdjustSeparatorsSyncCounter(bookmark.parentId, bookmark.position, syncChangeDelta);
+ NS_ENSURE_SUCCESS(rv, rv);
+
if (isUntagging) {
// If we're removing a tag, increment the change counter for all bookmarks
// with the URI.
rv = AddSyncChangesForBookmarksWithURL(bookmark.url, syncChangeDelta);
NS_ENSURE_SUCCESS(rv, rv);
}
rv = transaction.Commit();
@@ -1409,23 +1451,34 @@ nsNavBookmarks::MoveItem(int64_t aItemId
rv = SetItemDateInternal(LAST_MODIFIED, syncChangeDelta,
bookmark.parentId, now);
NS_ENSURE_SUCCESS(rv, rv);
if (sameParent) {
// If we're moving within the same container, only the parent needs a sync
// change. Update the item's last modified date without bumping its counter.
rv = SetItemDateInternal(LAST_MODIFIED, 0, bookmark.id, now);
NS_ENSURE_SUCCESS(rv, rv);
+
+ // Mark all affected separators as changed
+ int32_t startIndex = std::min(bookmark.position, newIndex);
+ rv = AdjustSeparatorsSyncCounter(bookmark.parentId, startIndex, syncChangeDelta);
+ NS_ENSURE_SUCCESS(rv, rv);
} else {
// Otherwise, if we're moving between containers, both parents and the child
// need sync changes.
rv = SetItemDateInternal(LAST_MODIFIED, syncChangeDelta, aNewParent, now);
NS_ENSURE_SUCCESS(rv, rv);
rv = SetItemDateInternal(LAST_MODIFIED, syncChangeDelta, bookmark.id, now);
NS_ENSURE_SUCCESS(rv, rv);
+
+ // Mark all affected separators as changed
+ rv = AdjustSeparatorsSyncCounter(bookmark.parentId, bookmark.position, syncChangeDelta);
+ NS_ENSURE_SUCCESS(rv, rv);
+ rv = AdjustSeparatorsSyncCounter(aNewParent, newIndex, syncChangeDelta);
+ NS_ENSURE_SUCCESS(rv, rv);
}
bool isChangingTagFolder = bookmark.parentId == mTagsRoot;
if (isChangingTagFolder) {
// Moving a tag folder out of the tags root untags all its bookmarks. This
// is an odd case, but the tagging service adds an observer to handle it,
// so we bump the change counter for each untagged item for consistency.
rv = AddSyncChangesForBookmarksInFolder(bookmark.id, syncChangeDelta);
@@ -2734,16 +2787,19 @@ nsNavBookmarks::SetItemIndex(int64_t aIt
rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("delta"),
syncChangeDelta);
NS_ENSURE_SUCCESS(rv, rv);
rv = stmt->Execute();
NS_ENSURE_SUCCESS(rv, rv);
}
+ rv = AdjustSeparatorsSyncCounter(bookmark.parentId, aNewIndex, syncChangeDelta);
+ NS_ENSURE_SUCCESS(rv, rv);
+
rv = PreventSyncReparenting(bookmark);
NS_ENSURE_SUCCESS(rv, rv);
rv = transaction.Commit();
NS_ENSURE_SUCCESS(rv, rv);
NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
nsINavBookmarkObserver,
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -247,16 +247,20 @@ private:
*/
nsresult ReadRoots();
nsresult AdjustIndices(int64_t aFolder,
int32_t aStartIndex,
int32_t aEndIndex,
int32_t aDelta);
+ nsresult AdjustSeparatorsSyncCounter(int64_t aFolderId,
+ int32_t aStartIndex,
+ int64_t aSyncChangeDelta);
+
/**
* Fetches properties of a folder.
*
* @param aFolderId
* Folder to count children for.
* @param _folderCount
* Number of children in the folder.
* @param _guid
--- a/toolkit/components/places/tests/bookmarks/test_sync_fields.js
+++ b/toolkit/components/places/tests/bookmarks/test_sync_fields.js
@@ -69,16 +69,24 @@ class TestCases {
do_print("Test 2: reparenting");
try {
await this.testReparenting();
} finally {
do_print("Reset sync fields after test 2");
await PlacesTestUtils.markBookmarksAsSynced();
}
+
+ do_print("Test 3: separators");
+ try {
+ await this.testSeparators();
+ } finally {
+ do_print("Reset sync fields after test 3");
+ await PlacesTestUtils.markBookmarksAsSynced();
+ }
}
async testChanges() {
let testUri = NetUtil.newURI("http://test.mozilla.org");
let guid = await this.insertBookmark(PlacesUtils.bookmarks.unfiledGuid,
testUri,
PlacesUtils.bookmarks.DEFAULT_INDEX,
@@ -113,16 +121,38 @@ class TestCases {
do_print(`Set keyword for bookmark ${guid}`);
await checkSyncFields(guid, { syncChangeCounter: 5 });
await this.removeKeyword(guid, "keyword");
do_print(`Removed keyword from bookmark ${guid}`);
await checkSyncFields(guid, { syncChangeCounter: 6 });
}
+ async testSeparators() {
+ let insertSyncedBookmark = async function(uri) {
+ return await this.insertBookmark(PlacesUtils.bookmarks.unfiledGuid,
+ NetUtil.newURI(uri),
+ PlacesUtils.bookmarks.DEFAULT_INDEX,
+ "A bookmark name");
+ }.bind(this);
+
+ await insertSyncedBookmark("http://foo.bar");
+ let secondBmk = await insertSyncedBookmark("http://bar.foo");
+ let sepGuid = await this.insertSeparator(PlacesUtils.bookmarks.unfiledGuid, PlacesUtils.bookmarks.DEFAULT_INDEX);
+ await insertSyncedBookmark("http://barbar.foo");
+
+ do_print("Move a bookmark around the separator");
+ await this.moveItem(secondBmk, PlacesUtils.bookmarks.unfiledGuid, 4);
+ await checkSyncFields(sepGuid, { syncChangeCounter: 2 });
+
+ do_print("Move a separator around directly");
+ await this.moveItem(sepGuid, PlacesUtils.bookmarks.unfiledGuid, 0);
+ await checkSyncFields(sepGuid, { syncChangeCounter: 3 });
+ }
+
async testReparenting() {
let counterTracker = new CounterTracker();
let folder1 = await this.createFolder(PlacesUtils.bookmarks.unfiledGuid,
"folder1",
PlacesUtils.bookmarks.DEFAULT_INDEX);
do_print(`Created the first folder, guid is ${folder1}`);
@@ -227,16 +257,22 @@ class SyncTestCases extends TestCases {
}
async insertBookmark(parentGuid, uri, index, title) {
let parentId = await PlacesUtils.promiseItemId(parentGuid);
let id = PlacesUtils.bookmarks.insertBookmark(parentId, uri, index, title);
return await PlacesUtils.promiseItemGuid(id);
}
+ async insertSeparator(parentGuid, index) {
+ let parentId = await PlacesUtils.promiseItemId(parentGuid);
+ let id = PlacesUtils.bookmarks.insertSeparator(parentId, index);
+ return await PlacesUtils.promiseItemGuid(id);
+ }
+
async moveItem(guid, newParentGuid, index) {
let id = await PlacesUtils.promiseItemId(guid);
let newParentId = await PlacesUtils.promiseItemId(newParentGuid);
PlacesUtils.bookmarks.moveItem(id, newParentId, index);
}
async removeItem(guid) {
let id = await PlacesUtils.promiseItemId(guid);
@@ -304,16 +340,25 @@ class AsyncTestCases extends TestCases {
parentGuid,
url: uri,
index,
title,
});
return item.guid;
}
+ async insertSeparator(parentGuid, index) {
+ let item = await PlacesUtils.bookmarks.insert({
+ type: PlacesUtils.bookmarks.TYPE_SEPARATOR,
+ parentGuid,
+ index
+ });
+ return item.guid;
+ }
+
async moveItem(guid, newParentGuid, index) {
await PlacesUtils.bookmarks.update({
guid,
parentGuid: newParentGuid,
index,
});
}
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -2130,16 +2130,74 @@ add_task(function* test_touch() {
} finally {
yield stopServer();
}
yield PlacesUtils.bookmarks.eraseEverything();
yield PlacesSyncUtils.bookmarks.reset();
});
+add_task(function* test_separator() {
+ yield ignoreChangedRoots();
+
+ yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: "menu",
+ syncId: makeGuid(),
+ url: "https://example.com",
+ });
+ let childBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: "menu",
+ syncId: makeGuid(),
+ url: "https://foo.bar",
+ });
+ let separatorSyncId = makeGuid();
+ let separator = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "separator",
+ parentSyncId: "menu",
+ syncId: separatorSyncId
+ });
+ yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: "menu",
+ syncId: makeGuid(),
+ url: "https://bar.foo",
+ });
+ let child2Id = yield syncIdToId(childBmk.syncId);
+ let parentId = yield syncIdToId("menu");
+ let separatorId = yield syncIdToId(separator.syncId);
+ let separatorGuid = PlacesSyncUtils.bookmarks.syncIdToGuid(separatorSyncId);
+
+ do_print("Move a bookmark around the separator");
+ PlacesUtils.bookmarks.moveItem(child2Id, parentId, separator + 1);
+ let changes = yield PlacesSyncUtils.bookmarks.pullChanges();
+ deepEqual(Object.keys(changes).sort(),
+ [separator.syncId, "menu"].sort());
+
+ yield setChangesSynced(changes);
+
+ do_print("Move a separator around directly");
+ PlacesUtils.bookmarks.moveItem(separatorId, parentId, 0);
+ changes = yield PlacesSyncUtils.bookmarks.pullChanges();
+ deepEqual(Object.keys(changes).sort(),
+ [separator.syncId, "menu"].sort());
+
+ yield setChangesSynced(changes);
+
+ do_print("Move a separator around directly using update");
+ yield PlacesUtils.bookmarks.update({ guid: separatorGuid, index: 2 });
+ changes = yield PlacesSyncUtils.bookmarks.pullChanges();
+ deepEqual(Object.keys(changes).sort(),
+ [separator.syncId, "menu"].sort());
+
+ yield PlacesUtils.bookmarks.eraseEverything();
+ yield PlacesSyncUtils.bookmarks.reset();
+});
+
add_task(function* test_remove() {
yield ignoreChangedRoots();
do_print("Insert subtree for removal");
let parentFolder = yield PlacesSyncUtils.bookmarks.insert({
kind: "folder",
parentSyncId: "menu",
syncId: makeGuid(),