Closed Bug 1094818 Opened 9 years ago Closed 7 years ago

Use Bookmarks.jsm in controller.js


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




Firefox 57
57.1 - Aug 15
Performance Impact medium
Tracking Status
firefox57 --- fixed


(Reporter: mak, Assigned: mak)


(Blocks 2 open bugs, )


(Keywords: perf, Whiteboard: [reserve-photon-performance] [fxsearch])


(1 file, 2 obsolete files)

This involves changes to controller.js, browserPlacesViews.js, tree.xml, treeview.js
And very likely some parts of PlacesUIUtils.
Flags: firefox-backlog+
Points: --- → 8
Flags: qe-verify+
Nothing to fix in browserPlacesViews.js and tree.xml afacit.

In treeView.js, the only problem is getKeywordForBookmark (because there's no node.keyword). Fixing this is possible, but somewhat annoying (getCellText is synchronous, so we will have to return "", get the keyword and cache it, invalidate the row and return the cached value from getCellText). Alternatively, we could introduce node.keyword, or just do away with the keyword column in the Library (the usecase is very unclear to me).

Most of the work to be done is in PlacesUIUtils.
(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #1)
> just do
> away with the keyword column in the Library (the usecase is very unclear to
> me).

I'd be fine with getting away with the keyword column. very few bookmarks have keywords, and then we'd better serve the "find all keywords" use case with a Keywords left pane root (I'm not suggesting we do that, but we could ideally)
Depends on: 1145063
split keyword column removal to bug 1145063.
picking up this
Assignee: nobody → mak77
there is a call to getKeywordForBookmark and various bookmarks calls in editBookmarkOverlay.js
various bookmarks calls in controller.js
and, as said, various bookmarks calls in PlacesUIUtils.jsm

Some of these will be very tricky since bound to synchronous behaviors (drag&drop or building the Library left pane)
Iteration: --- → 40.3 - 11 May
It's very unlikely we can fix PlacesUIUTils here, a lot of code depends on the synchronous behavior of PlacesUIUtils.leftPaneFolderId and friends (PUIU.leftPaneQueries, PUIU.allBookmarksFolderId, PUIU.getLeftPaneQueryNameFromId, PUIU.isContentsReadOnly)
Those require a so deep refactoring that I need to split them to their own bug.
That won't block async bookmarks milestone 1 though, it would be too expensive for now.
filed bug 1161091.
Iteration: 40.3 - 11 May → 41.1 - May 25
Priority: -- → P1
Keywords: perf
Whiteboard: [qf]
Iteration: 41.1 - May 25 → ---
Points: 8 → ---
Flags: qe-verify-
Flags: qe-verify+
Flags: firefox-backlog+
Whiteboard: [qf] → [photon-performance] [qf]
Whiteboard: [photon-performance] [qf] → [photon-performance] [qf:p1]
Whiteboard: [photon-performance] [qf:p1] → [photon-performance] [qf:p1][fxsearch]
Priority: P1 → P2
Priority: P2 → P3
Whiteboard: [photon-performance] [qf:p1][fxsearch] → [reserve-photon-performance] [qf:p1][fxsearch]
Whiteboard: [reserve-photon-performance] [qf:p1][fxsearch] → [reserve-photon-performance] [qf:p2][fxsearch]
Priority: P3 → P2
Priority: P2 → P1
Summary: Use Bookmarks.jsm in controller and UI views → Use Bookmarks.jsm in controller.js
I'm splitting out a separate bug for editBookmarkOverlay.js
Assignee: mak77 → nobody
Attachment #8604228 - Attachment is obsolete: true
Attachment #8604229 - Attachment is obsolete: true
Priority: P1 → P3
Not everything is feasible here, canDrop() cannot be easily converted, it needs a way to synchronously fetch the whole hierarchy when dragging a folder. Maybe we could fetch that hiearchy at the time we populate the dataTransfer/clipboard, but that's still a synchronous operation (at least for D&D). There's work ongoing to convert D&D to async, that may help.
In the meanwhile, we'll have to keep getFolderIdForItem around and file a bug apart to fix in the future.
Assignee: nobody → mak77
Priority: P3 → P1
Blocks: 1382991
Blocks: 1382992
Attachment #8888748 - Flags: review?(standard8)
Comment on attachment 8888748 [details]
Bug 1094818 - Use Bookmarks.jsm in controller.js.

Looks good.
Attachment #8888748 - Flags: review?(standard8) → review+
Pushed by
Use Bookmarks.jsm in controller.js. r=standard8
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.1 - Aug 15
Performance Impact: --- → P2
Whiteboard: [reserve-photon-performance] [qf:p2][fxsearch] → [reserve-photon-performance] [fxsearch]
You need to log in before you can comment on or make changes to this bug.