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

@@ -657,7 +657,9 @@ PlacesViewBase.prototype = {
               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);
               }

browser/components/places/content/treeView.js

@@ -896,7 +896,9 @@ PlacesTreeView.prototype = {
             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);
             }

toolkit/components/places/mozIAsyncLivemarks.idl

@@ -16,7 +16,7 @@ interface mozILivemark;
 
 interface nsINavHistoryResultObserver;
 
-[scriptable, uuid(addaa7c5-bd85-4c83-9c21-81c8a825c358)]
+[scriptable, uuid(1dbf174c-696e-4d9b-af0f-350da50d2249)]
 interface mozIAsyncLivemarks : nsISupports
 {
   /**
@@ -67,9 +67,15 @@ interface mozIAsyncLivemarks : nsISupports
                    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)]

toolkit/components/places/nsLivemarkService.js

@@ -151,6 +151,7 @@ LivemarkService.prototype = {
     }
   },
 
+  _reloading: false,
   _startReloadTimer: function LS__startReloadTimer()
   {
     if (this._reloadTimer) {
@@ -160,6 +161,7 @@ LivemarkService.prototype = {
       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);
@@ -179,6 +181,7 @@ LivemarkService.prototype = {
       }
 
       if (this._reloadTimer) {
+        this._reloading = false;
         this._reloadTimer.cancel();
         delete this._reloadTimer;
       }
@@ -363,7 +366,7 @@ LivemarkService.prototype = {
   {
     this._reportDeprecatedMethod();
 
-    this._reloadLivemarks();
+    this._reloadLivemarks(true);
   },
 
   //////////////////////////////////////////////////////////////////////////////
@@ -393,7 +396,15 @@ LivemarkService.prototype = {
         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) {
@@ -469,20 +480,31 @@ 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.
@@ -841,6 +863,10 @@ Livemark.prototype = {
 
     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
@@ -1003,6 +1029,8 @@ Livemark.prototype = {
    */
   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 = [];

toolkit/components/places/tests/chrome/Makefile.in

@@ -61,6 +61,7 @@ _CHROME_FILES	= \
 		test_favicon_annotations.xul \
 		test_303567.xul \
 		test_381357.xul \
+		test_reloadLivemarks.xul \
 		$(NULL)
 
 libs:: $(_HTTP_FILES)

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>

toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js

@@ -243,6 +243,24 @@ add_test(function test_addLivemark_callback_succeeds()
   });
 });
 
+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"

toolkit/components/places/tests/unit/test_null_interfaces.js

@@ -56,7 +56,7 @@ let testServices = [
   ["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", []],