Closed
Bug 549192
Opened 14 years ago
Closed 14 years ago
History view not updated after deleting entry
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: elmar.ludwig, Assigned: asaf)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
6.63 KB,
patch
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•14 years ago
|
Comment 3•14 years ago
|
||
(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]
Comment 4•14 years ago
|
||
actually we have landed a couple fixes to the treeview in bug 555293, among them also a typo. So please, always report your UA.
Comment 5•14 years ago
|
||
"a typo fix", clearly.
Comment 6•14 years ago
|
||
(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
Assignee | ||
Comment 7•14 years ago
|
||
Patch coming, likely without a test for now...
Assignee: nobody → mano
Status: NEW → ASSIGNED
Is this not more likely a regression of bug 492797?
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #448345 -
Flags: review?(mak77)
Comment 13•14 years ago
|
||
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+
Assignee | ||
Comment 14•14 years ago
|
||
I can't make vtime a constant and also ++ it...
Assignee | ||
Comment 15•14 years ago
|
||
Attachment #448345 -
Attachment is obsolete: true
Assignee | ||
Comment 16•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2ed1eee89a4c
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•