Marco Bonardo [:mak] Assignee
	Description • 10 years ago

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

	Asaf Romano (gone)
	Comment 1 • 10 years ago

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.

	Marco Bonardo [:mak] Assignee
	Comment 2 • 10 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)

	Marco Bonardo [:mak] Assignee
	Comment 3 • 9 years ago

split keyword column removal to bug 1145063.

	Marco Bonardo [:mak] Assignee
	Comment 4 • 9 years ago

picking up this

	Marco Bonardo [:mak] Assignee
	Comment 5 • 9 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)

	Marco Bonardo [:mak] Assignee
	Comment 6 • 9 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.

	Marco Bonardo [:mak] Assignee
	Comment 7 • 9 years ago

filed bug 1161091.

	Marco Bonardo [:mak] Assignee
	Comment 10 • 7 years ago

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

	Marco Bonardo [:mak] Assignee
	Comment 11 • 7 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.

	Mark Banner (:standard8)
	Comment 13 • 7 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.

	Pulsebot
	Comment 14 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 7 years ago
bugherder

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