Closed Bug 517719 Opened 10 years ago Closed 2 years ago

Drop or move nsINavHistoryResultTreeViewer from toolkit

Categories

(Firefox :: Bookmarks & History, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: mano, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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
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 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+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1e076595d32
Remove unnecessary nsINavHistoryResultTreeViewer interface. r=mak
https://hg.mozilla.org/mozilla-central/rev/b1e076595d32
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Priority: -- → P2
Whiteboard: [fxsearch]
Depends on: 1444599
You need to log in before you can comment on or make changes to this bug.