From 9a6a4b23e7c10b1b18be78c26a56a7b189231988 Mon Sep 17 00:00:00 2001
From: Marco Bonardo <mbonardo@mozilla.com>
Date: Mon, 3 Jun 2013 12:17:36 +0200
Subject: [PATCH] Bug 874407 - new visits are inserted incorrectly in the
 Library and the sidebar treeviews. r=Mano

---
 browser/components/places/content/treeView.js |   8 +-
 .../components/places/tests/chrome/head.js    |  14 +++
 .../places/tests/chrome/test_bug549192.xul    | 100 +++++++++---------
 3 files changed, 71 insertions(+), 51 deletions(-)

diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js
index c7975a8d973..a197f556bd8 100644
--- a/browser/components/places/content/treeView.js
+++ b/browser/components/places/content/treeView.js
@@ -718,7 +718,13 @@ PlacesTreeView.prototype = {
     if (PlacesUtils.nodeIsSeparator(aNode) && this.isSorted())
       return;
 
-    let oldRow = this._getRowForNode(aNode, true);
+    // Note that at this point the node has already been moved by the backend,
+    // so we must give hints to _getRowForNode to get the old row position.
+    let oldParentRow = aOldParent == this._rootNode ?
+                         undefined : this._getRowForNode(aOldParent, true);
+    let oldRow = this._getRowForNode(aNode, true, oldParentRow, aOldIndex);
+    if (oldRow < 0)
+      throw Cr.NS_ERROR_UNEXPECTED;
 
     // If this node is a container it could take up more than one row.
     let count = this._countVisibleRowsForNodeAtRow(oldRow);
diff --git a/browser/components/places/tests/chrome/head.js b/browser/components/places/tests/chrome/head.js
index 90d19c9defe..878c28c9291 100644
--- a/browser/components/places/tests/chrome/head.js
+++ b/browser/components/places/tests/chrome/head.js
@@ -53,3 +53,17 @@ function addVisits(aPlaceInfo, aCallback) {
     }
   );
 }
+
+/**
+ * Clears history invoking callback when done.
+ */
+function waitForClearHistory(aCallback) {
+  const TOPIC_EXPIRATION_FINISHED = "places-expiration-finished";
+  Services.obs.addObserver(function observer(aSubject, aTopic, aData) {
+    Services.obs.removeObserver(observer, TOPIC_EXPIRATION_FINISHED);
+    aCallback();
+  }, TOPIC_EXPIRATION_FINISHED, false);
+  Cc["@mozilla.org/browser/nav-history-service;1"]
+    .getService(Ci.nsINavHistoryService)
+    .QueryInterface(Ci.nsIBrowserHistory).removeAllPages();
+}
diff --git a/browser/components/places/tests/chrome/test_bug549192.xul b/browser/components/places/tests/chrome/test_bug549192.xul
index e5916431ccd..f193bf82cd1 100644
--- a/browser/components/places/tests/chrome/test_bug549192.xul
+++ b/browser/components/places/tests/chrome/test_bug549192.xul
@@ -35,9 +35,9 @@
 
   <script type="application/javascript"><![CDATA[
     /**
+     * Bug 874407
+     * Ensures that history views are updated properly after visits.
      * Bug 549192
-     * https://bugzilla.mozilla.org/show_bug.cgi?id=549192
-     *
      * Ensures that history views are updated after deleting entries.
      */
 
@@ -52,64 +52,64 @@
       // Add some visits.
       let vtime = Date.now() * 1000;
       const ttype = PlacesUtils.history.TRANSITION_TYPED;
-
-      addVisits(
+      let places =
         [{ uri: Services.io.newURI("http://example.tld/", null, null),
            visitDate: vtime, transition: ttype },
          { uri: Services.io.newURI("http://example2.tld/", null, null),
            visitDate: vtime++, transition: ttype },
          { uri: Services.io.newURI("http://example3.tld/", null, null),
-           visitDate: vtime++, transition: ttype }],
-         function() {
-           // Make a history query.
-           let query = PlacesUtils.history.getNewQuery();
-           let opts = PlacesUtils.history.getNewQueryOptions();
-           let queryURI = PlacesUtils.history.queriesToQueryString([query], 1, opts);
+           visitDate: vtime++, transition: ttype }];
 
-           // Setup the places tree contents.
-           var tree = document.getElementById("tree");
-           tree.place = queryURI;
+      addVisits(places, function() {
+        // Make a history query.
+        let query = PlacesUtils.history.getNewQuery();
+        let opts = PlacesUtils.history.getNewQueryOptions();
+        opts.sortingMode = opts.SORT_BY_DATE_DESCENDING;
+        let queryURI = PlacesUtils.history.queriesToQueryString([query], 1, opts);
 
-           // loop through the rows and check formatting
-           let treeView = tree.view;
-           for (let i = 0; i < rc; i++) {
-             selection.select(rc);
-             let node = tree.selectedNode;
-             ok(true, "found " + node.title);
-           }
-           let rc = treeView.rowCount;
-           is(rc, 3, "Rows found.");
-           let selection = treeView.selection;
-           for (let i = 0; i < rc; i++) {
-             selection.select(0);
-             let node = tree.selectedNode;
-             tree.controller.remove("Removing page");
-             ok(treeView.treeIndexForNode(node) == Ci.nsINavHistoryResultTreeViewer.INDEX_INVISIBLE,
-               node.uri + " removed.");
-             ok(treeView.rowCount == rc - i - 1, "Rows count decreased");
-           }
+        // Setup the places tree contents.
+        var tree = document.getElementById("tree");
+        tree.place = queryURI;
 
-           // Cleanup.
-           waitForClearHistory(SimpleTest.finish);
-         });
-    }
+        // loop through the rows and check them.
+        let treeView = tree.view;
+        let selection = treeView.selection;
+        let rc = treeView.rowCount;
 
-    /**
-     * Clears history invoking callback when done.
-     */
-    function waitForClearHistory(aCallback) {
-      const TOPIC_EXPIRATION_FINISHED = "places-expiration-finished";
-      let observer = {
-        observe: function(aSubject, aTopic, aData) {
-          Services.obs.removeObserver(this, TOPIC_EXPIRATION_FINISHED);
-          aCallback();
+        for (let i = 0; i < rc; i++) {
+          selection.select(i);
+          let node = tree.selectedNode;
+          is(node.uri, places[rc - i - 1].uri.spec,
+             "Found expected node at position " + i + ".");
         }
-      };
-      Services.obs.addObserver(observer, TOPIC_EXPIRATION_FINISHED, false);
-      let hs = Cc["@mozilla.org/browser/nav-history-service;1"].
-               getService(Ci.nsINavHistoryService);
-      hs.QueryInterface(Ci.nsIBrowserHistory).removeAllPages();
-   }
+
+        is(rc, 3, "Found expected number of rows.");
+
+        // First check live-update of the view when adding visits.
+        places.forEach(place => place.visitDate = ++vtime);
+        addVisits(places, function() {
+          for (let i = 0; i < rc; i++) {
+            selection.select(i);
+            let node = tree.selectedNode;
+            is(node.uri, places[rc - i - 1].uri.spec,
+               "Found expected node at position " + i + ".");
+          }
+
+          // Now remove the pages and verify live-update again.
+          for (let i = 0; i < rc; i++) {
+            selection.select(0);
+            let node = tree.selectedNode;
+            tree.controller.remove("Removing page");
+            ok(treeView.treeIndexForNode(node) == Ci.nsINavHistoryResultTreeViewer.INDEX_INVISIBLE,
+              node.uri + " removed.");
+            ok(treeView.rowCount == rc - i - 1, "Rows count decreased");
+          }
+
+          // Cleanup.
+          waitForClearHistory(SimpleTest.finish);
+        });
+      });
+    }
 
   ]]></script>
 </window>