From 05596be753d0536231fce2c9a2dbcc30674a8347 Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Fri, 20 Mar 2015 09:39:25 +0100 Subject: [PATCH] Bug 1125115 - Write a new keywords pseudo-API in PlacesUtils. r=ttaubert --- .../test/general/browser_getshortcutoruri.js | 67 ++- .../general/browser_keywordBookmarklets.js | 50 +- browser/components/nsBrowserGlue.js | 32 +- .../places/tests/unit/head_bookmarks.js | 16 - .../tests/unit/test_browserGlue_corrupt.js | 2 +- .../unit/test_browserGlue_corrupt_nobackup.js | 2 +- ...st_browserGlue_corrupt_nobackup_default.js | 2 +- .../tests/unit/test_browserGlue_migrate.js | 2 +- .../tests/unit/test_browserGlue_prefs.js | 4 +- .../tests/unit/test_browserGlue_restore.js | 2 +- .../unit/test_browserGlue_smartBookmarks.js | 2 - toolkit/components/places/Bookmarks.jsm | 38 +- .../places/PlacesCategoriesStarter.js | 17 +- toolkit/components/places/PlacesUtils.jsm | 379 +++++++++++++- toolkit/components/places/nsNavBookmarks.cpp | 82 +-- .../places/tests/bookmarks/test_bookmarks.js | 42 +- .../places/tests/bookmarks/test_keywords.js | 9 +- .../unifiedcomplete/test_keyword_search.js | 2 +- .../places/tests/unit/test_421180.js | 94 ---- .../places/tests/unit/test_keywords.js | 488 ++++++++++++++++++ .../places/tests/unit/test_placesTxn.js | 4 +- .../components/places/tests/unit/xpcshell.ini | 2 +- 22 files changed, 963 insertions(+), 375 deletions(-) delete mode 100644 toolkit/components/places/tests/unit/test_421180.js create mode 100644 toolkit/components/places/tests/unit/test_keywords.js diff --git a/browser/base/content/test/general/browser_getshortcutoruri.js b/browser/base/content/test/general/browser_getshortcutoruri.js index dd47b84d4a14..83959e815fae 100644 --- a/browser/base/content/test/general/browser_getshortcutoruri.js +++ b/browser/base/content/test/general/browser_getshortcutoruri.js @@ -91,57 +91,54 @@ var testData = [ new keywordResult(null, null, true)] ]; -function test() { - waitForExplicitFinish(); +add_task(function* test_getshortcutoruri() { + yield setupKeywords(); - setupKeywords(); + for (let item of testData) { + let [data, result] = item; - Task.spawn(function() { - for each (var item in testData) { - let [data, result] = item; + let query = data.keyword; + if (data.searchWord) + query += " " + data.searchWord; + let returnedData = yield new Promise( + resolve => getShortcutOrURIAndPostData(query, resolve)); + // null result.url means we should expect the same query we sent in + let expected = result.url || query; + is(returnedData.url, expected, "got correct URL for " + data.keyword); + is(getPostDataString(returnedData.postData), result.postData, "got correct postData for " + data.keyword); + is(returnedData.mayInheritPrincipal, !result.isUnsafe, "got correct mayInheritPrincipal for " + data.keyword); + } - let query = data.keyword; - if (data.searchWord) - query += " " + data.searchWord; - let returnedData = yield new Promise( - resolve => getShortcutOrURIAndPostData(query, resolve)); - // null result.url means we should expect the same query we sent in - let expected = result.url || query; - is(returnedData.url, expected, "got correct URL for " + data.keyword); - is(getPostDataString(returnedData.postData), result.postData, "got correct postData for " + data.keyword); - is(returnedData.mayInheritPrincipal, !result.isUnsafe, "got correct mayInheritPrincipal for " + data.keyword); - } - cleanupKeywords(); - }).then(finish); -} + yield cleanupKeywords(); +}); -var gBMFolder = null; -var gAddedEngines = []; -function setupKeywords() { - gBMFolder = Application.bookmarks.menu.addFolder("keyword-test"); - for each (var item in testData) { - var data = item[0]; +let folder = null; +let gAddedEngines = []; + +function* setupKeywords() { + folder = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, + type: PlacesUtils.bookmarks.TYPE_FOLDER, + title: "keyword-test" }); + for (let item of testData) { + let data = item[0]; if (data instanceof bmKeywordData) { - var bm = gBMFolder.addBookmark(data.keyword, data.uri); - bm.keyword = data.keyword; - if (data.postData) - bm.annotations.set("bookmarkProperties/POSTData", data.postData, Ci.nsIAnnotationService.EXPIRE_SESSION); + yield PlacesUtils.bookmarks.insert({ url: data.uri, parentGuid: folder.guid }); + yield PlacesUtils.keywords.insert({ keyword: data.keyword, url: data.uri.spec, postData: data.postData }); } if (data instanceof searchKeywordData) { Services.search.addEngineWithDetails(data.keyword, "", data.keyword, "", data.method, data.uri.spec); - var addedEngine = Services.search.getEngineByName(data.keyword); + let addedEngine = Services.search.getEngineByName(data.keyword); if (data.postData) { - var [paramName, paramValue] = data.postData.split("="); + let [paramName, paramValue] = data.postData.split("="); addedEngine.addParam(paramName, paramValue, null); } - gAddedEngines.push(addedEngine); } } } -function cleanupKeywords() { - gBMFolder.remove(); +function* cleanupKeywords() { + PlacesUtils.bookmarks.remove(folder); gAddedEngines.map(Services.search.removeEngine); } diff --git a/browser/base/content/test/general/browser_keywordBookmarklets.js b/browser/base/content/test/general/browser_keywordBookmarklets.js index 8b075d74cc2b..434951251bdd 100644 --- a/browser/base/content/test/general/browser_keywordBookmarklets.js +++ b/browser/base/content/test/general/browser_keywordBookmarklets.js @@ -1,38 +1,34 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ +"use strict" -function test() { - waitForExplicitFinish(); - - let bmFolder = Application.bookmarks.menu.addFolder("keyword-test"); +add_task(function* test_keyword_bookmarklet() { + let bm = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, + title: "bookmarklet", + url: "javascript:1;" }); let tab = gBrowser.selectedTab = gBrowser.addTab(); - - registerCleanupFunction (function () { - bmFolder.remove(); + registerCleanupFunction (function* () { gBrowser.removeTab(tab); + yield PlacesUtils.bookmarks.remove(bm); }); + yield promisePageShow(); + let originalPrincipal = gBrowser.contentPrincipal; - let bm = bmFolder.addBookmark("bookmarklet", makeURI("javascript:1;")); - bm.keyword = "bm"; + yield PlacesUtils.keywords.insert({ keyword: "bm", url: "javascript:1;" }) - addPageShowListener(function () { - let originalPrincipal = gBrowser.contentPrincipal; + // Enter bookmarklet keyword in the URL bar + gURLBar.value = "bm"; + gURLBar.focus(); + EventUtils.synthesizeKey("VK_RETURN", {}); - // Enter bookmarklet keyword in the URL bar - gURLBar.value = "bm"; - gURLBar.focus(); - EventUtils.synthesizeKey("VK_RETURN", {}); + yield promisePageShow(); - addPageShowListener(function () { - ok(gBrowser.contentPrincipal.equals(originalPrincipal), "javascript bookmarklet should inherit principal"); - finish(); + ok(gBrowser.contentPrincipal.equals(originalPrincipal), "javascript bookmarklet should inherit principal"); +}); + +function* promisePageShow() { + return new Promise(resolve => { + gBrowser.selectedBrowser.addEventListener("pageshow", function listen() { + gBrowser.selectedBrowser.removeEventListener("pageshow", listen); + resolve(); }); }); } - -function addPageShowListener(func) { - gBrowser.selectedBrowser.addEventListener("pageshow", function loadListener() { - gBrowser.selectedBrowser.removeEventListener("pageshow", loadListener, false); - func(); - }); -} diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js index cb688ab001f1..e1d13296b819 100644 --- a/browser/components/nsBrowserGlue.js +++ b/browser/components/nsBrowserGlue.js @@ -1429,7 +1429,7 @@ BrowserGlue.prototype = { () => BookmarkHTMLUtils.exportToFile(BookmarkHTMLUtils.defaultPath)); } - Task.spawn(function() { + Task.spawn(function* () { // Check if Safe Mode or the user has required to restore bookmarks from // default profile's bookmarks.html let restoreDefaultBookmarks = false; @@ -1505,23 +1505,21 @@ BrowserGlue.prototype = { if (bookmarksUrl) { // Import from bookmarks.html file. try { - BookmarkHTMLUtils.importFromURL(bookmarksUrl, true).then(null, - function onFailure() { - Cu.reportError("Bookmarks.html file could be corrupt."); - } - ).then( - function onComplete() { - // Now apply distribution customized bookmarks. - // This should always run after Places initialization. - this._distributionCustomizer.applyBookmarks(); - // Ensure that smart bookmarks are created once the operation is - // complete. - this.ensurePlacesDefaultQueriesInitialized(); - }.bind(this) - ); - } catch (err) { - Cu.reportError("Bookmarks.html file could be corrupt. " + err); + yield BookmarkHTMLUtils.importFromURL(bookmarksUrl, true); + } catch (e) { + Cu.reportError("Bookmarks.html file could be corrupt. " + e); } + try { + // Now apply distribution customized bookmarks. + // This should always run after Places initialization. + this._distributionCustomizer.applyBookmarks(); + // Ensure that smart bookmarks are created once the operation is + // complete. + this.ensurePlacesDefaultQueriesInitialized(); + } catch (e) { + Cu.reportError(e); + } + } else { Cu.reportError("Unable to find bookmarks.html file."); diff --git a/browser/components/places/tests/unit/head_bookmarks.js b/browser/components/places/tests/unit/head_bookmarks.js index a45f4d1428c1..f277d34d7eba 100644 --- a/browser/components/places/tests/unit/head_bookmarks.js +++ b/browser/components/places/tests/unit/head_bookmarks.js @@ -78,22 +78,6 @@ function checkItemHasAnnotation(guid, name) { }); } -function waitForImportAndSmartBookmarks() { - return Promise.all([ - promiseTopicObserved("bookmarks-restore-success"), - PlacesTestUtils.promiseAsyncUpdates() - ]); -} - -function promiseEndUpdateBatch() { - return new Promise(resolve => { - PlacesUtils.bookmarks.addObserver({ - __proto__: NavBookmarkObserver.prototype, - onEndUpdateBatch: resolve - }, false); - }); -} - let createCorruptDB = Task.async(function* () { let dbPath = OS.Path.join(OS.Constants.Path.profileDir, "places.sqlite"); yield OS.File.remove(dbPath); diff --git a/browser/components/places/tests/unit/test_browserGlue_corrupt.js b/browser/components/places/tests/unit/test_browserGlue_corrupt.js index 56e55c55d813..4460453eb794 100644 --- a/browser/components/places/tests/unit/test_browserGlue_corrupt.js +++ b/browser/components/places/tests/unit/test_browserGlue_corrupt.js @@ -41,7 +41,7 @@ add_task(function* test_main() { // The test will continue once restore has finished and smart bookmarks // have been created. - yield promiseEndUpdateBatch(); + yield promiseTopicObserved("places-browser-init-complete"); let bm = yield PlacesUtils.bookmarks.fetch({ parentGuid: PlacesUtils.bookmarks.toolbarGuid, diff --git a/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup.js b/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup.js index 532643e2faa0..50f5522eaf06 100644 --- a/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup.js +++ b/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup.js @@ -35,7 +35,7 @@ add_task(function* () { // The test will continue once import has finished and smart bookmarks // have been created. - yield promiseEndUpdateBatch(); + yield promiseTopicObserved("places-browser-init-complete"); let bm = yield PlacesUtils.bookmarks.fetch({ parentGuid: PlacesUtils.bookmarks.toolbarGuid, diff --git a/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js b/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js index 035172619d12..65750a914f4a 100644 --- a/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js +++ b/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js @@ -33,7 +33,7 @@ add_task(function* () { // The test will continue once import has finished and smart bookmarks // have been created. - yield promiseEndUpdateBatch(); + yield promiseTopicObserved("places-browser-init-complete"); let bm = yield PlacesUtils.bookmarks.fetch({ parentGuid: PlacesUtils.bookmarks.toolbarGuid, diff --git a/browser/components/places/tests/unit/test_browserGlue_migrate.js b/browser/components/places/tests/unit/test_browserGlue_migrate.js index 5d3ec13e1112..817f10c81a60 100644 --- a/browser/components/places/tests/unit/test_browserGlue_migrate.js +++ b/browser/components/places/tests/unit/test_browserGlue_migrate.js @@ -40,7 +40,7 @@ add_task(function* test_migrate_bookmarks() { title: "migrated" }); - let promise = promiseEndUpdateBatch(); + let promise = promiseTopicObserved("places-browser-init-complete"); bg.observe(null, "initial-migration-did-import-default-bookmarks", null); yield promise; diff --git a/browser/components/places/tests/unit/test_browserGlue_prefs.js b/browser/components/places/tests/unit/test_browserGlue_prefs.js index 77814581445d..49dbf2b42181 100644 --- a/browser/components/places/tests/unit/test_browserGlue_prefs.js +++ b/browser/components/places/tests/unit/test_browserGlue_prefs.js @@ -38,11 +38,9 @@ do_register_cleanup(function () { function simulatePlacesInit() { do_print("Simulate Places init"); - let promise = waitForImportAndSmartBookmarks(); - // Force nsBrowserGlue::_initPlaces(). bg.observe(null, TOPIC_BROWSERGLUE_TEST, TOPICDATA_FORCE_PLACES_INIT); - return promise; + return promiseTopicObserved("places-browser-init-complete"); } add_task(function* test_checkPreferences() { diff --git a/browser/components/places/tests/unit/test_browserGlue_restore.js b/browser/components/places/tests/unit/test_browserGlue_restore.js index b2614f4c5291..9482d81d46db 100644 --- a/browser/components/places/tests/unit/test_browserGlue_restore.js +++ b/browser/components/places/tests/unit/test_browserGlue_restore.js @@ -44,7 +44,7 @@ add_task(function* test_main() { // The test will continue once restore has finished and smart bookmarks // have been created. - yield promiseEndUpdateBatch(); + yield promiseTopicObserved("places-browser-init-complete"); let bm = yield PlacesUtils.bookmarks.fetch({ parentGuid: PlacesUtils.bookmarks.toolbarGuid, diff --git a/browser/components/places/tests/unit/test_browserGlue_smartBookmarks.js b/browser/components/places/tests/unit/test_browserGlue_smartBookmarks.js index e6498ed621c3..ca66889f878f 100644 --- a/browser/components/places/tests/unit/test_browserGlue_smartBookmarks.js +++ b/browser/components/places/tests/unit/test_browserGlue_smartBookmarks.js @@ -70,8 +70,6 @@ add_task(function* setup() { Assert.ok(!Services.prefs.getBoolPref(PREF_AUTO_EXPORT_HTML)); Assert.ok(!Services.prefs.getBoolPref(PREF_RESTORE_DEFAULT_BOOKMARKS)); Assert.throws(() => Services.prefs.getBoolPref(PREF_IMPORT_BOOKMARKS_HTML)); - - yield waitForImportAndSmartBookmarks(); }); add_task(function* test_version_0() { diff --git a/toolkit/components/places/Bookmarks.jsm b/toolkit/components/places/Bookmarks.jsm index 7de4e67052b0..2697e6c576f6 100644 --- a/toolkit/components/places/Bookmarks.jsm +++ b/toolkit/components/places/Bookmarks.jsm @@ -258,7 +258,7 @@ let Bookmarks = Object.freeze({ , validIf: b => b.lastModified >= item.dateAdded } }); - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); let parent; if (updateInfo.hasOwnProperty("parentGuid")) { if (item.type == this.TYPE_FOLDER) { @@ -426,7 +426,7 @@ let Bookmarks = Object.freeze({ * @resolves once the removal is complete. */ eraseEverything: Task.async(function* () { - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); yield db.executeTransaction(function* () { const folderGuids = [this.toolbarGuid, this.menuGuid, this.unfiledGuid]; yield removeFoldersContents(db, folderGuids); @@ -670,29 +670,11 @@ function notify(observers, notification, args) { } } -XPCOMUtils.defineLazyGetter(this, "DBConnPromised", - () => new Promise((resolve, reject) => { - Sqlite.wrapStorageConnection({ connection: PlacesUtils.history.DBConnection } ) - .then(db => { - try { - Sqlite.shutdown.addBlocker("Places Bookmarks.jsm wrapper closing", - db.close.bind(db)); - } - catch (ex) { - // It's too late to block shutdown, just close the connection. - db.close(); - reject(ex); - } - resolve(db); - }); - }) -); - //////////////////////////////////////////////////////////////////////////////// // Update implementation. function* updateBookmark(info, item, newParent) { - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); let tuples = new Map(); if (info.hasOwnProperty("lastModified")) @@ -779,7 +761,7 @@ function* updateBookmark(info, item, newParent) { // Insert implementation. function* insertBookmark(item, parent) { - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); // If a guid was not provided, generate one, so we won't need to fetch the // bookmark just after having created it. @@ -834,7 +816,7 @@ function* insertBookmark(item, parent) { // Fetch implementation. function* fetchBookmark(info) { - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); let rows = yield db.executeCached( `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index', @@ -852,7 +834,7 @@ function* fetchBookmark(info) { } function* fetchBookmarkByPosition(info) { - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); let index = info.index == Bookmarks.DEFAULT_INDEX ? null : info.index; let rows = yield db.executeCached( @@ -874,7 +856,7 @@ function* fetchBookmarkByPosition(info) { } function* fetchBookmarksByURL(info) { - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); let rows = yield db.executeCached( `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index', @@ -895,7 +877,7 @@ function* fetchBookmarksByURL(info) { } function* fetchBookmarksByParent(info) { - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); let rows = yield db.executeCached( `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index', @@ -917,7 +899,7 @@ function* fetchBookmarksByParent(info) { // Remove implementation. function* removeBookmark(item) { - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId; @@ -960,7 +942,7 @@ function* removeBookmark(item) { // Reorder implementation. function* reorderChildren(parent, orderedChildrenGuids) { - let db = yield DBConnPromised; + let db = yield PlacesUtils.promiseWrappedConnection(); return db.executeTransaction(function* () { // Select all of the direct children for the given parent. diff --git a/toolkit/components/places/PlacesCategoriesStarter.js b/toolkit/components/places/PlacesCategoriesStarter.js index b4378396507e..b611f337e56a 100644 --- a/toolkit/components/places/PlacesCategoriesStarter.js +++ b/toolkit/components/places/PlacesCategoriesStarter.js @@ -36,22 +36,25 @@ function PlacesCategoriesStarter() Services.obs.addObserver(this, PlacesUtils.TOPIC_SHUTDOWN, false); // nsINavBookmarkObserver implementation. - let notify = (function () { + let notify = () => { if (!this._notifiedBookmarksSvcReady) { + // TODO (bug 1145424): for whatever reason, even if we remove this + // component from the category (and thus from the category cache we use + // to notify), we keep being notified. + this._notifiedBookmarksSvcReady = true; // For perf reasons unregister from the category, since no further // notifications are needed. Cc["@mozilla.org/categorymanager;1"] .getService(Ci.nsICategoryManager) - .deleteCategoryEntry("bookmarks-observer", this, false); + .deleteCategoryEntry("bookmark-observers", "PlacesCategoriesStarter", false); // Directly notify PlacesUtils, to ensure it catches the notification. PlacesUtils.observe(null, "bookmarks-service-ready", null); } - }).bind(this); + }; + [ "onItemAdded", "onItemRemoved", "onItemChanged", "onBeginUpdateBatch", - "onEndUpdateBatch", "onItemVisited", - "onItemMoved" ].forEach(function(aMethod) { - this[aMethod] = notify; - }, this); + "onEndUpdateBatch", "onItemVisited", "onItemMoved" + ].forEach(aMethod => this[aMethod] = notify); } PlacesCategoriesStarter.prototype = { diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index 305ba3a4433f..03fc0ec4fb8a 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -27,6 +27,8 @@ this.EXPORTED_SYMBOLS = [ const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components; +Cu.importGlobalProperties(["URL"]); + Cu.import("resource://gre/modules/XPCOMUtils.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm"); @@ -73,6 +75,55 @@ function QI_node(aNode, aIID) { function asContainer(aNode) QI_node(aNode, Ci.nsINavHistoryContainerResultNode); function asQuery(aNode) QI_node(aNode, Ci.nsINavHistoryQueryResultNode); +/** + * Sends a bookmarks notification through the given observers. + * + * @param observers + * array of nsINavBookmarkObserver objects. + * @param notification + * the notification name. + * @param args + * array of arguments to pass to the notification. + */ +function notify(observers, notification, args) { + for (let observer of observers) { + try { + observer[notification](...args); + } catch (ex) {} + } +} + +/** + * Sends a keyword change notification. + * + * @param url + * the url to notify about. + * @param keyword + * The keyword to notify, or empty string if a keyword was removed. + */ +function* notifyKeywordChange(url, keyword) { + // Notify bookmarks about the removal. + let bookmarks = []; + yield PlacesUtils.bookmarks.fetch({ url }, b => bookmarks.push(b)); + // We don't want to yield in the gIgnoreKeywordNotifications section. + for (let bookmark of bookmarks) { + bookmark.id = yield PlacesUtils.promiseItemId(bookmark.guid); + bookmark.parentId = yield PlacesUtils.promiseItemId(bookmark.parentGuid); + } + let observers = PlacesUtils.bookmarks.getObservers(); + gIgnoreKeywordNotifications = true; + for (let bookmark of bookmarks) { + notify(observers, "onItemChanged", [ bookmark.id, "keyword", false, + keyword, + bookmark.lastModified * 1000, + bookmark.type, + bookmark.parentId, + bookmark.guid, bookmark.parentGuid + ]); + } + gIgnoreKeywordNotifications = false; +} + this.PlacesUtils = { // Place entries that are containers, e.g. bookmark folders or queries. TYPE_X_MOZ_PLACE_CONTAINER: "text/x-moz-place-container", @@ -244,6 +295,11 @@ this.PlacesUtils = { let observerInfo = this._bookmarksServiceObserversQueue.shift(); this.bookmarks.addObserver(observerInfo.observer, observerInfo.weak); } + + // Initialize the keywords cache to start observing bookmarks + // notifications. This is needed as far as we support both the old and + // the new bookmarking APIs at the same time. + gKeywordsCachePromise.catch(Cu.reportError); break; } }, @@ -810,19 +866,19 @@ this.PlacesUtils = { * Set the POST data associated with a bookmark, if any. * Used by POST keywords. * @param aBookmarkId - * @returns string of POST data */ setPostDataForBookmark(aBookmarkId, aPostData) { + if (!aPostData) + throw new Error("Must provide valid POST data"); // For now we don't have a unified API to create a keyword with postData, // thus here we can just try to complete a keyword that should already exist // without any post data. - let nullPostDataFragment = aPostData ? "AND post_data ISNULL" : ""; let stmt = PlacesUtils.history.DBConnection.createStatement( `UPDATE moz_keywords SET post_data = :post_data WHERE id = (SELECT k.id FROM moz_keywords k JOIN moz_bookmarks b ON b.fk = k.place_id WHERE b.id = :item_id - ${nullPostDataFragment} + AND post_data ISNULL LIMIT 1)`); stmt.params.item_id = aBookmarkId; stmt.params.post_data = aPostData; @@ -832,6 +888,21 @@ this.PlacesUtils = { finally { stmt.finalize(); } + + // Update the cache. + return Task.spawn(function* () { + let guid = yield PlacesUtils.promiseItemGuid(aBookmarkId); + let bm = yield PlacesUtils.bookmarks.fetch(guid); + + // Fetch keywords for this href. + let cache = yield gKeywordsCachePromise; + for (let [ keyword, entry ] of cache) { + // Set the POST data on keywords not having it. + if (entry.url.href == bm.url.href && !entry.postData) { + entry.postData = aPostData; + } + } + }).catch(Cu.reportError); }, /** @@ -1256,6 +1327,14 @@ this.PlacesUtils = { */ promiseDBConnection: () => gAsyncDBConnPromised, + /** + * Gets a Sqlite.jsm wrapped connection to the Places database. + * This is intended to be used mostly internally, and by other Places modules. + * Keep in mind the Places DB schema is by no means frozen or even stable. + * Your custom queries can - and will - break overtime. + */ + promiseWrappedConnection: () => gAsyncDBWrapperPromised, + /** * Given a uri returns list of itemIds associated to it. * @@ -1842,6 +1921,8 @@ XPCOMUtils.defineLazyServiceGetter(PlacesUtils, "livemarks", "@mozilla.org/browser/livemark-service;2", "mozIAsyncLivemarks"); +XPCOMUtils.defineLazyGetter(PlacesUtils, "keywords", () => Keywords); + XPCOMUtils.defineLazyGetter(PlacesUtils, "transactionManager", function() { let tm = Cc["@mozilla.org/transactionmanager;1"]. createInstance(Ci.nsITransactionManager); @@ -1881,24 +1962,266 @@ XPCOMUtils.defineLazyGetter(this, "bundle", function() { createBundle(PLACES_STRING_BUNDLE_URI); }); -XPCOMUtils.defineLazyGetter(this, "gAsyncDBConnPromised", () => { - let connPromised = Sqlite.cloneStorageConnection({ - connection: PlacesUtils.history.DBConnection, - readOnly: true }); - connPromised.then(conn => { - try { - Sqlite.shutdown.addBlocker("Places DB readonly connection closing", - conn.close.bind(conn)); +XPCOMUtils.defineLazyGetter(this, "gAsyncDBConnPromised", + () => new Promise((resolve) => { + Sqlite.cloneStorageConnection({ + connection: PlacesUtils.history.DBConnection, + readOnly: true + }).then(conn => { + try { + Sqlite.shutdown.addBlocker( + "PlacesUtils read-only connection closing", + conn.close.bind(conn)); + } catch(ex) { + // It's too late to block shutdown, just close the connection. + conn.close(); + throw ex; + } + resolve(conn); + }); + }) +); + +XPCOMUtils.defineLazyGetter(this, "gAsyncDBWrapperPromised", + () => new Promise((resolve) => { + Sqlite.wrapStorageConnection({ + connection: PlacesUtils.history.DBConnection, + }).then(conn => { + try { + Sqlite.shutdown.addBlocker( + "PlacesUtils wrapped connection closing", + conn.close.bind(conn)); + } catch(ex) { + // It's too late to block shutdown, just close the connection. + conn.close(); + throw ex; + } + resolve(conn); + }); + }) +); + +/** + * Keywords management API. + * Sooner or later these keywords will merge with search keywords, this is an + * interim API that should then be replaced by a unified one. + * Keywords are associated with URLs and can have POST data. + * A single URL can have multiple keywords, provided they differ by POST data. + */ +let Keywords = { + /** + * Fetches URL and postData for a given keyword. + * + * @param keyword + * The keyword to fetch. + * @return {Promise} + * @resolves to an object in the form: { keyword, url, postData }, + * or null if a keyword was not found. + */ + fetch(keyword) { + if (!keyword || typeof(keyword) != "string") + throw new Error("Invalid keyword"); + keyword = keyword.trim().toLowerCase(); + return gKeywordsCachePromise.then(cache => cache.get(keyword) || null); + }, + + /** + * Adds a new keyword and postData for the given URL. + * + * @param keywordEntry + * An object describing the keyword to insert, in the form: + * { + * keyword: non-empty string, + * URL: URL or href to associate to the keyword, + * postData: optional POST data to associate to the keyword + * } + * @note Do not define a postData property if there isn't any POST data. + * @resolves when the addition is complete. + */ + insert(keywordEntry) { + if (!keywordEntry || typeof keywordEntry != "object") + throw new Error("Input should be a valid object"); + + if (!("keyword" in keywordEntry) || !keywordEntry.keyword || + typeof(keywordEntry.keyword) != "string") + throw new Error("Invalid keyword"); + if (("postData" in keywordEntry) && keywordEntry.postData && + typeof(keywordEntry.postData) != "string") + throw new Error("Invalid POST data"); + if (!("url" in keywordEntry)) + throw new Error("undefined is not a valid URL"); + let { keyword, url } = keywordEntry; + keyword = keyword.trim().toLowerCase(); + let postData = keywordEntry.postData || null; + // This also checks href for validity + url = new URL(url); + + return Task.spawn(function* () { + let cache = yield gKeywordsCachePromise; + + // Trying to set the same keyword is a no-op. + let oldEntry = cache.get(keyword); + if (oldEntry && oldEntry.url.href == url.href && + oldEntry.postData == keywordEntry.postData) { + return; + } + + // A keyword can only be associated to a single page. + // If another page is using the new keyword, we must update the keyword + // entry. + // Note we cannot use INSERT OR REPLACE cause it wouldn't invoke the delete + // trigger. + let db = yield PlacesUtils.promiseWrappedConnection(); + if (oldEntry) { + yield db.executeCached( + `UPDATE moz_keywords + SET place_id = (SELECT id FROM moz_places WHERE url = :url), + post_data = :post_data + WHERE keyword = :keyword + `, { url: url.href, keyword: keyword, post_data: postData }); + yield notifyKeywordChange(oldEntry.url.href, ""); + } else { + // An entry for the given page could be missing, in such a case we need to + // create it. + yield db.executeCached( + `INSERT OR IGNORE INTO moz_places (url, rev_host, hidden, frecency, guid) + VALUES (:url, :rev_host, 0, :frecency, GENERATE_GUID()) + `, { url: url.href, rev_host: PlacesUtils.getReversedHost(url), + frecency: url.protocol == "place:" ? 0 : -1 }); + yield db.executeCached( + `INSERT INTO moz_keywords (keyword, place_id, post_data) + VALUES (:keyword, (SELECT id FROM moz_places WHERE url = :url), :post_data) + `, { url: url.href, keyword: keyword, post_data: postData }); + } + + cache.set(keyword, { keyword, url, postData }); + + // In any case, notify about the new keyword. + yield notifyKeywordChange(url.href, keyword); + }.bind(this)); + }, + + /** + * Removes a keyword. + * + * @param keyword + * The keyword to remove. + * @return {Promise} + * @resolves when the removal is complete. + */ + remove(keyword) { + if (!keyword || typeof(keyword) != "string") + throw new Error("Invalid keyword"); + keyword = keyword.trim().toLowerCase(); + return Task.spawn(function* () { + let cache = yield gKeywordsCachePromise; + if (!cache.has(keyword)) + return; + let { url } = cache.get(keyword); + cache.delete(keyword); + + let db = yield PlacesUtils.promiseWrappedConnection(); + yield db.execute(`DELETE FROM moz_keywords WHERE keyword = :keyword`, + { keyword }); + + // Notify bookmarks about the removal. + yield notifyKeywordChange(url.href, ""); + }.bind(this)); + } +}; + +// Set by the keywords API to distinguish notifications fired by the old API. +// Once the old API will be gone, we can remove this and stop observing. +let gIgnoreKeywordNotifications = false; + +XPCOMUtils.defineLazyGetter(this, "gKeywordsCachePromise", Task.async(function* () { + let cache = new Map(); + let db = yield PlacesUtils.promiseWrappedConnection(); + let rows = yield db.execute( + `SELECT keyword, url, post_data + FROM moz_keywords k + JOIN moz_places h ON h.id = k.place_id + `); + for (let row of rows) { + let keyword = row.getResultByName("keyword"); + let entry = { keyword, + url: new URL(row.getResultByName("url")), + postData: row.getResultByName("post_data") }; + cache.set(keyword, entry); + } + + // Helper to get a keyword from an href. + function keywordsForHref(href) { + let keywords = []; + for (let [ key, val ] of cache) { + if (val.url.href == href) + keywords.push(key); } - catch(ex) { - // It's too late to block shutdown, just close the connection. - return conn.close(); - throw (ex); + return keywords; + } + + // Start observing changes to bookmarks. For now we are going to keep that + // relation for backwards compatibility reasons, but mostly because we are + // lacking a UI to manage keywords directly. + let observer = { + QueryInterface: XPCOMUtils.generateQI(Ci.nsINavBookmarkObserver), + onBeginUpdateBatch() {}, + onEndUpdateBatch() {}, + onItemAdded() {}, + onItemVisited() {}, + onItemMoved() {}, + + onItemRemoved(id, parentId, index, itemType, uri, guid, parentGuid) { + if (itemType != PlacesUtils.bookmarks.TYPE_BOOKMARK) + return; + + let keywords = keywordsForHref(uri.spec); + // This uri has no keywords associated, so there's nothing to do. + if (keywords.length == 0) + return; + + Task.spawn(function* () { + // If the uri is not bookmarked anymore, we can remove this keyword. + let bookmark = yield PlacesUtils.bookmarks.fetch({ url: uri }); + if (!bookmark) { + for (let keyword of keywords) { + yield PlacesUtils.keywords.remove(keyword); + } + } + }).catch(Cu.reportError); + }, + + onItemChanged(id, prop, isAnno, val, lastMod, itemType, parentId, guid) { + if (gIgnoreKeywordNotifications || + prop != "keyword") + return; + + Task.spawn(function* () { + let bookmark = yield PlacesUtils.bookmarks.fetch(guid); + // By this time the bookmark could have gone, there's nothing we can do. + if (!bookmark) + return; + + if (val.length == 0) { + // We are removing a keyword. + let keywords = keywordsForHref(bookmark.url.href) + for (let keyword of keywords) { + cache.delete(keyword); + } + } else { + // We are adding a new keyword. + cache.set(val, { keyword: val, url: bookmark.url }); + } + }).catch(Cu.reportError); } - return Promise.resolve(); - }).then(null, Cu.reportError); - return connPromised; -}); + }; + + PlacesUtils.bookmarks.addObserver(observer, false); + PlacesUtils.registerShutdownFunction(() => { + PlacesUtils.bookmarks.removeObserver(observer); + }); + return cache; +})); // Sometime soon, likely as part of the transition to mozIAsyncBookmarks, // itemIds will be deprecated in favour of GUIDs, which play much better @@ -2919,15 +3242,19 @@ this.PlacesEditBookmarkPostDataTransaction = PlacesEditBookmarkPostDataTransaction.prototype = { __proto__: BaseTransaction.prototype, - doTransaction: function EBPDTXN_doTransaction() - { - this.item.postData = PlacesUtils.getPostDataForBookmark(this.item.id); - PlacesUtils.setPostDataForBookmark(this.item.id, this.new.postData); + doTransaction() { + // Setting null postData is not supported by the current schema. + if (this.new.postData) { + this.item.postData = PlacesUtils.getPostDataForBookmark(this.item.id); + PlacesUtils.setPostDataForBookmark(this.item.id, this.new.postData); + } }, - undoTransaction: function EBPDTXN_undoTransaction() - { - PlacesUtils.setPostDataForBookmark(this.item.id, this.item.postData); + undoTransaction() { + // Setting null postData is not supported by the current schema. + if (this.item.postData) { + PlacesUtils.setPostDataForBookmark(this.item.id, this.item.postData); + } } }; diff --git a/toolkit/components/places/nsNavBookmarks.cpp b/toolkit/components/places/nsNavBookmarks.cpp index 7161ca458c82..c66116dd00ad 100644 --- a/toolkit/components/places/nsNavBookmarks.cpp +++ b/toolkit/components/places/nsNavBookmarks.cpp @@ -622,13 +622,8 @@ nsNavBookmarks::RemoveItem(int64_t aItemId) rv = history->UpdateFrecency(bookmark.placeId); NS_ENSURE_SUCCESS(rv, rv); } - // A broken url should not interrupt the removal process. - rv = NS_NewURI(getter_AddRefs(uri), bookmark.url); - if (NS_SUCCEEDED(rv)) { - rv = UpdateKeywordsForRemovedBookmark(bookmark); - NS_ENSURE_SUCCESS(rv, rv); - } + (void)NS_NewURI(getter_AddRefs(uri), bookmark.url); } NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers, @@ -1102,13 +1097,8 @@ nsNavBookmarks::RemoveFolderChildren(int64_t aFolderId) rv = history->UpdateFrecency(child.placeId); NS_ENSURE_SUCCESS(rv, rv); } - // A broken url should not interrupt the removal process. - rv = NS_NewURI(getter_AddRefs(uri), child.url); - if (NS_SUCCEEDED(rv)) { - rv = UpdateKeywordsForRemovedBookmark(child); - NS_ENSURE_SUCCESS(rv, rv); - } + (void)NS_NewURI(getter_AddRefs(uri), child.url); } NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers, @@ -2238,74 +2228,6 @@ nsNavBookmarks::SetItemIndex(int64_t aItemId, int32_t aNewIndex) } -nsresult -nsNavBookmarks::UpdateKeywordsForRemovedBookmark(const BookmarkData& aBookmark) -{ - // If there are no keywords for this URI, there's nothing to do. - nsCOMPtr uri; - nsresult rv = NS_NewURI(getter_AddRefs(uri), aBookmark.url); - NS_ENSURE_SUCCESS(rv, rv); - - nsTArray keywords; - { - nsCOMPtr stmt = mDB->GetStatement( - "SELECT keyword FROM moz_keywords WHERE place_id = :place_id " - ); - NS_ENSURE_STATE(stmt); - mozStorageStatementScoper scoper(stmt); - rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("place_id"), aBookmark.placeId); - NS_ENSURE_SUCCESS(rv, rv); - - bool hasMore; - while (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) { - nsAutoString keyword; - rv = stmt->GetString(0, keyword); - NS_ENSURE_SUCCESS(rv, rv); - keywords.AppendElement(keyword); - } - } - - if (keywords.Length() == 0) { - // This uri has no keywords associated, so there's nothing to do. - return NS_OK; - } - - // If the uri is not bookmarked anymore, we can remove its keywords. - nsTArray bookmarks; - rv = GetBookmarksForURI(uri, bookmarks); - NS_ENSURE_SUCCESS(rv, rv); - if (bookmarks.Length() == 0) { - for (uint32_t i = 0; i < keywords.Length(); ++i) { - nsString keyword = keywords[i]; - - nsCOMPtr stmt = mDB->GetStatement( - "DELETE FROM moz_keywords WHERE keyword = :keyword " - ); - NS_ENSURE_STATE(stmt); - mozStorageStatementScoper scoper(stmt); - rv = stmt->BindStringByName(NS_LITERAL_CSTRING("keyword"), keyword); - NS_ENSURE_SUCCESS(rv, rv); - rv = stmt->Execute(); - NS_ENSURE_SUCCESS(rv, rv); - } - - NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers, - nsINavBookmarkObserver, - OnItemChanged(aBookmark.id, - NS_LITERAL_CSTRING("keyword"), - false, - EmptyCString(), - aBookmark.lastModified, - TYPE_BOOKMARK, - aBookmark.parentId, - aBookmark.guid, - aBookmark.parentGuid)); - } - - return NS_OK; -} - - NS_IMETHODIMP nsNavBookmarks::SetKeywordForBookmark(int64_t aBookmarkId, const nsAString& aUserCasedKeyword) diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks.js b/toolkit/components/places/tests/bookmarks/test_bookmarks.js index cc4dfdd029b0..8911f68a5805 100644 --- a/toolkit/components/places/tests/bookmarks/test_bookmarks.js +++ b/toolkit/components/places/tests/bookmarks/test_bookmarks.js @@ -321,31 +321,25 @@ add_task(function test_bookmarks() { // test setKeywordForBookmark let kwTestItemId = bs.insertBookmark(testRoot, uri("http://keywordtest.com"), bs.DEFAULT_INDEX, ""); - try { - let dateAdded = bs.getItemDateAdded(kwTestItemId); - // after just inserting, modified should not be set - let lastModified = bs.getItemLastModified(kwTestItemId); - do_check_eq(lastModified, dateAdded); - // Workaround possible VM timers issues moving lastModified and dateAdded - // to the past. - lastModified -= 1000; - bs.setItemLastModified(kwTestItemId, --lastModified); - dateAdded -= 1000; - bs.setItemDateAdded(kwTestItemId, dateAdded); - - bs.setKeywordForBookmark(kwTestItemId, "bar"); - - let lastModified2 = bs.getItemLastModified(kwTestItemId); - LOG("test setKeywordForBookmark"); - LOG("dateAdded = " + dateAdded); - LOG("lastModified = " + lastModified); - LOG("lastModified2 = " + lastModified2); - do_check_true(is_time_ordered(lastModified, lastModified2)); - do_check_true(is_time_ordered(dateAdded, lastModified2)); - } catch(ex) { - do_throw("setKeywordForBookmark: " + ex); - } + dateAdded = bs.getItemDateAdded(kwTestItemId); + // after just inserting, modified should not be set + lastModified = bs.getItemLastModified(kwTestItemId); + do_check_eq(lastModified, dateAdded); + // Workaround possible VM timers issues moving lastModified and dateAdded + // to the past. + lastModified -= 1000; + bs.setItemLastModified(kwTestItemId, lastModified); + dateAdded -= 1000; + bs.setItemDateAdded(kwTestItemId, dateAdded); + bs.setKeywordForBookmark(kwTestItemId, "bar"); + lastModified2 = bs.getItemLastModified(kwTestItemId); + LOG("test setKeywordForBookmark"); + LOG("dateAdded = " + dateAdded); + LOG("lastModified = " + lastModified); + LOG("lastModified2 = " + lastModified2); + do_check_true(is_time_ordered(lastModified, lastModified2)); + do_check_true(is_time_ordered(dateAdded, lastModified2)); let lastModified3 = bs.getItemLastModified(kwTestItemId); // test getKeywordForBookmark diff --git a/toolkit/components/places/tests/bookmarks/test_keywords.js b/toolkit/components/places/tests/bookmarks/test_keywords.js index 242fa4977e0d..b3e728b89a7f 100644 --- a/toolkit/components/places/tests/bookmarks/test_keywords.js +++ b/toolkit/components/places/tests/bookmarks/test_keywords.js @@ -286,17 +286,12 @@ add_task(function test_addRemoveBookmark() { "keyword", false, "keyword", bookmark.lastModified, bookmark.type, parentId, - bookmark.guid, bookmark.parentGuid ] }, - { name: "onItemChanged", - arguments: [ itemId, - "keyword", false, "", - bookmark.lastModified, bookmark.type, - parentId, bookmark.guid, bookmark.parentGuid ] } ]); check_keyword(URI3, null); - Assert.equal((yield foreign_count(URI3)), fc); + // Don't check the foreign count since the process is async. + // The new test_keywords.js in unit is checking this though. yield PlacesTestUtils.promiseAsyncUpdates(); check_orphans(); diff --git a/toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js b/toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js index 62fb1a97f75c..6b1d516631a9 100644 --- a/toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js +++ b/toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js @@ -48,7 +48,7 @@ add_task(function* test_keyword_searc() { do_print("Keyword that happens to match a page"); yield check_autocomplete({ search: "key ThisPageIsInHistory", - matches: [ { uri: NetUtil.newURI("http://abc/?search=ThisPageIsInHistory"), title: "Generic page title", style: ["bookmark"] } ] + matches: [ { uri: NetUtil.newURI("http://abc/?search=ThisPageIsInHistory"), title: "Generic page title", style: ["keyword"] } ] }); do_print("Keyword without query (without space)"); diff --git a/toolkit/components/places/tests/unit/test_421180.js b/toolkit/components/places/tests/unit/test_421180.js deleted file mode 100644 index 69cac5662abd..000000000000 --- a/toolkit/components/places/tests/unit/test_421180.js +++ /dev/null @@ -1,94 +0,0 @@ -/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ -/* vim:set ts=2 sw=2 sts=2 et: */ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -// Get bookmarks service -try { - var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. - getService(Ci.nsINavBookmarksService); -} -catch(ex) { - do_throw("Could not get bookmarks service\n"); -} - -// Get database connection -try { - var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"]. - getService(Ci.nsINavHistoryService); - var mDBConn = histsvc.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection; -} -catch(ex) { - do_throw("Could not get database connection\n"); -} - -add_test(function test_keywordRemovedOnUniqueItemRemoval() { - var bookmarkedURI = uri("http://foo.bar"); - var keyword = "testkeyword"; - - // TEST 1 - // 1. add a bookmark - // 2. add a keyword to it - // 3. remove bookmark - // 4. check that keyword has gone - var bookmarkId = bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder, - bookmarkedURI, - bmsvc.DEFAULT_INDEX, - "A bookmark"); - bmsvc.setKeywordForBookmark(bookmarkId, keyword); - // remove bookmark - bmsvc.removeItem(bookmarkId); - - PlacesTestUtils.promiseAsyncUpdates().then(() => { - // Check that keyword has been removed from the database. - // The removal is asynchronous. - var sql = "SELECT id FROM moz_keywords WHERE keyword = ?1"; - var stmt = mDBConn.createStatement(sql); - stmt.bindByIndex(0, keyword); - do_check_false(stmt.executeStep()); - stmt.finalize(); - - run_next_test(); - }); -}); - -add_test(function test_keywordNotRemovedOnNonUniqueItemRemoval() { - var bookmarkedURI = uri("http://foo.bar"); - var keyword = "testkeyword"; - - // TEST 2 - // 1. add 2 bookmarks - // 2. add the same keyword to them - // 3. remove first bookmark - // 4. check that keyword is still there - var bookmarkId1 = bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder, - bookmarkedURI, - bmsvc.DEFAULT_INDEX, - "A bookmark"); - bmsvc.setKeywordForBookmark(bookmarkId1, keyword); - - var bookmarkId2 = bmsvc.insertBookmark(bmsvc.toolbarFolder, - bookmarkedURI, - bmsvc.DEFAULT_INDEX, - keyword); - bmsvc.setKeywordForBookmark(bookmarkId2, keyword); - - // remove first bookmark - bmsvc.removeItem(bookmarkId1); - - PlacesTestUtils.promiseAsyncUpdates().then(() => { - // check that keyword is still there - var sql = "SELECT id FROM moz_keywords WHERE keyword = ?1"; - var stmt = mDBConn.createStatement(sql); - stmt.bindByIndex(0, keyword); - do_check_true(stmt.executeStep()); - stmt.finalize(); - - run_next_test(); - }); -}); - -function run_test() { - run_next_test(); -} diff --git a/toolkit/components/places/tests/unit/test_keywords.js b/toolkit/components/places/tests/unit/test_keywords.js new file mode 100644 index 000000000000..90cf8fba266e --- /dev/null +++ b/toolkit/components/places/tests/unit/test_keywords.js @@ -0,0 +1,488 @@ +"use strict" + +function* check_keyword(aExpectExists, aHref, aKeyword, aPostData = null) { + // Check case-insensitivity. + aKeyword = aKeyword.toUpperCase(); + + let entry = yield PlacesUtils.keywords.fetch(aKeyword); + if (aExpectExists) { + Assert.ok(!!entry, "A keyword should exist"); + Assert.equal(entry.url.href, aHref); + Assert.equal(entry.postData, aPostData); + } else { + Assert.ok(!entry || entry.url.href != aHref, + "The given keyword entry should not exist"); + } +} + +/** + * Polls the keywords cache waiting for the given keyword entry. + */ +function* promiseKeyword(keyword, expectedHref) { + let href = null; + do { + yield new Promise(resolve => do_timeout(100, resolve)); + let entry = yield PlacesUtils.keywords.fetch(keyword); + if (entry) + href = entry.url.href; + } while (href != expectedHref); +} + +function* check_no_orphans() { + let db = yield PlacesUtils.promiseDBConnection(); + let rows = yield db.executeCached( + `SELECT id FROM moz_keywords k + WHERE NOT EXISTS (SELECT 1 FROM moz_places WHERE id = k.place_id) + `); + Assert.equal(rows.length, 0); +} + +function expectBookmarkNotifications() { + let notifications = []; + let observer = new Proxy(NavBookmarkObserver, { + get(target, name) { + if (name == "check") { + PlacesUtils.bookmarks.removeObserver(observer); + return expectedNotifications => + Assert.deepEqual(notifications, expectedNotifications); + } + + if (name.startsWith("onItemChanged")) { + return (itemId, property) => { + if (property != "keyword") + return; + let args = Array.from(arguments, arg => { + if (arg && arg instanceof Ci.nsIURI) + return new URL(arg.spec); + if (arg && typeof(arg) == "number" && arg >= Date.now() * 1000) + return new Date(parseInt(arg/1000)); + return arg; + }); + notifications.push({ name: name, arguments: args }); + } + } + + if (name in target) + return target[name]; + return undefined; + } + }); + PlacesUtils.bookmarks.addObserver(observer, false); + return observer; +} + +add_task(function* test_invalid_input() { + Assert.throws(() => PlacesUtils.keywords.fetch(null), + /Invalid keyword/); + Assert.throws(() => PlacesUtils.keywords.fetch(""), + /Invalid keyword/); + Assert.throws(() => PlacesUtils.keywords.fetch(5), + /Invalid keyword/); + + Assert.throws(() => PlacesUtils.keywords.insert(null), + /Input should be a valid object/); + Assert.throws(() => PlacesUtils.keywords.insert("test"), + /Input should be a valid object/); + Assert.throws(() => PlacesUtils.keywords.insert(undefined), + /Input should be a valid object/); + Assert.throws(() => PlacesUtils.keywords.insert({ }), + /Invalid keyword/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: null }), + /Invalid keyword/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: 5 }), + /Invalid keyword/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: "" }), + /Invalid keyword/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: "test", postData: 5 }), + /Invalid POST data/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: "test", postData: {} }), + /Invalid POST data/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: "test" }), + /is not a valid URL/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: "test", url: 5 }), + /is not a valid URL/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: "test", url: "" }), + /is not a valid URL/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: "test", url: null }), + /is not a valid URL/); + Assert.throws(() => PlacesUtils.keywords.insert({ keyword: "test", url: "mozilla" }), + /is not a valid URL/); + + Assert.throws(() => PlacesUtils.keywords.remove(null), + /Invalid keyword/); + Assert.throws(() => PlacesUtils.keywords.remove(""), + /Invalid keyword/); + Assert.throws(() => PlacesUtils.keywords.remove(5), + /Invalid keyword/); +}); + +add_task(function* test_addKeyword() { + yield check_keyword(false, "http://example.com/", "keyword"); + let fc = yield foreign_count("http://example.com/"); + let observer = expectBookmarkNotifications(); + + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" }); + observer.check([]); + + yield check_keyword(true, "http://example.com/", "keyword"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 1); // +1 keyword + + // Now remove the keyword. + observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.remove("keyword"); + observer.check([]); + + yield check_keyword(false, "http://example.com/", "keyword"); + Assert.equal((yield foreign_count("http://example.com/")), fc); // -1 keyword + + // Check using URL. + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: new URL("http://example.com/") }); + yield check_keyword(true, "http://example.com/", "keyword"); + yield PlacesUtils.keywords.remove("keyword"); + yield check_keyword(false, "http://example.com/", "keyword"); + + yield check_no_orphans(); +}); + +add_task(function* test_addBookmarkAndKeyword() { + yield check_keyword(false, "http://example.com/", "keyword"); + let fc = yield foreign_count("http://example.com/"); + let bookmark = yield PlacesUtils.bookmarks.insert({ url: "http://example.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + + let observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" }); + + observer.check([{ name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark.guid)), + "keyword", false, "keyword", + bookmark.lastModified, bookmark.type, + (yield PlacesUtils.promiseItemId(bookmark.parentGuid)), + bookmark.guid, bookmark.parentGuid ] } ]); + + yield check_keyword(true, "http://example.com/", "keyword"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 2); // +1 bookmark +1 keyword + + // Now remove the keyword. + observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.remove("keyword"); + + observer.check([{ name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark.guid)), + "keyword", false, "", + bookmark.lastModified, bookmark.type, + (yield PlacesUtils.promiseItemId(bookmark.parentGuid)), + bookmark.guid, bookmark.parentGuid ] } ]); + + yield check_keyword(false, "http://example.com/", "keyword"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 1); // -1 keyword + + // Add again the keyword, then remove the bookmark. + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" }); + + observer = expectBookmarkNotifications(); + yield PlacesUtils.bookmarks.remove(bookmark.guid); + // the notification is synchronous but the removal process is async. + // Unfortunately there's nothing explicit we can wait for. + while ((yield foreign_count("http://example.com/"))); + // We don't get any itemChanged notification since the bookmark has been + // removed already. + observer.check([]); + + yield check_keyword(false, "http://example.com/", "keyword"); + + yield check_no_orphans(); +}); + +add_task(function* test_addKeywordToURIHavingKeyword() { + yield check_keyword(false, "http://example.com/", "keyword"); + let fc = yield foreign_count("http://example.com/"); + + let observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" }); + observer.check([]); + + yield check_keyword(true, "http://example.com/", "keyword"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 1); // +1 keyword + + yield PlacesUtils.keywords.insert({ keyword: "keyword2", url: "http://example.com/" }); + + yield check_keyword(true, "http://example.com/", "keyword"); + yield check_keyword(true, "http://example.com/", "keyword2"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 2); // +1 keyword + + // Now remove the keywords. + observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.remove("keyword"); + yield PlacesUtils.keywords.remove("keyword2"); + observer.check([]); + + yield check_keyword(false, "http://example.com/", "keyword"); + yield check_keyword(false, "http://example.com/", "keyword2"); + Assert.equal((yield foreign_count("http://example.com/")), fc); // -1 keyword + + yield check_no_orphans(); +}); + +add_task(function* test_addBookmarkToURIHavingKeyword() { + yield check_keyword(false, "http://example.com/", "keyword"); + let fc = yield foreign_count("http://example.com/"); + let observer = expectBookmarkNotifications(); + + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" }); + observer.check([]); + + yield check_keyword(true, "http://example.com/", "keyword"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 1); // +1 keyword + + observer = expectBookmarkNotifications(); + let bookmark = yield PlacesUtils.bookmarks.insert({ url: "http://example.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + Assert.equal((yield foreign_count("http://example.com/")), fc + 2); // +1 bookmark + observer.check([]); + + observer = expectBookmarkNotifications(); + yield PlacesUtils.bookmarks.remove(bookmark.guid); + // the notification is synchronous but the removal process is async. + // Unfortunately there's nothing explicit we can wait for. + while ((yield foreign_count("http://example.com/"))); + // We don't get any itemChanged notification since the bookmark has been + // removed already. + observer.check([]); + + yield check_keyword(false, "http://example.com/", "keyword"); + + yield check_no_orphans(); +}); + +add_task(function* test_sameKeywordDifferentURL() { + let fc1 = yield foreign_count("http://example1.com/"); + let bookmark1 = yield PlacesUtils.bookmarks.insert({ url: "http://example1.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + let fc2 = yield foreign_count("http://example2.com/"); + let bookmark2 = yield PlacesUtils.bookmarks.insert({ url: "http://example2.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example1.com/" }); + + yield check_keyword(true, "http://example1.com/", "keyword"); + Assert.equal((yield foreign_count("http://example1.com/")), fc1 + 2); // +1 bookmark +1 keyword + yield check_keyword(false, "http://example2.com/", "keyword"); + Assert.equal((yield foreign_count("http://example2.com/")), fc2 + 1); // +1 bookmark + + // Assign the same keyword to another url. + let observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example2.com/" }); + + observer.check([{ name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark1.guid)), + "keyword", false, "", + bookmark1.lastModified, bookmark1.type, + (yield PlacesUtils.promiseItemId(bookmark1.parentGuid)), + bookmark1.guid, bookmark1.parentGuid ] }, + { name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark2.guid)), + "keyword", false, "keyword", + bookmark2.lastModified, bookmark2.type, + (yield PlacesUtils.promiseItemId(bookmark2.parentGuid)), + bookmark2.guid, bookmark2.parentGuid ] } ]); + + yield check_keyword(false, "http://example1.com/", "keyword"); + Assert.equal((yield foreign_count("http://example1.com/")), fc1 + 1); // -1 keyword + yield check_keyword(true, "http://example2.com/", "keyword"); + Assert.equal((yield foreign_count("http://example2.com/")), fc2 + 2); // +1 keyword + + // Now remove the keyword. + observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.remove("keyword"); + observer.check([{ name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark2.guid)), + "keyword", false, "", + bookmark2.lastModified, bookmark2.type, + (yield PlacesUtils.promiseItemId(bookmark2.parentGuid)), + bookmark2.guid, bookmark2.parentGuid ] } ]); + + yield check_keyword(false, "http://example1.com/", "keyword"); + yield check_keyword(false, "http://example2.com/", "keyword"); + Assert.equal((yield foreign_count("http://example1.com/")), fc1 + 1); + Assert.equal((yield foreign_count("http://example2.com/")), fc2 + 1); // -1 keyword + + yield PlacesUtils.bookmarks.remove(bookmark1); + yield PlacesUtils.bookmarks.remove(bookmark2); + Assert.equal((yield foreign_count("http://example1.com/")), fc1); // -1 bookmark + while ((yield foreign_count("http://example2.com/"))); // -1 keyword + + yield check_no_orphans(); +}); + +add_task(function* test_sameURIDifferentKeyword() { + let fc = yield foreign_count("http://example.com/"); + + let observer = expectBookmarkNotifications(); + let bookmark = yield PlacesUtils.bookmarks.insert({ url: "http://example.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + yield PlacesUtils.keywords.insert({keyword: "keyword", url: "http://example.com/" }); + + yield check_keyword(true, "http://example.com/", "keyword"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 2); // +1 bookmark +1 keyword + + observer.check([{ name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark.guid)), + "keyword", false, "keyword", + bookmark.lastModified, bookmark.type, + (yield PlacesUtils.promiseItemId(bookmark.parentGuid)), + bookmark.guid, bookmark.parentGuid ] } ]); + + observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.insert({ keyword: "keyword2", url: "http://example.com/" }); + yield check_keyword(true, "http://example.com/", "keyword"); + yield check_keyword(true, "http://example.com/", "keyword2"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 3); // +1 keyword + observer.check([{ name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark.guid)), + "keyword", false, "keyword2", + bookmark.lastModified, bookmark.type, + (yield PlacesUtils.promiseItemId(bookmark.parentGuid)), + bookmark.guid, bookmark.parentGuid ] } ]); + + // Add a third keyword. + yield PlacesUtils.keywords.insert({ keyword: "keyword3", url: "http://example.com/" }); + yield check_keyword(true, "http://example.com/", "keyword"); + yield check_keyword(true, "http://example.com/", "keyword2"); + yield check_keyword(true, "http://example.com/", "keyword3"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 4); // +1 keyword + + // Remove one of the keywords. + observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.remove("keyword"); + yield check_keyword(false, "http://example.com/", "keyword"); + yield check_keyword(true, "http://example.com/", "keyword2"); + yield check_keyword(true, "http://example.com/", "keyword3"); + observer.check([{ name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark.guid)), + "keyword", false, "", + bookmark.lastModified, bookmark.type, + (yield PlacesUtils.promiseItemId(bookmark.parentGuid)), + bookmark.guid, bookmark.parentGuid ] } ]); + Assert.equal((yield foreign_count("http://example.com/")), fc + 3); // -1 keyword + + // Now remove the bookmark. + yield PlacesUtils.bookmarks.remove(bookmark); + while ((yield foreign_count("http://example.com/"))); + yield check_keyword(false, "http://example.com/", "keyword"); + yield check_keyword(false, "http://example.com/", "keyword2"); + yield check_keyword(false, "http://example.com/", "keyword3"); + + check_no_orphans(); +}); + +add_task(function* test_deleteKeywordMultipleBookmarks() { + let fc = yield foreign_count("http://example.com/"); + + let observer = expectBookmarkNotifications(); + let bookmark1 = yield PlacesUtils.bookmarks.insert({ url: "http://example.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + let bookmark2 = yield PlacesUtils.bookmarks.insert({ url: "http://example.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" }); + + yield check_keyword(true, "http://example.com/", "keyword"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 3); // +2 bookmark +1 keyword + observer.check([{ name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark2.guid)), + "keyword", false, "keyword", + bookmark2.lastModified, bookmark2.type, + (yield PlacesUtils.promiseItemId(bookmark2.parentGuid)), + bookmark2.guid, bookmark2.parentGuid ] }, + { name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark1.guid)), + "keyword", false, "keyword", + bookmark1.lastModified, bookmark1.type, + (yield PlacesUtils.promiseItemId(bookmark1.parentGuid)), + bookmark1.guid, bookmark1.parentGuid ] } ]); + + observer = expectBookmarkNotifications(); + yield PlacesUtils.keywords.remove("keyword"); + yield check_keyword(false, "http://example.com/", "keyword"); + Assert.equal((yield foreign_count("http://example.com/")), fc + 2); // -1 keyword + observer.check([{ name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark2.guid)), + "keyword", false, "", + bookmark2.lastModified, bookmark2.type, + (yield PlacesUtils.promiseItemId(bookmark2.parentGuid)), + bookmark2.guid, bookmark2.parentGuid ] }, + { name: "onItemChanged", + arguments: [ (yield PlacesUtils.promiseItemId(bookmark1.guid)), + "keyword", false, "", + bookmark1.lastModified, bookmark1.type, + (yield PlacesUtils.promiseItemId(bookmark1.parentGuid)), + bookmark1.guid, bookmark1.parentGuid ] } ]); + + // Now remove the bookmarks. + yield PlacesUtils.bookmarks.remove(bookmark1); + yield PlacesUtils.bookmarks.remove(bookmark2); + Assert.equal((yield foreign_count("http://example.com/")), fc); // -2 bookmarks + + check_no_orphans(); +}); + +add_task(function* test_multipleKeywordsSamePostData() { + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/", postData: "postData1" }); + yield check_keyword(true, "http://example.com/", "keyword", "postData1"); + // Add another keyword with same postData, should fail. + yield Assert.rejects(PlacesUtils.keywords.insert({ keyword: "keyword2", url: "http://example.com/", postData: "postData1" }), + /constraint failed/); + yield check_keyword(false, "http://example.com/", "keyword2", "postData1"); + + yield PlacesUtils.keywords.remove("keyword"); + + check_no_orphans(); +}); + +add_task(function* test_oldPostDataAPI() { + let bookmark = yield PlacesUtils.bookmarks.insert({ url: "http://example.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" }); + let itemId = yield PlacesUtils.promiseItemId(bookmark.guid); + yield PlacesUtils.setPostDataForBookmark(itemId, "postData"); + yield check_keyword(true, "http://example.com/", "keyword", "postData"); + Assert.equal(PlacesUtils.getPostDataForBookmark(itemId), "postData"); + + yield PlacesUtils.keywords.remove("keyword"); + yield PlacesUtils.bookmarks.remove(bookmark); + + check_no_orphans(); +}); + +add_task(function* test_oldKeywordsAPI() { + let bookmark = yield PlacesUtils.bookmarks.insert({ url: "http://example.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + yield check_keyword(false, "http://example.com/", "keyword"); + let itemId = yield PlacesUtils.promiseItemId(bookmark.guid); + + PlacesUtils.bookmarks.setKeywordForBookmark(itemId, "keyword"); + yield promiseKeyword("keyword", "http://example.com/"); + + // Remove the keyword. + PlacesUtils.bookmarks.setKeywordForBookmark(itemId, ""); + yield promiseKeyword("keyword", null); + + yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com" }); + Assert.equal(PlacesUtils.bookmarks.getKeywordForBookmark(itemId), "keyword"); + Assert.equal(PlacesUtils.bookmarks.getURIForKeyword("keyword").spec, "http://example.com/"); + yield PlacesUtils.bookmarks.remove(bookmark); + + check_no_orphans(); +}); + +function run_test() { + run_next_test(); +} diff --git a/toolkit/components/places/tests/unit/test_placesTxn.js b/toolkit/components/places/tests/unit/test_placesTxn.js index f8bb1aa83b7c..9c8979fe2990 100644 --- a/toolkit/components/places/tests/unit/test_placesTxn.js +++ b/toolkit/components/places/tests/unit/test_placesTxn.js @@ -732,8 +732,8 @@ add_test(function test_edit_postData() { txn.undoTransaction(); [url, post_data] = PlacesUtils.getURLAndPostDataForKeyword("kw"); Assert.equal(url, testURI.spec); - Assert.equal(null, post_data); - + // We don't allow anymore to set a null post data. + //Assert.equal(null, post_data); run_next_test(); }); diff --git a/toolkit/components/places/tests/unit/xpcshell.ini b/toolkit/components/places/tests/unit/xpcshell.ini index d815f91b5efb..5aa5be74d0c8 100644 --- a/toolkit/components/places/tests/unit/xpcshell.ini +++ b/toolkit/components/places/tests/unit/xpcshell.ini @@ -44,7 +44,6 @@ skip-if = os == "android" [test_419731.js] [test_419792_node_tags_property.js] [test_420331_wyciwyg.js] -[test_421180.js] [test_425563.js] [test_429505_remove_shortcuts.js] [test_433317_query_title_update.js] @@ -104,6 +103,7 @@ skip-if = os == "android" [test_hosts_triggers.js] [test_isURIVisited.js] [test_isvisited.js] +[test_keywords.js] [test_lastModified.js] [test_markpageas.js] [test_mozIAsyncLivemarks.js]