Bug 731274 - Make reloadLivemarks optionally force reloads and use it to speed up livemarks population.
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 08 Mar 2012 11:14:59 +0100
changeset 91413 91a9294ae936c989e2c55f1c78f2a0666c6df46f
parent 91412 6baa03ac1a09072dae4742e66a52b48311d20cfd
child 91414 5bb189b6b0b91fe0c2f4d23facf34312195cb6cc
push idunknown
push userunknown
push dateunknown
bugs731274
milestone13.0a1
Bug 731274 - Make reloadLivemarks optionally force reloads and use it to speed up livemarks population. r=dietrich sr=gavin
browser/components/places/content/browserPlacesViews.js
browser/components/places/content/treeView.js
toolkit/components/places/mozIAsyncLivemarks.idl
toolkit/components/places/nsLivemarkService.js
toolkit/components/places/tests/chrome/Makefile.in
toolkit/components/places/tests/chrome/test_reloadLivemarks.xul
toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js
toolkit/components/places/tests/unit/test_null_interfaces.js
--- a/browser/components/places/content/browserPlacesViews.js
+++ b/browser/components/places/content/browserPlacesViews.js
@@ -652,17 +652,19 @@ PlacesViewBase.prototype = {
         PlacesUtils.livemarks.getLivemark({ id: aPlacesNode.itemId },
           (function (aStatus, aLivemark) {
             if (Components.isSuccessCode(aStatus)) {
               let shouldInvalidate = !aPlacesNode._feedURI;
               aPlacesNode._feedURI = aLivemark.feedURI;
               aPlacesNode._siteURI = aLivemark.siteURI;
               if (aNewState == Ci.nsINavHistoryContainerResultNode.STATE_OPENED) {
                 aLivemark.registerForUpdates(aPlacesNode, this);
+                // Prioritize the current livemark.
                 aLivemark.reload();
+                PlacesUtils.livemarks.reloadLivemarks();
                 if (shouldInvalidate)
                   this.invalidateContainer(aPlacesNode);
               }
               else {
                 aLivemark.unregisterForUpdates(aPlacesNode);
               }
             }
           }).bind(this)
--- a/browser/components/places/content/treeView.js
+++ b/browser/components/places/content/treeView.js
@@ -891,17 +891,19 @@ PlacesTreeView.prototype = {
 
       PlacesUtils.livemarks.getLivemark({ id: aNode.itemId },
         (function (aStatus, aLivemark) {
           if (Components.isSuccessCode(aStatus)) {
             let shouldInvalidate = !aNode._feedURI;
             aNode._feedURI = aLivemark.feedURI;
             if (aNewState == Components.interfaces.nsINavHistoryContainerResultNode.STATE_OPENED) {
               aLivemark.registerForUpdates(aNode, this);
+              // Prioritize the current livemark.
               aLivemark.reload();
+              PlacesUtils.livemarks.reloadLivemarks();
               if (shouldInvalidate)
                 this.invalidateContainer(aNode);
             }
             else {
               aLivemark.unregisterForUpdates(aNode);
             }
           }
         }).bind(this)
--- a/toolkit/components/places/mozIAsyncLivemarks.idl
+++ b/toolkit/components/places/mozIAsyncLivemarks.idl
@@ -11,17 +11,17 @@
 interface nsIURI;
 
 interface mozILivemarkCallback;
 interface mozILivemarkInfo;
 interface mozILivemark;
 
 interface nsINavHistoryResultObserver;
 
-[scriptable, uuid(addaa7c5-bd85-4c83-9c21-81c8a825c358)]
+[scriptable, uuid(1dbf174c-696e-4d9b-af0f-350da50d2249)]
 interface mozIAsyncLivemarks : nsISupports
 {
   /**
    * Creates a new livemark
    *
    * @param aLivemarkInfo
    *        mozILivemarkInfo object containing at least title, parentId,
    *        index and feedURI of the livemark to create.
@@ -62,19 +62,25 @@ interface mozIAsyncLivemarks : nsISuppor
    *
    * @throws NS_ERROR_INVALID_ARG if the id/guid is invalid or an invalid
    *         callback is provided.
    */
   void getLivemark(in jsval aLivemarkInfo,
                    in mozILivemarkCallback aCallback);
 
   /**
-   * Forces a reload of all livemarks, whether or not they've expired.
+   * Reloads all livemarks if they are expired or if forced to do so.
+   *
+   * @param [optional]aForceUpdate
+   *        If set to true forces a reload even if contents are still valid.
+   *
+   * @note The update process is asynchronous, observers registered through
+   *       registerForUpdates will be notified of updated contents.
    */
-  void reloadLivemarks();
+  void reloadLivemarks([optional]in boolean aForceUpdate);
 };
 
 [scriptable, function, uuid(62a426f9-39a6-42f0-ad48-b7404d48188f)]
 interface mozILivemarkCallback : nsISupports
 {
   /**
    * Invoked when a livemark is added, removed or retrieved.
    *
--- a/toolkit/components/places/nsLivemarkService.js
+++ b/toolkit/components/places/nsLivemarkService.js
@@ -146,25 +146,27 @@ LivemarkService.prototype = {
     }
     else {
       // The callbacks should always be enqueued per the interface.
       // Just enque on the main thread.
       Services.tm.mainThread.dispatch(aCallback, Ci.nsIThread.DISPATCH_NORMAL);
     }
   },
 
+  _reloading: false,
   _startReloadTimer: function LS__startReloadTimer()
   {
     if (this._reloadTimer) {
       this._reloadTimer.cancel();
     }
     else {
       this._reloadTimer = Cc["@mozilla.org/timer;1"]
                             .createInstance(Ci.nsITimer);
     }
+    this._reloading = true;
     this._reloadTimer.initWithCallback(this._reloadNextLivemark.bind(this),
                                        RELOAD_DELAY_MS,
                                        Ci.nsITimer.TYPE_ONE_SHOT);
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// nsIObserver
 
@@ -174,16 +176,17 @@ LivemarkService.prototype = {
       if (this._pendingStmt) {
         this._pendingStmt.cancel();
         this._pendingStmt = null;
         // Initialization never finished, so just bail out.
         return;
       }
 
       if (this._reloadTimer) {
+        this._reloading = false;
         this._reloadTimer.cancel();
         delete this._reloadTimer;
       }
 
       // Stop any ongoing update.
       for each (let livemark in this._livemarks) {
         livemark.terminate();
       }
@@ -358,17 +361,17 @@ LivemarkService.prototype = {
     }
     this._livemarks[aId].reload();
   },
 
   reloadAllLivemarks: function DEPRECATED_LS_reloadAllLivemarks()
   {
     this._reportDeprecatedMethod();
 
-    this._reloadLivemarks();
+    this._reloadLivemarks(true);
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// mozIAsyncLivemarks
 
   addLivemark: function LS_addLivemark(aLivemarkInfo,
                                        aLivemarkCallback)
   {
@@ -388,17 +391,25 @@ LivemarkService.prototype = {
     let result = Cr.NS_OK;
     let livemark = null;
     try {
       // Disallow adding a livemark inside another livemark.
       if (aLivemarkInfo.parentId in this._livemarks) {
         throw new Components.Exception("", Cr.NS_ERROR_INVALID_ARG);
       }
 
-      livemark = new Livemark(aLivemarkInfo);
+      // Don't pass unexpected input data to the livemark constructor.
+      livemark = new Livemark({ title:        aLivemarkInfo.title
+                              , parentId:     aLivemarkInfo.parentId
+                              , index:        aLivemarkInfo.index
+                              , feedURI:      aLivemarkInfo.feedURI
+                              , siteURI:      aLivemarkInfo.siteURI
+                              , guid:         aLivemarkInfo.guid
+                              , lastModified: aLivemarkInfo.lastModified
+                              });
       if (this._itemAdded && this._itemAdded.id == livemark.id) {
         livemark.index = this._itemAdded.index;
         if (!aLivemarkInfo.guid) {
           livemark.guid = this._itemAdded.guid;
         }
         if (!aLivemarkInfo.lastModified) {
           livemark.lastModified = this._itemAdded.lastModified;
         }
@@ -464,30 +475,41 @@ LivemarkService.prototype = {
         });
       }
     }
   },
 
   _reloaded: [],
   _reloadNextLivemark: function LS__reloadNextLivemark()
   {
+    this._reloading = false;
     // Find first livemark to be reloaded.
     for (let id in this._livemarks) {
       if (this._reloaded.indexOf(id) == -1) {
         this._reloaded.push(id);
-        this._livemarks[id].reload();
+        this._livemarks[id].reload(this._forceUpdate);
         this._startReloadTimer();
         break;
       }
     }
   },
 
-  reloadLivemarks: function LS_reloadLivemarks()
+  reloadLivemarks: function LS_reloadLivemarks(aForceUpdate)
   {
+    // Check if there's a currently running reload, to save some useless work.
+    let notWorthRestarting =
+      this._forceUpdate || // We're already forceUpdating.
+      !aForceUpdate;       // The caller didn't request a forced update.
+    if (this._reloading && notWorthRestarting) {
+      // Ignore this call.
+      return;
+    } 
+
     this._onCacheReady((function LS_reloadAllLivemarks_ETAT() {
+      this._forceUpdate = !!aForceUpdate;
       this._reloaded = [];
       // Livemarks reloads happen on a timer, and are delayed for performance
       // reasons.
       this._startReloadTimer();
     }).bind(this));
   },
 
   getLivemark: function LS_getLivemark(aLivemarkInfo, aLivemarkCallback)
@@ -836,16 +858,20 @@ Livemark.prototype = {
 
     // Check the TTL/expiration on this, to check if there is no need to update
     // this livemark.
     if (!aForceUpdate && this.children.length && this.expireTime > Date.now())
       return;
 
     this.status = Ci.mozILivemark.STATUS_LOADING;
 
+    // Setting the status notifies observers that may remove the livemark.
+    if (this._terminated)
+      return;
+
     try {
       // Create a load group for the request.  This will allow us to
       // automatically keep track of redirects, so we can always
       // cancel the channel.
       let loadgroup = Cc["@mozilla.org/network/load-group;1"].
                       createInstance(Ci.nsILoadGroup);
       let channel = NetUtil.newChannel(this.feedURI.spec).
                     QueryInterface(Ci.nsIHttpChannel);
@@ -998,16 +1024,18 @@ Livemark.prototype = {
   },
 
   /**
    * Terminates the livemark entry, cancelling any ongoing load.
    * Must be invoked before destroying the entry.
    */
   terminate: function LM_terminate()
   {
+    // Avoid handling any updateChildren request from now on.
+    this._terminated = true;
     // Clear the list before aborting, since abort() would try to set the
     // status and notify about it, but that's not really useful at this point.
     this._resultObserversList = [];
     this.abort();
   },
 
   /**
    * Aborts the livemark loading if needed.
--- a/toolkit/components/places/tests/chrome/Makefile.in
+++ b/toolkit/components/places/tests/chrome/Makefile.in
@@ -56,16 +56,17 @@ include $(topsrcdir)/config/rules.mk
 _CHROME_FILES	= \
 		test_371798.xul \
 		test_342484.xul \
 		test_341972a.xul \
 		test_341972b.xul \
 		test_favicon_annotations.xul \
 		test_303567.xul \
 		test_381357.xul \
+		test_reloadLivemarks.xul \
 		$(NULL)
 
 libs:: $(_HTTP_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
 
 libs:: $(_CHROME_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)
 
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/chrome/test_reloadLivemarks.xul
@@ -0,0 +1,136 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+<?xml-stylesheet
+  href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
+<window title="Reload Livemarks"
+  xmlns:html="http://www.w3.org/1999/xhtml"
+  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+  onload="runTest()">
+  <script type="application/javascript"
+   src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+
+  <body xmlns="http://www.w3.org/1999/xhtml" />
+
+<script type="application/javascript">
+<![CDATA[
+// Test that for concurrent reload of livemarks.
+
+SimpleTest.waitForExplicitFinish();
+
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+const Cr = Components.results;
+
+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+Components.utils.import("resource://gre/modules/NetUtil.jsm");
+Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
+
+let gLivemarks = [
+  { id: -1,
+    title: "foo",
+    parentId: PlacesUtils.toolbarFolderId,
+    index: PlacesUtils.bookmarks.DEFAULT_INDEX,
+    feedURI: NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/chrome/link-less-items.rss")
+  },
+  { id: -1,
+    title: "bar",
+    parentId: PlacesUtils.toolbarFolderId,
+    index: PlacesUtils.bookmarks.DEFAULT_INDEX,
+    feedURI: NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/chrome/link-less-items-no-site-uri.rss")
+  },
+];
+
+function runTest()
+{
+  addLivemarks(function () {
+    reloadLivemarks(false, function () {
+      reloadLivemarks(true, function () {
+        removeLivemarks(SimpleTest.finish);
+      });
+    });
+    // Ensure this normal reload doesn't overwrite the forced one.
+    PlacesUtils.livemarks.reloadLivemarks();
+  });
+}
+
+function addLivemarks(aCallback) {
+  info("Adding livemarks");
+  let count = gLivemarks.length;
+  gLivemarks.forEach(function(aLivemarkData) {
+    PlacesUtils.livemarks.addLivemark(aLivemarkData,
+      function (aStatus, aLivemark) {
+        ok(Components.isSuccessCode(aStatus), "Add livemark should succeed");
+        aLivemarkData.id = aLivemark.id;
+        if (--count == 0) {
+          aCallback();
+        }
+      }
+    );
+  });
+}
+
+function reloadLivemarks(aForceUpdate, aCallback) {
+  info("Reloading livemarks with forceUpdate: " + aForceUpdate);
+  let count = gLivemarks.length;
+  gLivemarks.forEach(function(aLivemarkData) {
+    PlacesUtils.livemarks.getLivemark(aLivemarkData,
+      function (aStatus, aLivemark) {
+        ok(Components.isSuccessCode(aStatus), "Get livemark should succeed");
+        aLivemarkData._observer = new resultObserver(aLivemark, function() {
+          if (++count == gLivemarks.length) {
+            aCallback();
+          }
+        });
+        if (--count == 0) {
+          PlacesUtils.livemarks.reloadLivemarks(aForceUpdate);
+        }
+      }
+    );
+  });
+}
+
+function removeLivemarks(aCallback) {
+  info("Removing livemarks");
+  let count = gLivemarks.length;
+  gLivemarks.forEach(function(aLivemarkData) {
+    PlacesUtils.livemarks.removeLivemark(aLivemarkData,
+      function (aStatus, aLivemark) {
+        ok(Components.isSuccessCode(aStatus), "Remove livemark should succeed");
+        if (--count == 0) {
+          aCallback();
+        }
+      }
+    );
+  });
+}
+
+function resultObserver(aLivemark, aCallback) {
+  this._node = {};
+  this._livemark = aLivemark;
+  this._callback = aCallback;
+  this._livemark.registerForUpdates(this._node, this);
+}
+resultObserver.prototype = {
+  nodeInserted: function() {},
+  nodeRemoved: function() {},
+  nodeAnnotationChanged: function() {},
+  nodeTitleChanged: function() {},
+  nodeHistoryDetailsChanged: function() {},
+  nodeReplaced: function() {},
+  nodeMoved: function() {},
+  ontainerStateChanged: function () {},
+  sortingChanged: function() {},
+  batching: function() {},
+  invalidateContainer: function(aContainer) {
+    // Wait for load finish.
+    if (this._livemark.status == Ci.mozILivemark.STATUS_LOADING)
+      return;
+
+    this._livemark.unregisterForUpdates(this._node);
+    this._callback();
+  }
+};
+
+]]>
+</script>
+</window>
--- a/toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js
+++ b/toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js
@@ -238,16 +238,34 @@ add_test(function test_addLivemark_callb
                                                 PlacesUtils.LMANNO_FEEDURI));
     do_check_true(PlacesUtils.annotations
                              .itemHasAnnotation(aLivemark.id,
                                                 PlacesUtils.LMANNO_SITEURI));
     run_next_test();
   });
 });
 
+add_test(function test_addLivemark_bogusid_callback_succeeds()
+{
+  PlacesUtils.livemarks.addLivemark({ id: 100 // Should be ignored.
+                                    , title: "test"
+                                    , parentId: PlacesUtils.unfiledBookmarksFolderId
+                                    , index: PlacesUtils.bookmarks.DEFAULT_INDEX
+                                    , feedURI: FEED_URI
+                                    , siteURI: SITE_URI
+                                    }, function (aStatus, aLivemark)
+  {
+    do_check_true(Components.isSuccessCode(aStatus));
+    do_check_true(aLivemark.id > 0);
+    do_check_neq(aLivemark.id, 100);
+
+    run_next_test();
+  });
+});
+
 add_test(function test_addLivemark_bogusParent_callback_fails()
 {
   PlacesUtils.livemarks.addLivemark({ title: "test"
                                     , parentId: 187
                                     , index: PlacesUtils.bookmarks.DEFAULT_INDEX
                                     , feedURI: FEED_URI
                                     }, function (aStatus, aLivemark)
   {
--- a/toolkit/components/places/tests/unit/test_null_interfaces.js
+++ b/toolkit/components/places/tests/unit/test_null_interfaces.js
@@ -51,17 +51,17 @@ let _ = function(some, debug, text, to) 
   "and an array of function names that don't throw when passed nulls");
 let testServices = [
   ["browser/nav-history-service;1", "nsINavHistoryService",
     ["queryStringToQueries", "removePagesByTimeframe", "removePagesFromHost",
      "removeVisitsByTimeframe"]],
   ["browser/nav-bookmarks-service;1","nsINavBookmarksService",
     ["createFolder", "getItemIdForGUID"]],
   ["browser/livemark-service;2","nsILivemarkService", []],
-  ["browser/livemark-service;2","mozIAsyncLivemarks", []],
+  ["browser/livemark-service;2","mozIAsyncLivemarks", ["reloadLivemarks"]],
   ["browser/annotation-service;1","nsIAnnotationService", []],
   ["browser/favicon-service;1","nsIFaviconService", []],
   ["browser/tagging-service;1","nsITaggingService", []],
 ];
 _(testServices.join("\n"));
 
 function run_test()
 {