Closed Bug 1388827 Opened 7 years ago Closed 7 years ago

Port bug 1107364 to SeaMonkey: move findNodeByDetails to treeView.js and make it use an hash

Categories

(SeaMonkey :: Bookmarks & History, defect)

Product:
SeaMonkey
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.53 fixed, seamonkey2.54 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.54
Tracking Flags:
Tracking Status
seamonkey2.53 --- fixed
seamonkey2.54 --- fixed

People

(Reporter: frg, Assigned: frg)

References

Details

User Story

What we need to to here is removing FindNodeByDetails from nsNavHistoryContainerResultNode and  nsINavHistoryResultNode, and reimplement it in browser/components/places/content/treeView.js.

In the js version, instead of linearly walking all the containers recursively, we should instead create a Map between node details (the key could be a combination of itemId, url and time) and tree row index. The challenge here is to keep the Map up-to-date with the tree. the points where we already update the _rows array *could* be a good starting point for that.

Attachments

(4 files, 2 obsolete files)

1388827-old-times-part1.patch
4.55 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
1388827-norow-part3.patch
2.67 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
1388827-treeview-part4.patch
25.75 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
1388827-hint-part2-V2.patch
7.66 KB, patch
frg
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1107364 +++ Timestamp: 8/9/2017, 6:10:45 PM Error: TypeError: aUpdatedContainer.findNodeByDetails is not a function Source File: chrome://communicator/content/history/treeView.js Line: 363
Depends on: 1393289
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attached patch 1388827-treeview-WIP.patch (obsolete) — — Splinter Review
More or less a 1:1 port but we have two treeview.js files. I ran into problems while porting and changed some other statements too. Works but need to clean it up first.
Prerequisite part 1 Port Bug 1368754.
Attachment #8900523 - Attachment is obsolete: true
Attachment #8900961 - Flags: review?(iann_bugzilla)
Attached patch 1388827-hint-part2.patch (obsolete) — — Splinter Review
Prerequisite part 2. Port Bug 874407
Attachment #8900962 - Flags: review?(iann_bugzilla)
Prerequisite part 3. Port Bug 580814. I can reproduce the error in both the history and the bookmarks panel.
Attachment #8900963 - Flags: review?(iann_bugzilla)
Part 4. The actual fix. Port of bug 1107364.
Attachment #8900965 - Flags: review?(iann_bugzilla)
Comment on attachment 8900961 [details] [diff] [review] 1388827-old-times-part1.patch LGTM r=me
Attachment #8900961 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8900962 [details] [diff] [review] 1388827-hint-part2.patch LGTM r=me
Attachment #8900962 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8900962 [details] [diff] [review] 1388827-hint-part2.patch You should also port the test fixes from Bug 874407 - https://hg.mozilla.org/mozilla-central/rev/d8cfe163b77d
Flags: needinfo?(frgrahl)
Comment on attachment 8900963 [details] [diff] [review] 1388827-norow-part3.patch LGTM r=me
Attachment #8900963 - Flags: review?(iann_bugzilla) → review+
Patch updated with test fixes from Bug 874407. Straight 1:1 so r+ from IanN carried forward.
Attachment #8900962 - Attachment is obsolete: true
Flags: needinfo?(frgrahl)
Attachment #8901148 - Flags: review+
Comment on attachment 8900965 [details] [diff] [review] 1388827-treeview-part4.patch LGTM r=me
Attachment #8900965 - Flags: review?(iann_bugzilla) → review+
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/e91a8dac406d Part 1. Port Bug 1368754 to SeaMonkey. Change HistoryDetailsChanged/NodeURIChanged to be called with the old time/access count/uri rather than the new ones. r=IanN https://hg.mozilla.org/comm-central/rev/8c5e7391525e Part 2. Port Bug 874407 to SeaMonkey. New visits are inserted incorrectly in the Library and the sidebar treeviews. r=IanN https://hg.mozilla.org/comm-central/rev/c2dc67cad239 Port Bug 580814 to SeaMonkey. [Avoid accessing rows that don't exist in treeviews for Places]. r=IanN https://hg.mozilla.org/comm-central/rev/0098371b5fce Part 4. Port bug 1107364 to SeaMonkey: move findNodeByDetails to treeView.js and make it use an hash. r=IanN
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
status-seamonkey2.54: affected → fixed
Target Milestone: --- → seamonkey2.54
Comment on attachment 8900961 [details] [diff] [review] 1388827-old-times-part1.patch [Approval Request Comment] Regression caused by (bug #): -- User impact if declined: error not fixed Testing completed (on m-c, etc.): c-b Risk to taking this patch (and alternatives if risky): none broken already. String changes made by this patch: none
Attachment #8900961 - Flags: approval-comm-beta?
Comment on attachment 8900963 [details] [diff] [review] 1388827-norow-part3.patch [Approval Request Comment] Regression caused by (bug #): -- User impact if declined: error not fixed Testing completed (on m-c, etc.): c-b Risk to taking this patch (and alternatives if risky): none broken already. String changes made by this patch: none
Attachment #8900963 - Flags: approval-comm-beta?
Comment on attachment 8900965 [details] [diff] [review] 1388827-treeview-part4.patch [Approval Request Comment] Regression caused by (bug #): -- User impact if declined: error not fixed Testing completed (on m-c, etc.): c-b Risk to taking this patch (and alternatives if risky): none broken already. String changes made by this patch: none
Attachment #8900965 - Flags: approval-comm-beta?
Comment on attachment 8901148 [details] [diff] [review] 1388827-hint-part2-V2.patch [Approval Request Comment] Regression caused by (bug #): -- User impact if declined: error not fixed Testing completed (on m-c, etc.): c-b Risk to taking this patch (and alternatives if risky): none broken already. String changes made by this patch: none
Attachment #8901148 - Flags: approval-comm-beta?
Comment on attachment 8900961 [details] [diff] [review] 1388827-old-times-part1.patch a=me
Attachment #8900961 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8900963 [details] [diff] [review] 1388827-norow-part3.patch a=me
Attachment #8900963 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8900965 [details] [diff] [review] 1388827-treeview-part4.patch a=me
Attachment #8900965 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8901148 [details] [diff] [review] 1388827-hint-part2-V2.patch a=me
Attachment #8901148 - Flags: approval-comm-beta? → approval-comm-beta+
No longer blocks: 2.56BulkMalfunctions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: