Closed Bug 1388827 Opened 2 years ago Closed 2 years ago

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

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set

Tracking

(seamonkey2.53 fixed, seamonkey2.54 fixed)

RESOLVED FIXED
seamonkey2.54
Tracking Status
seamonkey2.53 --- fixed
seamonkey2.54 --- fixed

People

(Reporter: frg, Assigned: frg)

References

(Blocks 1 open bug)

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)

+++ 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: 2 years ago
Resolution: --- → 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.