The patch in bug 951651 takes care of this for the most part (by using nodes, actually). Anything left should be relatively trivial.

We should probably wait for bug 951651 before doing anything here.

Mano, please let me know if you still plan to work on this or I should pick it up. Personally I'd prefer if you'd concentrate on the dependencies of bug 1071513, but maybe you have a wip patch already.

final answer on Sunday.

This is an updated query with most of the stuff that should be changed in browser/ http://searchfox.org/mozilla-central/search?q=PlacesUtils%5C.(bookmarks%5C.(get%7Cset%7CinsertB%7CremoveI%7Ccreate%7CremoveF%7Cmove%7CinsertS%7Cis%7Cchange%7Crun)%7CgetMostRecent%7CgetBookmarksForURI)&case=false&regexp=true&path=%5Ebrowser%2F I think this bug should concentrate only on browser-places.js. I may further split out of Bug 1094818 a separate bug for editBookmarkOverlay, since I have patches for part of that already in that bug. And then Bug 1094818 would just handle controller.js. If code is part of old transactions, it should not be touched. After async PlacesTransactions are enabled and the legacy code removed, we can loop back to check what's left (likely tests and minor calls).

Comment on attachment 8887905 [details] Bug 1094812 - Use Bookmarks.jsm in browser-places.js. https://reviewboard.mozilla.org/r/158816/#review164210 ::: browser/base/content/browser-places.js:102 (Diff revision 1) > } > PlacesTransactions.undo().catch(Cu.reportError); > - break; > + return; > } > + (async () => { > + let bookmark = await PlacesUtils.bookmarks.fetch(this._bookmarkForRemoval); the problem I see is that there's too many requests to the db for info we could already know. For example, later we fetch bookmarks for the url, just then to convert one of those bookmarks to a url here, and then RemoveBookmarksForUrls will likely again fetch bookmarks from the url. I feel like we can do better. StarUI has an uri property that looks dead (likely due to refactorings). It sounds like we could revive it, store into it gBrowser.currentURI when we currently store _itemId and clear it where we currently clear _itemId (on popup hidden). in removeBookmarkButtonCommand, just store a bool of whether we should remove the bookmark (this._removeBookmarksOnPopupHidden), that's the only information needed. That should allow here to just pass this.uri to getBookmarksForURI or RemovebookmarksForUrls. But there's more. I'm not sure having PlacesTransactions.RemoveBookmarksForUrls is a win VS just using PlacesTransactions.Remove... Indeed here we already have all the guids where we fetch the number of bookmarks, then we could store ._itemGuids instead of the uri and directly use PlacesTransactions.Remove. It would avoid another DB lookup to fetch the guids from the url that RemoveBookmarksForUrls is doing internally. Then we could remove RemoveBookmarksForUrls, since this looks like the only reason it exists for. It basically seems to exist only to make this do more IO. What do you think?

Comment on attachment 8887905 [details] Bug 1094812 - Use Bookmarks.jsm in browser-places.js. https://reviewboard.mozilla.org/r/158816/#review164210 > the problem I see is that there's too many requests to the db for info we could already know. > > For example, later we fetch bookmarks for the url, just then to convert one of those bookmarks to a url here, and then RemoveBookmarksForUrls will likely again fetch bookmarks from the url. > > I feel like we can do better. > StarUI has an uri property that looks dead (likely due to refactorings). It sounds like we could revive it, store into it gBrowser.currentURI when we currently store _itemId and clear it where we currently clear _itemId (on popup hidden). > in removeBookmarkButtonCommand, just store a bool of whether we should remove the bookmark (this._removeBookmarksOnPopupHidden), that's the only information needed. > That should allow here to just pass this.uri to getBookmarksForURI or RemovebookmarksForUrls. > > But there's more. I'm not sure having PlacesTransactions.RemoveBookmarksForUrls is a win VS just using PlacesTransactions.Remove... Indeed here we already have all the guids where we fetch the number of bookmarks, then we could store ._itemGuids instead of the uri and directly use PlacesTransactions.Remove. It would avoid another DB lookup to fetch the guids from the url that RemoveBookmarksForUrls is doing internally. Then we could remove RemoveBookmarksForUrls, since this looks like the only reason it exists for. It basically seems to exist only to make this do more IO. > > What do you think? I think your suggestions are a good way of improving performance even more, so I've added them to the patch.

Comment on attachment 8887905 [details] Bug 1094812 - Use Bookmarks.jsm in browser-places.js. https://reviewboard.mozilla.org/r/158816/#review168748 r=me with the following addressed: ::: browser/base/content/browser-places.js:132 (Diff revision 2) > } > // Remove all bookmarks for the bookmark's url, this also removes > // the tags for the url. > if (!PlacesUIUtils.useAsyncTransactions) { > - let itemIds = PlacesUtils.getBookmarksForURI(this._uriForRemoval); > + let itemIds = PlacesUtils.getBookmarksForURI(Services.io.newURI(urlForRemoval)); > for (let itemId of itemIds) { you could use promiseItemId and convert the guids, then you would not need _itemUri at all. It should be fast since those guids are likely already in the cache. ::: browser/base/content/browser-places.js:315 (Diff revision 2) > + > + await PlacesUtils.bookmarks.fetch({url: this._itemUri}, > + bookmark => this._itemGuids.push(bookmark.guid)); > let forms = gNavigatorBundle.getString("editBookmark.removeBookmarks.label"); > - let label = PluralForm.get(bookmarks.length, forms).replace("#1", bookmarks.length); > + let bookmarksCount = this._itemGuids.length; > + let label = PluralForm.get(bookmarksCount, forms).replace("#1", bookmarksCount); nit: indentation ....get() .replace()

Comment on attachment 8892056 [details] Bug 1094812 - Remove now unused PlacesTransactions.RemoveBookmarksForUrls. https://reviewboard.mozilla.org/r/163066/#review168754

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7c26b5a9e668 Use Bookmarks.jsm in browser-places.js. r=mak https://hg.mozilla.org/integration/autoland/rev/7865bd32d235 Remove now unused PlacesTransactions.RemoveBookmarksForUrls. r=mak

https://hg.mozilla.org/mozilla-central/rev/7c26b5a9e668 https://hg.mozilla.org/mozilla-central/rev/7865bd32d235