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)

Assignee

Description

5 years ago
This involves changes to controller.js, browserPlacesViews.js, tree.xml, treeview.js
And very likely some parts of PlacesUIUtils.
Flags: firefox-backlog+
Assignee

Updated

5 years ago
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.
Assignee

Comment 2

5 years ago
(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)
Assignee

Updated

4 years ago
Depends on: 1145063
Assignee

Comment 3

4 years ago
split keyword column removal to bug 1145063.
Assignee

Comment 4

4 years ago
picking up this
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee

Comment 5

4 years ago
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
Assignee

Comment 6

4 years ago
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.
Assignee

Comment 7

4 years ago
filed bug 1161091.

Updated

4 years ago
Iteration: 40.3 - 11 May → 41.1 - May 25
Assignee

Updated

4 years ago
Priority: -- → P1
Assignee

Updated

4 years ago
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]
Assignee

Updated

2 years ago
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]
Assignee

Updated

2 years ago
Priority: P3 → P2
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee

Updated

2 years ago
Summary: Use Bookmarks.jsm in controller and UI views → Use Bookmarks.jsm in controller.js
Assignee

Comment 10

2 years ago
I'm splitting out a separate bug for editBookmarkOverlay.js
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Assignee

Updated

2 years ago
Attachment #8604228 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8604229 - Attachment is obsolete: true
Priority: P1 → P3
Assignee

Comment 11

2 years ago
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
Assignee

Updated

2 years ago
Blocks: 1382991
Assignee

Updated

2 years ago
Blocks: 1382992
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8888748 - Flags: review?(standard8)

Comment 13

2 years ago
mozreview-review
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+

Comment 14

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/e72d0fc07a0b
Use Bookmarks.jsm in controller.js. r=standard8

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e72d0fc07a0b
Status: ASSIGNED → RESOLVED
Last Resolved: 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.