Drop or move nsINavHistoryResultTreeViewer from toolkit

RESOLVED FIXED in Firefox 60

Status

()

P2
normal
RESOLVED FIXED
9 years ago
8 months ago

People

(Reporter: mano, Assigned: standard8)

Tracking

unspecified
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

nsINavHistoryResultTreeViewer does not belong in toolkit. It should have been moved to tabbrowser back when we "moved" the treeview implementation.

Actually, considering that the current implementaion is an internal JS object of browser/, we could use standard js for this couple of helpers.
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Comment hidden (mozreview-request)
(Assignee)

Comment 3

9 months ago
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=efde191433099958d5c93c161836640b11c982d2
Assignee: nobody → standard8
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All

Comment 4

9 months ago
mozreview-review
Comment on attachment 8944038 [details]
Bug 517719 - Remove unnecessary nsINavHistoryResultTreeViewer interface.

https://reviewboard.mozilla.org/r/214366/#review220214

::: browser/components/places/content/treeView.js:1212
(Diff revision 1)
>  
>      return this._getNodeForRow(aIndex);
>    },
>  
> -  treeIndexForNode: function PTV_treeNodeForIndex(aNode) {
> +  /**
> +   * Reverse of nodeForFlatIndex, returns the row index for a given result node.

I suppose this should be "nodeForTreeIndex", not "nodeForFlatIndex".

::: browser/components/places/content/treeView.js:1218
(Diff revision 1)
> +   *
> +   * Note: This sounds sort of obvious, but it got me: aNode must be a node
> +   *       retrieved from the same result that this viewer is for. If you
> +   *       execute another query and get a node from a _different_ result, this
> +   *       function will always return the index of that node in the tree that
> +   *       is attached to that result.

I think this comment is sort of outdated, and thus confusin.
You can likely just add to the basic method description that the node to retrieve should be part of the result for this tree.

the reason for the original comment was likely that the code was in cpp and was looking for the node in its own parent result.
Attachment #8944038 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 6

9 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1e076595d32
Remove unnecessary nsINavHistoryResultTreeViewer interface. r=mak

Comment 7

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1e076595d32
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

Updated

8 months ago
Priority: -- → P2
Whiteboard: [fxsearch]

Updated

8 months ago
Depends on: 1444599
You need to log in before you can comment on or make changes to this bug.