--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -408,17 +408,16 @@ class SyncedBookmarksMirror {
await this.db.execute(`DELETE FROM mergeStates`);
await this.db.execute(`DELETE FROM itemsAdded`);
await this.db.execute(`DELETE FROM guidsChanged`);
await this.db.execute(`DELETE FROM itemsChanged`);
await this.db.execute(`DELETE FROM itemsRemoved`);
await this.db.execute(`DELETE FROM itemsMoved`);
await this.db.execute(`DELETE FROM annosChanged`);
- await this.db.execute(`DELETE FROM keywordsChanged`);
await this.db.execute(`DELETE FROM itemsToUpload`);
return changeRecords;
}, this.db.TRANSACTION_IMMEDIATE);
MirrorLog.debug("Replaying recorded observer notifications");
try {
await observersToNotify.notifyAll();
@@ -1322,39 +1321,22 @@ class SyncedBookmarksMirror {
if (row.getResultByName("wasRemoved")) {
observersToNotify.noteAnnoRemoved(id, name);
} else {
observersToNotify.noteAnnoSet(id, name);
}
}
MirrorLog.debug("Recording notifications for changed keywords");
- // `ORDER BY k.ROWID` replays additions and deletions for the same keyword
- // or URL in order.
- let changedKeywordRows = await this.db.execute(`
- SELECT b.id, IFNULL(k.keyword, "") AS keyword, b.lastModified, b.type,
- p.id AS parentId, b.guid, p.guid AS parentGuid, h.url
- FROM keywordsChanged k
- JOIN moz_bookmarks b ON b.id = k.itemId
- JOIN moz_bookmarks p ON p.id = b.parent
- JOIN moz_places h ON h.id = k.placeId
- ORDER BY k.ROWID`);
- for (let row of changedKeywordRows) {
- let info = {
- id: row.getResultByName("id"),
- keyword: row.getResultByName("keyword"),
- lastModified: row.getResultByName("lastModified"),
- type: row.getResultByName("type"),
- parentId: row.getResultByName("parentId"),
- guid: row.getResultByName("guid"),
- parentGuid: row.getResultByName("parentGuid"),
- urlHref: row.getResultByName("url"),
- };
- observersToNotify.noteKeywordChanged(info);
- }
+ let keywordsChangedRows = await this.db.execute(`
+ SELECT EXISTS(SELECT 1 FROM itemsAdded WHERE keywordChanged) OR
+ EXISTS(SELECT 1 FROM itemsChanged WHERE keywordChanged)
+ AS keywordsChanged`);
+ observersToNotify.shouldInvalidateKeywords =
+ !!keywordsChangedRows[0].getResultByName("keywordsChanged");
}
/**
* Stores a snapshot of all locally changed items in a temporary table for
* upload. This is called from within the merge transaction, to ensure that
* structure changes made during the sync don't cause us to upload an
* inconsistent tree.
*
@@ -1969,16 +1951,23 @@ async function initializeTempMirrorEntit
guid = OLD.mergedGuid;
END`);
// Inserts items from the mirror that don't exist locally.
await db.execute(`
CREATE TEMP TRIGGER insertNewLocalItems
INSTEAD OF DELETE ON newRemoteItems WHEN OLD.localId IS NULL
BEGIN
+ /* Record an item added notification for the new item. */
+ INSERT INTO itemsAdded(guid, keywordChanged)
+ VALUES(OLD.mergedGuid, OLD.newKeyword NOT NULL OR
+ EXISTS(SELECT 1 FROM moz_keywords
+ WHERE place_id = OLD.newPlaceId OR
+ keyword = OLD.newKeyword));
+
/* Sync associates keywords with bookmarks, and doesn't sync POST data;
Places associates keywords with (URL, POST data) pairs, and multiple
bookmarks may have the same URL. For simplicity, we bump the change
counter for all local bookmarks with the remote URL (bug 1328737),
then remove all local keywords from remote URLs, and the remote keyword
from local URLs. */
UPDATE moz_bookmarks SET
syncChangeCounter = syncChangeCounter + 1
@@ -1986,24 +1975,16 @@ async function initializeTempMirrorEntit
/* We intentionally use "place_id = OLD.newPlaceId" in the subquery,
instead of "fk = OLD.newPlaceId OR fk IN (...)" in the WHERE clause
above, because we only want to bump the counter if the URL has
keywords. */
SELECT place_id FROM moz_keywords
WHERE place_id = OLD.newPlaceId OR
keyword = OLD.newKeyword);
- /* Record item changed notifications for existing items with the new
- keyword and URL. */
- INSERT INTO keywordsChanged(itemId, placeId, keyword)
- SELECT b.id, b.fk, NULL FROM moz_bookmarks b
- JOIN moz_keywords k ON k.place_id = b.fk
- WHERE k.place_id = OLD.newPlaceId OR
- k.keyword = OLD.newKeyword;
-
/* Remove the new keyword from existing items, and all keywords from the
new URL. */
DELETE FROM moz_keywords WHERE place_id = OLD.newPlaceId OR
keyword = OLD.newKeyword;
/* Remove existing tags for the new URL. */
DELETE FROM localTags WHERE placeId = OLD.newPlaceId;
@@ -2013,33 +1994,22 @@ async function initializeTempMirrorEntit
INSERT INTO moz_bookmarks(guid, parent, position, type, fk, title,
dateAdded, lastModified, syncStatus,
syncChangeCounter)
VALUES(OLD.mergedGuid, -1, -1, OLD.type, OLD.newPlaceId, OLD.title,
OLD.dateAdded, STRFTIME('%s', 'now', 'localtime', 'utc') * 1000000,
${PlacesUtils.bookmarks.SYNC_STATUS.NORMAL},
OLD.syncChangeCounter);
- /* Record an item added notification for the new item. */
- INSERT INTO itemsAdded(guid)
- VALUES(OLD.mergedGuid);
-
- /* Insert new keywords after the item, so that "noteKeywordAdded" can find
- the new item by Place ID. */
+ /* Insert a new keyword for the new URL, if one is set. */
INSERT INTO moz_keywords(keyword, place_id)
SELECT OLD.newKeyword, OLD.newPlaceId
WHERE OLD.newKeyword NOT NULL;
- /* Record item changed notifications for the new keyword. */
- INSERT INTO keywordsChanged(itemId, placeId, keyword)
- SELECT b.id, OLD.newPlaceId, OLD.newKeyword FROM moz_bookmarks b
- WHERE b.guid = OLD.mergedGuid AND
- OLD.newKeyword NOT NULL;
-
- /* Insert new tags for the URL. */
+ /* Insert new tags for the new URL. */
INSERT INTO localTags(tag, placeId)
SELECT t.tag, OLD.newPlaceId FROM tags t
WHERE t.itemId = OLD.remoteId;
/* Insert new synced annos. These are almost identical to the statements
for updates, except we need an additional subquery to fetch the new
item's ID. We can also skip removing existing annos. */
INSERT OR IGNORE INTO moz_anno_attributes(name)
@@ -2069,19 +2039,23 @@ async function initializeTempMirrorEntit
END`);
// Updates existing items with new values from the mirror.
await db.execute(`
CREATE TEMP TRIGGER updateExistingLocalItems
INSTEAD OF DELETE ON newRemoteItems WHEN OLD.needsUpdate AND
OLD.localId NOT NULL
BEGIN
- /* Record item changed notifications for the title and URL. */
- INSERT INTO itemsChanged(itemId, oldTitle, oldPlaceId)
- SELECT id, title, OLD.oldPlaceId FROM moz_bookmarks
+ /* Record an item changed notification for the existing item. */
+ INSERT INTO itemsChanged(itemId, oldTitle, oldPlaceId, keywordChanged)
+ SELECT id, title, OLD.oldPlaceId, OLD.newKeyword NOT NULL OR
+ EXISTS(SELECT 1 FROM moz_keywords
+ WHERE place_id IN (OLD.oldPlaceId, OLD.newPlaceId) OR
+ keyword = OLD.newKeyword)
+ FROM moz_bookmarks
WHERE id = OLD.localId;
UPDATE moz_bookmarks SET
title = OLD.title,
dateAdded = OLD.dateAdded,
lastModified = STRFTIME('%s', 'now', 'localtime', 'utc') * 1000000,
syncStatus = ${PlacesUtils.bookmarks.SYNC_STATUS.NORMAL},
syncChangeCounter = OLD.syncChangeCounter
@@ -2090,25 +2064,16 @@ async function initializeTempMirrorEntit
/* Bump the change counter for items with the old URL, new URL, and new
keyword. */
UPDATE moz_bookmarks SET
syncChangeCounter = syncChangeCounter + 1
WHERE fk IN (SELECT place_id FROM moz_keywords
WHERE place_id IN (OLD.oldPlaceId, OLD.newPlaceId) OR
keyword = OLD.newKeyword);
- /* Record change observer notifications for items with the old URL, new
- URL, and new keyword. This is identical to the subquery above; we just
- query the item ID instead of updating by the Place ID. */
- INSERT INTO keywordsChanged(itemId, placeId, keyword)
- SELECT b.id, b.fk, NULL FROM moz_bookmarks b
- JOIN moz_keywords k ON k.place_id = b.fk
- WHERE k.place_id IN (OLD.oldPlaceId, OLD.newPlaceId) OR
- k.keyword = OLD.newKeyword;
-
/* Remove the new keyword from existing items, and all keywords from the
old and new URLs. */
DELETE FROM moz_keywords WHERE place_id IN (OLD.oldPlaceId,
OLD.newPlaceId) OR
keyword = OLD.newKeyword;
/* Remove existing tags. */
DELETE FROM localTags WHERE placeId IN (OLD.oldPlaceId, OLD.newPlaceId);
@@ -2126,21 +2091,16 @@ async function initializeTempMirrorEntit
WHERE OLD.oldPlaceId <> OLD.newPlaceId AND
id IN (OLD.oldPlaceId, OLD.newPlaceId);
/* Insert a new keyword for the new URL, if one is set. */
INSERT INTO moz_keywords(keyword, place_id)
SELECT OLD.newKeyword, OLD.newPlaceId
WHERE OLD.newKeyword NOT NULL;
- /* Record an item changed notification for the new keyword. */
- INSERT INTO keywordsChanged(itemId, placeId, keyword)
- SELECT OLD.localId, OLD.newPlaceId, OLD.newKeyword
- WHERE OLD.newKeyword NOT NULL;
-
/* Insert new tags for the new URL. */
INSERT INTO localTags(tag, placeId)
SELECT t.tag, OLD.newPlaceId FROM tags t
WHERE t.itemId = OLD.remoteId;
/* Record anno removed notifications for the synced annos. */
REPLACE INTO annosChanged(itemId, annoName, wasRemoved)
SELECT a.item_id, n.name, 1 FROM moz_items_annos a
@@ -2328,29 +2288,31 @@ async function initializeTempMirrorEntit
p.title = NEW.tag AND
r.guid = '${PlacesUtils.bookmarks.tagsGuid}';
END`);
// Stores properties to pass to `onItem{Added, Changed, Moved, Removed}`
// bookmark observers for new, updated, moved, and deleted items.
await db.execute(`CREATE TEMP TABLE itemsAdded(
guid TEXT PRIMARY KEY,
- isTagging BOOLEAN NOT NULL DEFAULT 0
+ isTagging BOOLEAN NOT NULL DEFAULT 0,
+ keywordChanged BOOLEAN NOT NULL DEFAULT 0
) WITHOUT ROWID`);
await db.execute(`CREATE TEMP TABLE guidsChanged(
itemId INTEGER NOT NULL,
oldGuid TEXT NOT NULL,
PRIMARY KEY(itemId, oldGuid)
) WITHOUT ROWID`);
await db.execute(`CREATE TEMP TABLE itemsChanged(
itemId INTEGER PRIMARY KEY,
oldTitle TEXT,
- oldPlaceId INTEGER
+ oldPlaceId INTEGER,
+ keywordChanged BOOLEAN NOT NULL DEFAULT 0
)`);
await db.execute(`CREATE TEMP TABLE itemsMoved(
itemId INTEGER PRIMARY KEY,
oldParentId INTEGER NOT NULL,
oldParentGuid TEXT NOT NULL,
oldPosition INTEGER NOT NULL
)`);
@@ -2373,26 +2335,16 @@ async function initializeTempMirrorEntit
// observers.
await db.execute(`CREATE TEMP TABLE annosChanged(
itemId INTEGER NOT NULL,
annoName TEXT NOT NULL,
wasRemoved BOOLEAN NOT NULL,
PRIMARY KEY(itemId, annoName, wasRemoved)
) WITHOUT ROWID`);
- // Stores properties to pass to `onItemChanged` observers for new and removed
- // keywords. A NULL keyword means we're removing all keywords from a Place.
- // Note that an item may appear multiple times in this table, so `itemId` is
- // not a primary key.
- await db.execute(`CREATE TEMP TABLE keywordsChanged(
- itemId INTEGER NOT NULL,
- placeId INTEGER NOT NULL,
- keyword TEXT
- )`);
-
// Stores locally changed items staged for upload. See `stageItemsToUpload`
// for an explanation of why these tables exists.
await db.execute(`CREATE TEMP TABLE itemsToUpload(
guid TEXT PRIMARY KEY,
syncChangeCounter INTEGER NOT NULL,
isDeleted BOOLEAN NOT NULL DEFAULT 0,
parentGuid TEXT,
parentTitle TEXT,
@@ -3910,25 +3862,29 @@ function contentsMatch(localNode, localC
* Annotation observers don't require the extra context, so they're cheap to
* record and fire.
*/
class BookmarkObserverRecorder {
constructor(db) {
this.db = db;
this.bookmarkObserverNotifications = [];
this.annoObserverNotifications = [];
+ this.shouldInvalidateKeywords = false;
this.shouldInvalidateLivemarks = false;
}
/**
* Fires all recorded observer notifications, invalidates the livemark cache
* if necessary, and recalculates frecencies for changed URLs. This is called
* outside the merge transaction.
*/
async notifyAll() {
+ if (this.shouldInvalidateKeywords) {
+ await PlacesUtils.keywords.invalidateCachedKeywords();
+ }
this.notifyBookmarkObservers();
this.notifyAnnoObservers();
if (this.shouldInvalidateLivemarks) {
await PlacesUtils.livemarks.invalidateCachedLivemarks();
}
await this.updateFrecencies();
}
@@ -3999,26 +3955,16 @@ class BookmarkObserverRecorder {
this.bookmarkObserverNotifications.push({
name: "onItemRemoved",
isTagging: info.isUntagging,
args: [info.id, info.parentId, info.position, info.type, uri, info.guid,
info.parentGuid, PlacesUtils.bookmarks.SOURCES.SYNC],
});
}
- noteKeywordChanged(info) {
- this.bookmarkObserverNotifications.push({
- name: "onItemChanged",
- isTagging: false,
- args: [info.id, "keyword", /* isAnnotationProperty */ false, info.keyword,
- info.lastModified, info.type, info.parentId, info.guid, info.parentGuid,
- /* oldValue */ info.urlHref, PlacesUtils.bookmarks.SOURCES.SYNC],
- });
- }
-
noteAnnoSet(id, name) {
if (isLivemarkAnno(name)) {
this.shouldInvalidateLivemarks = true;
}
this.annoObserverNotifications.push({
name: "onItemAnnotationSet",
args: [id, name, PlacesUtils.bookmarks.SOURCES.SYNC],
});
--- a/toolkit/components/places/tests/sync/test_bookmark_value_changes.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_value_changes.js
@@ -146,25 +146,16 @@ add_task(async function test_value_combo
}, {
name: "onItemChanged",
params: { itemId: localItemIds.get("mozBmk______"), property: "title",
isAnnoProperty: false, newValue: "Mozilla home page",
type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
parentId: PlacesUtils.bookmarksMenuFolderId, guid: "mozBmk______",
parentGuid: PlacesUtils.bookmarks.menuGuid, oldValue: "Mozilla",
source: PlacesUtils.bookmarks.SOURCES.SYNC },
- }, {
- name: "onItemChanged",
- params: { itemId: localItemIds.get("tbBmk_______"), property: "keyword",
- isAnnoProperty: false, newValue: "tb",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: localItemIds.get("tFolder_____"), guid: "tbBmk_______",
- parentGuid: "tFolder_____",
- oldValue: "http://getthunderbird.com/",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
}]);
let fxBmk = await PlacesUtils.bookmarks.fetch("fxBmk_______");
ok(fxBmk, "New Firefox bookmark should exist");
equal(fxBmk.parentGuid, PlacesUtils.bookmarks.toolbarGuid,
"Should add Firefox bookmark to toolbar");
let fxTags = PlacesUtils.tagging.getTagsForURI(
Services.io.newURI("http://getfirefox.com"));
@@ -523,57 +514,25 @@ add_task(async function test_keywords()
info("Change keywords locally");
await PlacesUtils.keywords.insert({
keyword: "four",
url: "http://example.com/c",
});
await PlacesUtils.keywords.remove("three");
info("Apply remote");
- let observer = expectBookmarkChangeNotifications();
let changesToUpload = await buf.apply();
deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
let idsToUpload = inspectChangeRecords(changesToUpload);
deepEqual(idsToUpload, {
updated: ["bookmarkAAAA", "bookmarkCCCC", "bookmarkDDDD"],
deleted: [],
}, "Should reupload all local records with changed keywords");
- let localItemIds = await PlacesUtils.promiseManyItemIds(["bookmarkAAAA",
- "bookmarkBBBB"]);
- observer.check([{
- name: "onItemChanged",
- params: { itemId: localItemIds.get("bookmarkAAAA"), property: "keyword",
- isAnnoProperty: false, newValue: "",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkAAAA",
- parentGuid: PlacesUtils.bookmarks.menuGuid,
- oldValue: "http://example.com/a",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
- }, {
- name: "onItemChanged",
- params: { itemId: localItemIds.get("bookmarkBBBB"), property: "keyword",
- isAnnoProperty: false, newValue: "",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkBBBB",
- parentGuid: PlacesUtils.bookmarks.menuGuid,
- oldValue: "http://example.com/b",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
- }, {
- name: "onItemChanged",
- params: { itemId: localItemIds.get("bookmarkAAAA"), property: "keyword",
- isAnnoProperty: false, newValue: "two",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkAAAA",
- parentGuid: PlacesUtils.bookmarks.menuGuid,
- oldValue: "http://example.com/a",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
- }]);
-
let entryForOne = await PlacesUtils.keywords.fetch("one");
ok(!entryForOne, "Should remove existing keyword from A");
let entriesForTwo = await fetchAllKeywords("two");
deepEqual(entriesForTwo.map(entry => ({
keyword: entry.keyword,
url: entry.url.href,
})), [{
@@ -794,76 +753,16 @@ add_task(async function test_keywords_co
params: { itemId: localItemIds.get("bookmarkCCCC"), property: "uri",
isAnnoProperty: false, newValue: "http://example.com/c-remote",
type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkCCCC",
parentGuid: PlacesUtils.bookmarks.menuGuid,
oldValue: "http://example.com/c",
source: PlacesUtils.bookmarks.SOURCES.SYNC },
}];
- if (entriesForOne.length) {
- expectedNotifications.push({
- name: "onItemChanged",
- params: { itemId: localItemIds.get("bookmarkAAA1"), property: "keyword",
- isAnnoProperty: false, newValue: "two",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkAAA1",
- parentGuid: PlacesUtils.bookmarks.menuGuid,
- oldValue: "http://example.com/a",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
- }, {
- name: "onItemChanged",
- params: { itemId: localItemIds.get("bookmarkAAA1"), property: "keyword",
- isAnnoProperty: false, newValue: "",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkAAA1",
- parentGuid: PlacesUtils.bookmarks.menuGuid,
- oldValue: "http://example.com/a",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
- }, {
- name: "onItemChanged",
- params: { itemId: localItemIds.get("bookmarkAAAA"), property: "keyword",
- isAnnoProperty: false, newValue: "one",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkAAAA",
- parentGuid: PlacesUtils.bookmarks.menuGuid,
- oldValue: "http://example.com/a",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
- });
- } else {
- // TODO(kitcambridge): This never happens, even in chaos mode.
- }
- expectedNotifications.push({
- name: "onItemChanged",
- params: { itemId: localItemIds.get("bookmarkBBBB"), property: "keyword",
- isAnnoProperty: false, newValue: "",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkBBBB",
- parentGuid: PlacesUtils.bookmarks.menuGuid,
- oldValue: "http://example.com/b",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
- }, {
- name: "onItemChanged",
- params: { itemId: localItemIds.get("bookmarkCCCC"), property: "keyword",
- isAnnoProperty: false, newValue: "",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkCCCC",
- parentGuid: PlacesUtils.bookmarks.menuGuid,
- oldValue: "http://example.com/c",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
- }, {
- name: "onItemChanged",
- params: { itemId: localItemIds.get("bookmarkCCCC"), property: "keyword",
- isAnnoProperty: false, newValue: "six",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- parentId: PlacesUtils.bookmarksMenuFolderId, guid: "bookmarkCCCC",
- parentGuid: PlacesUtils.bookmarks.menuGuid,
- oldValue: "http://example.com/c-remote",
- source: PlacesUtils.bookmarks.SOURCES.SYNC },
- });
observer.check(expectedNotifications);
let entriesForFour = await fetchAllKeywords("four");
ok(!entriesForFour.length, "Should remove all keywords for B");
let entriesForOldC = await fetchAllKeywords({
url: "http://example.com/c",
});