Closed Bug 549192 Opened 14 years ago Closed 14 years ago

History view not updated after deleting entry

Categories

(Firefox :: Bookmarks & History, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: elmar.ludwig, Assigned: asaf)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a2pre) Gecko/20100227 Minefield/3.7a2pre ID:20100227031019

After deleting a history entry, the view is not updated and the deleted entry is still visible. This affects the history sidebar (when set to "View By Last Visited") and the Places Library window. Closing and reopening the view shows the correct list of entries. Tested with new profile, no extensions.

Steps to reproduce:

1. go to http://www.mozilla.org/
2. go to http://planet.mozilla.org/
3. go to https://wiki.mozilla.org/
4. open places library window, select History > Today
5. delete entry for planet.mozilla.org (via context menu or delete key)

Expected behaviour:
history entry disappears from list

Actual behaviour:
view is not updated, entry remains visible

Regression range:

firefox-3.7a2pre-2010-02-25-03: works
firefox-3.7a2pre-2010-02-26-03: fails

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c390c6f279ee&tochange=475768f37b1a

mano: Could this be a regression from bug 520659?
Depends on: 520659
Blocks: 520659
No longer depends on: 520659
Vista too, I believe, hence platform -> all.
OS: Linux → All
(In reply to comment #0)
> This affects the history sidebar (when set to "View By Last
> Visited") ...

Error: uncaught exception: [Exception... "Component returned failure code:
0x80070057 (NS_ERROR_ILLEGAL_VALUE)
[nsINavHistoryContainerResultNode.getChildIndex]"  nsresult: "0x80070057
(NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
chrome://browser/content/places/treeView.js :: PTV__getRowForNode :: line 187" 
data: no]
actually we have landed a couple fixes to the treeview in bug 555293, among them also a typo.
So please, always report your UA.
"a typo fix", clearly.
(In reply to comment #2)
> *** Bug 555974 has been marked as a duplicate of this bug. ***

In fact that bug is moving entry by revisiting a location but probably the same cause.

(In reply to comment #4)
> actually we have landed a couple fixes to the treeview in bug 555293, among
> them also a typo.
> So please, always report your UA.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a4pre) Gecko/20100330 Minefield/3.7a4pre ID:20100330035832
Patch coming, likely without a test for now...
Assignee: nobody → mano
Status: NEW → ASSIGNED
Blocks: 560198
Is this not more likely a regression of bug 492797?
Attached patch patch (obsolete) — Splinter Review
Attachment #448345 - Flags: review?(mak77)
Comment on attachment 448345 [details] [diff] [review]
patch

>diff -r e73cbd31c93e browser/components/places/content/treeView.js
>--- a/browser/components/places/content/treeView.js	Sun May 30 12:50:39 2010 +0300
>+++ b/browser/components/places/content/treeView.js	Mon May 31 11:22:57 2010 +0300
>@@ -146,25 +146,25 @@
>    *        A result node.  Do not pass an obsolete node, or any
>    *        node which isn't supposed to be in the tree (e.g. separators in
>    *        sorted trees).
>    * @param [optional] aForceBuild
>    *        @see _isPlainContainer.
>    *        If true, the row will be computed even if the node still isn't set
>    *        in our rows array.
>    * @param [optional] aParentRow
>-   *        The row of aNode's parent.
>-   *        DO NOT compute this yourself for the purpose of calling this
>-   *        function.  However, do pass it if you have it handy.
>-   *        Ignored for the root node.
>+   *        The row of aNode's parent. Ignored for the root node.
>    * @param [optional] aNodeIndex
>    *        The index of aNode in its parent.  Only used if aParentRow is
>    *        set too.
>    *
>    * @throws if aNode is invisible.
>+   * @note If aNode was removed from a plain-conainer before this method
>+   *       has been called, aParentRow and aNodeIndex must be passed (pass
>+   *       |undefined| for the root node).

This seems to weak the method validity, this method is getRowForNode, not calculateRowForNode, if node has been removed I'd expect that that it can't find and return a row value for it.
I don't want to limit usefulness of the method though, so you should probably change the note as:
@note If aParentRow and aNodeIndex are passed and parent is a plain container, this method will just return a calculated row value, without making assumptions on existence of the node at that position.
Otherwise you should make this stronger so that it throws if the node at the calculated position is not the requested one, and special case caller points to do calculation by themselves.

>-    let oldRow = this._getRowForNode(aNode, true);
>+    let parentRow = aParentNode == this._rootNode ?
>+                    undefined :this._getRowForNode(aParentNode, true);

space after the colon, and indent once more


>diff -r e73cbd31c93e browser/components/places/tests/chrome/test_bug549192.xul
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/places/tests/chrome/test_bug549192.xul	Mon May 31 11:22:57 2010 +0300
>@@ -0,0 +1,134 @@
>+<?xml version="1.0"?>
>+
>+<!-- ***** BEGIN LICENSE BLOCK *****
>+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1

we are starting using PD license for tests, so you can replace all the boilerplate here with


/*
 * Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/licenses/publicdomain/
 */


>+   - Contributor(s):
>+   -   Asaf Romano <mak77@bonardo.net> (Original Author)

the mail would have been wrong, moreover :)


>+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        title="435322: Places tree view's formatting"
>+        onload="runTest();">

title is from another bug


>+  <script type="application/javascript">
>+  <![CDATA[

nit: inline CDATA with <script> tag


>+    function runTest() {
>+      const Cc = Components.classes;
>+      const Ci = Components.interfaces;
>+
>+      var iosvc = Cc["@mozilla.org/network/io-service;1"].
>+                  getService(Ci.nsIIOService);
>+      function uri(spec) {
>+        return iosvc.newURI(spec, null, null);
>+      }

You could 
Components.utils.import("resource://gre/modules/Services.jsm");
and use Services.io.newURI(aURI, null, null);


>+      // Cleanup.
>+      PlacesUtils.bhistory.removeAllPages();
>+
>+      // Add some visits.
>+      let vtime = Date.now() * 1000;
>+      const ttype = PlacesUtils.history.TRANSITION_TYPED;

both seem const


>+      PlacesUtils.history.addVisit(uri("http://example.tld/"), vtime,
>+                                   null, ttype, false, 0);
>+      PlacesUtils.history.addVisit(uri("http://example2.tld/"), vtime,
>+                                   null, ttype, false, 0);
>+      PlacesUtils.history.addVisit(uri("http://exmample3.tld/"), vtime,
>+                                   null, ttype, false, 0);

vtime++ at each addVisit please


>+      let selection = treeView.selection;
>+      for (let i=0; i < rc; i++) {

nit: spaces around i = 0


>+        selection.select(0);
>+        let node = tree.selectedNode;
>+        tree.controller.remove("Remvoing page");

typo: removing


>+  ]]>
>+  </script>

nit: inline ]]> with </script> tag

the test is mostly hitting the root node, would be cool to also test a plain container that is not the root node, but I assume this test fails before the fix and is fine to cover the regression.

r=me with the above fixed
Attachment #448345 - Flags: review?(mak77) → review+
I can't make vtime a constant and also ++ it...
Attached patch for checkinSplinter Review
Attachment #448345 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/2ed1eee89a4c
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 566782
No longer blocks: 566782
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: