Bug 874407 - new visits are inserted incorrectly in the Library and the sidebar treeviews.

r=Mano

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);

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();
+}

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,34 +52,50 @@
       // Add some visits.
       let vtime = Date.now() * 1000;
       const ttype = PlacesUtils.history.TRANSITION_TYPED;
-
+      let places =
-      addVisits(
         [{ 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 }],
+           visitDate: vtime++, transition: ttype }];
-         function() {
+
+      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);
 
         // Setup the places tree contents.
         var tree = document.getElementById("tree");
         tree.place = queryURI;
 
-           // loop through the rows and check formatting
+        // loop through the rows and check them.
         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;
+        let rc = treeView.rowCount;
+
+        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 + ".");
+        }
+
+        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;
@@ -92,23 +108,7 @@
           // Cleanup.
           waitForClearHistory(SimpleTest.finish);
         });
-    }
+      });
-
-    /**
-     * 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();
-        }
-      };
-      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();
     }
 
   ]]></script>