diff --git a/browser/components/readinglist/ReadingList.jsm b/browser/components/readinglist/ReadingList.jsm index 66d4147e0ba1..781c0045c38c 100644 --- a/browser/components/readinglist/ReadingList.jsm +++ b/browser/components/readinglist/ReadingList.jsm @@ -233,15 +233,17 @@ ReadingListImpl.prototype = { * an Error on error. */ forEachItem: Task.async(function* (callback, ...optsList) { - yield this._forEachItem(callback, optsList, STORE_OPTIONS_IGNORE_DELETED); + let thisCallback = record => callback(this._itemFromRecord(record)); + yield this._forEachRecord(thisCallback, optsList, STORE_OPTIONS_IGNORE_DELETED); }), /** - * Like forEachItem, but enumerates only previously synced items that are - * marked as being locally deleted. + * Enumerates the GUIDs for previously synced items that are marked as being + * locally deleted. */ - forEachSyncedDeletedItem: Task.async(function* (callback, ...optsList) { - yield this._forEachItem(callback, optsList, { + forEachSyncedDeletedGUID: Task.async(function* (callback, ...optsList) { + let thisCallback = record => callback(record.guid); + yield this._forEachRecord(thisCallback, optsList, { syncStatus: SYNC_STATUS_DELETED, }); }), @@ -252,12 +254,12 @@ ReadingListImpl.prototype = { * @param storeOptions An options object passed to the store as the "control" * options. */ - _forEachItem: Task.async(function* (callback, optsList, storeOptions) { + _forEachRecord: Task.async(function* (callback, optsList, storeOptions) { let promiseChain = Promise.resolve(); yield this._store.forEachItem(record => { promiseChain = promiseChain.then(() => { return new Promise((resolve, reject) => { - let promise = callback(this._itemFromRecord(record)); + let promise = callback(record); if (promise instanceof Promise) { return promise.then(resolve, reject); } @@ -317,7 +319,9 @@ ReadingListImpl.prototype = { record.syncStatus = SYNC_STATUS_NEW; } + log.debug("addingItem with guid: ${guid}, url: ${url}", record); yield this._store.addItem(record); + log.trace("added item with guid: ${guid}, url: ${url}", record); this._invalidateIterators(); let item = this._itemFromRecord(record); this._callListeners("onItemAdded", item); @@ -345,7 +349,9 @@ ReadingListImpl.prototype = { throw new Error("The item must have a url"); } this._ensureItemBelongsToList(item); + log.debug("updatingItem with guid: ${guid}, url: ${url}", item._record); yield this._store.updateItem(item._record); + log.trace("finished update of item guid: ${guid}, url: ${url}", item._record); this._invalidateIterators(); this._callListeners("onItemUpdated", item); }), @@ -367,6 +373,7 @@ ReadingListImpl.prototype = { // the store. Otherwise mark it as deleted but don't actually delete it so // that its status can be synced. if (item._record.syncStatus == SYNC_STATUS_NEW) { + log.debug("deleteItem guid: ${guid}, url: ${url} - item is local so really deleting it", item._record); yield this._store.deleteItemByURL(item.url); } else { @@ -378,12 +385,16 @@ ReadingListImpl.prototype = { } newRecord.guid = item._record.guid; newRecord.syncStatus = SYNC_STATUS_DELETED; - item._record = newRecord; - yield this._store.updateItemByGUID(item._record); + log.debug("deleteItem guid: ${guid}, url: ${url} - item has been synced so updating to deleted state", item._record); + yield this._store.updateItemByGUID(newRecord); } + log.trace("finished db operation deleting item with guid: ${guid}, url: ${url}", item._record); item.list = null; - this._itemsByNormalizedURL.delete(item.url); + // failing to remove the item from the map points at something bad! + if (!this._itemsByNormalizedURL.delete(item.url)) { + log.error("Failed to remove item from the map", item); + } this._invalidateIterators(); let mm = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager); mm.broadcastAsyncMessage("Reader:Removed", item); @@ -514,6 +525,9 @@ ReadingListImpl.prototype = { * @return The ReadingListItem. */ _itemFromRecord(record) { + if (!record.url) { + throw new Error("record must have a URL"); + } let itemWeakRef = this._itemsByNormalizedURL.get(record.url); let item = itemWeakRef ? itemWeakRef.get() : null; if (item) { diff --git a/browser/components/readinglist/Sync.jsm b/browser/components/readinglist/Sync.jsm index 54c1cfdec6f7..65bb7509a1ef 100644 --- a/browser/components/readinglist/Sync.jsm +++ b/browser/components/readinglist/Sync.jsm @@ -296,9 +296,9 @@ SyncImpl.prototype = { // Get deleted synced local items. let requests = []; - yield this.list.forEachSyncedDeletedItem(localItem => { + yield this.list.forEachSyncedDeletedGUID(guid => { requests.push({ - path: "/articles/" + localItem.guid, + path: "/articles/" + guid, }); }); if (!requests.length) { diff --git a/browser/components/readinglist/test/xpcshell/test_ReadingList.js b/browser/components/readinglist/test/xpcshell/test_ReadingList.js index 760e29a9ed37..b42f1e8a7040 100644 --- a/browser/components/readinglist/test/xpcshell/test_ReadingList.js +++ b/browser/components/readinglist/test/xpcshell/test_ReadingList.js @@ -9,6 +9,10 @@ Cu.import("resource:///modules/readinglist/ReadingList.jsm"); Cu.import("resource:///modules/readinglist/SQLiteStore.jsm"); Cu.import("resource://gre/modules/Sqlite.jsm"); Cu.import("resource://gre/modules/Timer.jsm"); +Cu.import("resource://gre/modules/Log.jsm"); + +Log.repository.getLogger("readinglist.api").level = Log.Level.All; +Log.repository.getLogger("readinglist.api").addAppender(new Log.DumpAppender()); var gList; var gItems; @@ -294,10 +298,10 @@ add_task(function* forEachSyncedDeletedItem() { }); deletedItem._record.syncStatus = gList.SyncStatus.SYNCED; yield gList.deleteItem(deletedItem); - let items = []; - yield gList.forEachSyncedDeletedItem(item => items.push(item)); - Assert.equal(items.length, 1); - Assert.equal(items[0].guid, deletedItem.guid); + let guids = []; + yield gList.forEachSyncedDeletedGUID(guid => guids.push(guid)); + Assert.equal(guids.length, 1); + Assert.equal(guids[0], deletedItem.guid); }); add_task(function* forEachItem_promises() { @@ -654,7 +658,7 @@ add_task(function* listeners() { Assert.equal((yield gList.count()), gItems.length); }); -// This test deletes items so it should probably run last. +// This test deletes items so it should probably run last of the 'gItems' tests... add_task(function* deleteItem() { // delete first item with item.delete() let iter = gList.iterator({ @@ -692,6 +696,28 @@ add_task(function* deleteItem() { checkItems(items, gItems.slice(3)); }); +// Check that when we delete an item with a GUID it's no longer available as +// an item +add_task(function* deletedItemRemovedFromMap() { + yield gList.forEachItem(item => item.delete()); + Assert.equal((yield gList.count()), 0); + let map = gList._itemsByNormalizedURL; + Assert.equal(gList._itemsByNormalizedURL.size, 0, [for (i of map.keys()) i]); + let record = { + guid: "test-item", + url: "http://localhost", + syncStatus: gList.SyncStatus.SYNCED, + } + let item = yield gList.addItem(record); + Assert.equal(map.size, 1); + yield item.delete(); + Assert.equal(gList._itemsByNormalizedURL.size, 0, [for (i of map.keys()) i]); + + // Now enumerate deleted items - should not come back. + yield gList.forEachSyncedDeletedGUID(() => {}); + Assert.equal(gList._itemsByNormalizedURL.size, 0, [for (i of map.keys()) i]); +}); + function checkItems(actualItems, expectedItems) { Assert.equal(actualItems.length, expectedItems.length); for (let i = 0; i < expectedItems.length; i++) {