From 1a6fda01c63b1c873821a1795ad97671a133daab Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Thu, 27 Oct 2011 11:11:45 +0200 Subject: [PATCH] Bug 692120 - The star button doesn't need to observe bookmarks changes till bookmarks service is alive. r=dietrich --- browser/base/content/browser-places.js | 4 +- .../places/PlacesCategoriesStarter.js | 18 +++++ toolkit/components/places/PlacesUtils.jsm | 70 +++++++++++++++++-- .../components/places/nsLivemarkService.js | 4 +- toolkit/components/places/nsNavBookmarks.cpp | 8 +-- .../unit/test_PlacesUtils_lazyobservers.js | 34 +++++++++ .../components/places/tests/unit/xpcshell.ini | 1 + .../components/places/toolkitplaces.manifest | 1 + 8 files changed, 125 insertions(+), 15 deletions(-) create mode 100644 toolkit/components/places/tests/unit/test_PlacesUtils_lazyobservers.js diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js index 3c32815b4896..66cf691f2a1a 100644 --- a/browser/base/content/browser-places.js +++ b/browser/base/content/browser-places.js @@ -971,7 +971,7 @@ var PlacesStarButton = { uninit: function PSB_uninit() { if (this._hasBookmarksObserver) { - PlacesUtils.bookmarks.removeObserver(this); + PlacesUtils.removeLazyBookmarkObserver(this); } if (this._pendingStmt) { this._pendingStmt.cancel(); @@ -1035,7 +1035,7 @@ var PlacesStarButton = { // Start observing bookmarks if needed. if (!this._hasBookmarksObserver) { try { - PlacesUtils.bookmarks.addObserver(this, false); + PlacesUtils.addLazyBookmarkObserver(this); this._hasBookmarksObserver = true; } catch(ex) { Components.utils.reportError("PlacesStarButton failed adding a bookmarks observer: " + ex); diff --git a/toolkit/components/places/PlacesCategoriesStarter.js b/toolkit/components/places/PlacesCategoriesStarter.js index c0c7f93df81d..be8b33ba8863 100644 --- a/toolkit/components/places/PlacesCategoriesStarter.js +++ b/toolkit/components/places/PlacesCategoriesStarter.js @@ -66,6 +66,23 @@ function PlacesCategoriesStarter() { Services.obs.addObserver(this, TOPIC_GATHER_TELEMETRY, false); Services.obs.addObserver(this, PlacesUtils.TOPIC_SHUTDOWN, false); + + // nsINavBookmarkObserver implementation. + let notify = (function () { + if (!this._notifiedBookmarksSvcReady) { + // 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); + Services.obs.notifyObservers(null, "bookmarks-service-ready", null); + } + }).bind(this); + [ "onItemAdded", "onItemRemoved", "onItemChanged", "onBeginUpdateBatch", + "onEndUpdateBatch", "onBeforeItemRemoved", "onItemVisited", + "onItemMoved" ].forEach(function(aMethod) { + this[aMethod] = notify; + }, this); } PlacesCategoriesStarter.prototype = { @@ -106,6 +123,7 @@ PlacesCategoriesStarter.prototype = { QueryInterface: XPCOMUtils.generateQI([ Ci.nsIObserver + , Ci.nsINavBookmarkObserver ]) }; diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index 89d21c88ba86..c227ca963036 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -315,9 +315,22 @@ var PlacesUtils = { //// nsIObserver observe: function PU_observe(aSubject, aTopic, aData) { - if (aTopic == this.TOPIC_SHUTDOWN) { - Services.obs.removeObserver(this, this.TOPIC_SHUTDOWN); - this._shutdownFunctions.forEach(function (aFunc) aFunc.apply(this), this); + switch (aTopic) { + case this.TOPIC_SHUTDOWN: + Services.obs.removeObserver(this, this.TOPIC_SHUTDOWN); + this._shutdownFunctions.forEach(function (aFunc) aFunc.apply(this), this); + if (this._bookmarksServiceObserversQueue.length > 0) { + Services.obs.removeObserver(this, "bookmarks-service-ready", false); + this._bookmarksServiceObserversQueue.length = 0; + } + break; + case "bookmarks-service-ready": + Services.obs.removeObserver(this, "bookmarks-service-ready", false); + while (this._bookmarksServiceObserversQueue.length > 0) { + let observer = this._bookmarksServiceObserversQueue.shift(); + this.bookmarks.addObserver(observer, false); + } + break; } }, @@ -2116,7 +2129,56 @@ var PlacesUtils = { } } }); - } + }, + + _isServiceInstantiated: function PU__isServiceInstantiated(aContractID) { + try { + return Components.manager + .QueryInterface(Ci.nsIServiceManager) + .isServiceInstantiatedByContractID(aContractID, + Ci.nsISupports); + } catch (ex) {} + return false; + }, + + /** + * Lazily adds a bookmarks observer, waiting for the bookmarks service to be + * alive before registering the observer. This is especially useful in the + * startup path, to avoid initializing the service just to add an observer. + * + * @param aObserver + * Object implementing nsINavBookmarkObserver + * @note Correct functionality of lazy observers relies on the fact Places + * notifies categories before real observers, and uses + * PlacesCategoriesStarter component to kick-off the registration. + */ + _bookmarksServiceObserversQueue: [], + addLazyBookmarkObserver: + function PU_addLazyBookmarkObserver(aObserver) { + if (this._isServiceInstantiated("@mozilla.org/browser/nav-bookmarks-service;1")) { + this.bookmarks.addObserver(aObserver, false); + return; + } + Services.obs.addObserver(this, "bookmarks-service-ready", false); + this._bookmarksServiceObserversQueue.push(aObserver); + }, + /** + * Removes a bookmarks observer added through addLazyBookmarkObserver. + * + * @param aObserver + * Object implementing nsINavBookmarkObserver + */ + removeLazyBookmarkObserver: + function PU_removeLazyBookmarkObserver(aObserver) { + if (this._bookmarksServiceObserversQueue.length == 0) { + this.bookmarks.removeObserver(aObserver, false); + return; + } + let index = this._bookmarksServiceObserversQueue.indexOf(aObserver); + if (index != -1) { + this._bookmarksServiceObserversQueue.splice(index, 1); + } + }, }; /** diff --git a/toolkit/components/places/nsLivemarkService.js b/toolkit/components/places/nsLivemarkService.js index 57377a2c9c1d..6f29d59256e7 100644 --- a/toolkit/components/places/nsLivemarkService.js +++ b/toolkit/components/places/nsLivemarkService.js @@ -139,7 +139,7 @@ function LivemarkService() { Services.obs.addObserver(this, PlacesUtils.TOPIC_SHUTDOWN, false); // Observe bookmarks changes. - PlacesUtils.bookmarks.addObserver(this, false); + PlacesUtils.addLazyBookmarkObserver(this); } LivemarkService.prototype = { @@ -194,7 +194,7 @@ LivemarkService.prototype = { if (aTopic == PlacesUtils.TOPIC_SHUTDOWN) { Services.obs.removeObserver(this, aTopic); // Remove bookmarks observer. - PlacesUtils.bookmarks.removeObserver(this); + PlacesUtils.removeLazyBookmarkObserver(this); // Stop updating livemarks. this.stopUpdateLivemarks(); } diff --git a/toolkit/components/places/nsNavBookmarks.cpp b/toolkit/components/places/nsNavBookmarks.cpp index 56b25bc71d60..de2ceb6d0a44 100644 --- a/toolkit/components/places/nsNavBookmarks.cpp +++ b/toolkit/components/places/nsNavBookmarks.cpp @@ -301,14 +301,8 @@ nsNavBookmarks::Init() nsresult nsNavBookmarks::InitRoots(bool aForceCreate) { - // Get a read-only cloned connection to increase concurrency. - // It will be closed on destruction. - nsCOMPtr DBConn; - mDB->MainConn()->Clone(true, getter_AddRefs(DBConn)); - NS_ENSURE_STATE(DBConn); - nsCOMPtr stmt; - nsresult rv = DBConn->CreateStatement(NS_LITERAL_CSTRING( + nsresult rv = mDB->MainConn()->CreateStatement(NS_LITERAL_CSTRING( "SELECT root_name, folder_id FROM moz_bookmarks_roots" ), getter_AddRefs(stmt)); NS_ENSURE_SUCCESS(rv, rv); diff --git a/toolkit/components/places/tests/unit/test_PlacesUtils_lazyobservers.js b/toolkit/components/places/tests/unit/test_PlacesUtils_lazyobservers.js new file mode 100644 index 000000000000..3fb79f544327 --- /dev/null +++ b/toolkit/components/places/tests/unit/test_PlacesUtils_lazyobservers.js @@ -0,0 +1,34 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +function run_test() { + const TEST_URI = NetUtil.newURI("http://moz.org/") + let observer = { + QueryInterface: XPCOMUtils.generateQI([ + Ci.nsINavBookmarkObserver, + ]), + + onBeginUpdateBatch: function () {}, + onEndUpdateBatch: function () {}, + onItemAdded: function (aItemId, aParentId, aIndex, aItemType, aURI) { + do_check_true(aURI.equals(TEST_URI)); + PlacesUtils.removeLazyBookmarkObserver(this); + do_test_finished(); + }, + onBeforeItemRemoved: function () {}, + onItemRemoved: function () {}, + onItemChanged: function () {}, + onItemVisited: function () {}, + onItemMoved: function () {}, + }; + + // Check registration and removal with uninitialized bookmarks service. + PlacesUtils.addLazyBookmarkObserver(observer); + PlacesUtils.removeLazyBookmarkObserver(observer); + + PlacesUtils.addLazyBookmarkObserver(observer); + PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId, + TEST_URI, + PlacesUtils.bookmarks.DEFAULT_INDEX, + "Bookmark title"); +} diff --git a/toolkit/components/places/tests/unit/xpcshell.ini b/toolkit/components/places/tests/unit/xpcshell.ini index fc3cab0c9158..7ba78d8dc257 100644 --- a/toolkit/components/places/tests/unit/xpcshell.ini +++ b/toolkit/components/places/tests/unit/xpcshell.ini @@ -125,4 +125,5 @@ skip-if = os == "android" [test_utils_getURLsForContainerNode.js] [test_utils_setAnnotationsFor.js] [test_PlacesUtils_asyncGetBookmarkIds.js] +[test_PlacesUtils_lazyobservers.js] [test_telemetry.js] diff --git a/toolkit/components/places/toolkitplaces.manifest b/toolkit/components/places/toolkitplaces.manifest index d7b4459fb846..b315e2d59558 100644 --- a/toolkit/components/places/toolkitplaces.manifest +++ b/toolkit/components/places/toolkitplaces.manifest @@ -17,3 +17,4 @@ category history-observers nsPlacesExpiration @mozilla.org/places/expiration;1 component {803938d5-e26d-4453-bf46-ad4b26e41114} PlacesCategoriesStarter.js contract @mozilla.org/places/categoriesStarter;1 {803938d5-e26d-4453-bf46-ad4b26e41114} category idle-daily PlacesCategoriesStarter @mozilla.org/places/categoriesStarter;1 +category bookmark-observers PlacesCategoriesStarter @mozilla.org/places/categoriesStarter;1