Use Bookmarks.jsm in controller.js

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks 3 bugs, {perf})

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-performance] [qf:p2][fxsearch], )

Attachments

(1 attachment, 2 obsolete attachments)

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
Status: NEW → ASSIGNED
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]
Status: ASSIGNED → NEW
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
Status: NEW → ASSIGNED
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
Status: ASSIGNED → NEW
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
Status: NEW → ASSIGNED
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.

https://reviewboard.mozilla.org/r/159784/#review168768

Looks good.
Attachment #8888748 - Flags: review?(standard8) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/e72d0fc07a0b
Use Bookmarks.jsm in controller.js. r=standard8
https://hg.mozilla.org/mozilla-central/rev/e72d0fc07a0b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.1 - Aug 15
You need to log in before you can comment on or make changes to this bug.