1094812 - Use Bookmarks.jsm in browser-places.js

Status: RESOLVED FIXED

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

Description

[Marco Bonardo [:mak]]

We should refactor the star ui (both bookmarked status and the edit pane) to use the new Bookmarks.jsm API
This involves changes to browser-places.js, editBookmarkOverlay.js

Comment 1

[Asaf Romano (gone)]

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

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Asaf Romano (gone)]

final answer on Sunday.

Comment 6

[Marco Bonardo [:mak]]

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 7

[Mark Banner (:standard8)]

Attachment: Bug 1094812 - Use Bookmarks.jsm in browser-places.js.

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

Comment 8

[Marco Bonardo [:mak]]

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 9

[Mark Banner (:standard8)]

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 10

[Mark Banner (:standard8)]

Attachment: Bug 1094812 - Remove now unused PlacesTransactions.RemoveBookmarksForUrls.

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

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8887905 [details]
Bug 1094812 - Use Bookmarks.jsm in browser-places.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158816/diff/1-2/

Comment 12

[Marco Bonardo [:mak]]

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 13

[Marco Bonardo [:mak]]

Comment on attachment 8892056 [details]
Bug 1094812 - Remove now unused PlacesTransactions.RemoveBookmarksForUrls.

https://reviewboard.mozilla.org/r/163066/#review168754

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8887905 [details]
Bug 1094812 - Use Bookmarks.jsm in browser-places.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158816/diff/2-3/

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8892056 [details]
Bug 1094812 - Remove now unused PlacesTransactions.RemoveBookmarksForUrls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163066/diff/1-2/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

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

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