Bug 1388827 - Part 2. Port Bug 874407 to SeaMonkey. New visits are inserted incorrectly in the Library and the sidebar treeviews. r=IanN
authorFrank-Rainer Grahl <frgrahl@gmx.net>
Sun, 10 Sep 2017 21:08:41 +0200
changeset 29680 8c5e7391525ebcd4970ff8a6f602605a4272833f
parent 29679 e91a8dac406d38b4b3f8846e39c396e9fa0cb126
child 29681 c2dc67cad239ec8275c944a3ba4b9c840abb970f
push id378
push userclokep@gmail.com
push dateMon, 13 Nov 2017 18:45:35 +0000
reviewersIanN
bugs1388827, 874407
Bug 1388827 - Part 2. Port Bug 874407 to SeaMonkey. New visits are inserted incorrectly in the Library and the sidebar treeviews. r=IanN
suite/common/history/treeView.js
suite/common/places/tests/chrome/test_bug549192.xul
suite/common/places/treeView.js
--- a/suite/common/history/treeView.js
+++ b/suite/common/history/treeView.js
@@ -563,17 +563,23 @@ PlacesTreeView.prototype = {
   },
 
   nodeMoved:
   function PTV_nodeMoved(aNode, aOldParent, aOldIndex, aNewParent, aNewIndex) {
     NS_ASSERT(this._result, "Got a notification but have no result!");
     if (!this._tree || !this._result)
       return;
 
-    var 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.
     var count = this._countVisibleRowsForNodeAtRow(oldRow);
 
     // Persist selection state.
     var nodesToReselect =
       this._getSelectedNodesInRange(oldRow, oldRow + count);
     if (nodesToReselect.length > 0)
--- a/suite/common/places/tests/chrome/test_bug549192.xul
+++ b/suite/common/places/tests/chrome/test_bug549192.xul
@@ -32,91 +32,90 @@
     <treecols>
       <treecol label="Title" id="title" anonid="title" primary="true" ordinal="1" flex="1"/>
     </treecols>
     <treechildren flex="1"/>
   </tree>
 
   <script type="application/javascript"><![CDATA[
     /**
+     * Bug 1388827 / 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.
      */
       Components.utils.import("resource://gre/modules/Services.jsm");
       const Cc = Components.classes;
       const Ci = Components.interfaces;
 
     SimpleTest.waitForExplicitFinish();
 
     function runTest() {
       // The mochitest page is added to history.
       waitForClearHistory(continue_test);
     }
 
     function continue_test() {
-
       // 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 },
+           visitDate: ++vtime, transition: ttype },
          { uri: Services.io.newURI("http://example2.tld/", null, null),
-           visitDate: vtime++, transition: ttype },
+           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 }];
+
+      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;
 
-           // Setup the places tree contents.
-           var tree = document.getElementById("tree");
-           tree.place = queryURI;
+        // loop through the rows and check them.
+        let treeView = tree.view;
+        let selection = treeView.selection;
+        let rc = treeView.rowCount;
 
-          // loop through the rows and check formatting
-          let treeView = tree.view;
+        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(rc);
+            selection.select(i);
             let node = tree.selectedNode;
-            ok(true, "found " + node.title);
+            is(node.uri, places[rc - i - 1].uri.spec,
+               "Found expected node at position " + i + ".");
           }
-          let rc = treeView.rowCount;
-          is(rc, 3, "Rows found.");
-          let selection = treeView.selection;
+
+          // 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);
-        }
-      );
+        });
+      });
     }
 
-    /**
-     * 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>
 </window>
--- a/suite/common/places/treeView.js
+++ b/suite/common/places/treeView.js
@@ -697,17 +697,23 @@ PlacesTreeView.prototype = {
     NS_ASSERT(this._result, "Got a notification but have no result!");
     if (!this._tree || !this._result)
       return;
 
     // Bail out for hidden separators.
     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);
 
     // Persist selection state.
     let nodesToReselect =
       this._getSelectedNodesInRange(oldRow, oldRow + count);
     if (nodesToReselect.length > 0)