1094818 - Use Bookmarks.jsm in controller.js

Status: RESOLVED FIXED

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

Description

[Marco Bonardo [:mak]]

This involves changes to controller.js, browserPlacesViews.js, tree.xml, treeview.js
And very likely some parts of PlacesUIUtils.

Comment 1

[Asaf Romano (gone)]

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.

Comment 2

[Marco Bonardo [:mak]]

(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)

Comment 3

[Marco Bonardo [:mak]]

split keyword column removal to bug 1145063.

Comment 4

[Marco Bonardo [:mak]]

picking up this

Comment 5

[Marco Bonardo [:mak]]

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)

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Marco Bonardo [:mak]]

filed bug 1161091.

Comment 8

[Marco Bonardo [:mak]]

Attachment: part 1 - Get rid of GetFoldeIdForItem - WIP

Comment 9

[Marco Bonardo [:mak]]

Attachment: part 2 - Get rid of GetItemTitle - WIP

Comment 10

[Marco Bonardo [:mak]]

I'm splitting out a separate bug for editBookmarkOverlay.js

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Marco Bonardo [:mak]]

Attachment: Bug 1094818 - Use Bookmarks.jsm in controller.js.

Review commit: https://reviewboard.mozilla.org/r/159784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159784/

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8888748 [details]
Bug 1094818 - Use Bookmarks.jsm in controller.js.

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

Looks good.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e72d0fc07a0b
Use Bookmarks.jsm in controller.js. r=standard8

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e72d0fc07a0b