1095425 - Convert PlacesTransactions to the new Bookmarks.jsm API

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Marco Bonardo [:mak]]

luckily it's fake-async already, but pitfalls are behind the corner.

Comment 1

[Asaf Romano (gone)]

Note that it's not just "fake-async": Most (if not all) of the |execute| functions start with a |promiseItemId|-yield, meaning that no callers could rely on anything happening before the next tick at the very least.

I was playing with this a few days ago, and successfully implemented both |EditTitle| and |EditKeyword| (the tests passed). I had issues implementing |EditUrl| and |Move| (see the dependencies I filed). Other than that:
* SortByName is blocked by bug |1081108|
* I didn't try implementing the |NewFolder/Bookmark/Separator| transactions, but I don't expect any serious issues there. The |insert| API fits them perfectly.
* |Remove| and |Copy| use |promiseBookmarksTree|, and should switch to |fetchTree| once it's implemented (bug 1081106). However, since we expect |fetchTree| to work almost the same as |promiseBookmarksTree|, this is likely to be a trivial change, and as such it doesn't have to happen in this bug. Like the |New*| transactions mentioned above, |remove| and |insert| fit |Remove| and |Copy| quite well, so I don't expect problems with those transactions either.
* |NewLivemark| uses |setItemDateAdded| on redo. Bookmarks.jsm doesn't support modifying date added for existing item, and instead allow to preset the date added value in |insert|. Fixing the livemarks service to use Bookmarks.jsm is out of the scope of this bug, but we can make |addLivemark| accept an |dateAdded| input property here (and call |setItemDateAdded| in addLivemark for now).
* |Annotate| doesn't use the Bookmarks service at all, so it's to remain unchanged.
* I don't expect issue with |Tag| and |Untag|.

Everything is covered here, I think.

Comment 2

[Asaf Romano (gone)]

promiseBookmarksTree doesn't work week wrt. keywords due to bug 1083469.

Comment 3

[Asaf Romano (gone)]

If the keywords cache patch lands during the next iteration (very likely) and also the patch for bug 1107308 (ditto), there's no reason this cannot be done promptly. The patch is almost done already. I'll file a follow up for SortByName though, which needs reorder.

Comment 4

[Asaf Romano (gone)]

Attachment: checkpoint

Comment 5

[Asaf Romano (gone)]

Attachment: checkpoint

Comment 6

[Asaf Romano (gone)]

Attachment: patch

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8555104 [details] [diff] [review]
patch

Review of attachment 8555104 [details] [diff] [review]:
-----------------------------------------------------------------

bug 1125113 and bug 1125115 should land soon, so you could likely update these with new keywords API

I will ignore the keywords parts for now.
The other difference that I must point out, is that it won't be possible anymore, with the new keywords API, to undo post_data alone, you'll have to create a keyword w/ or w/o post_data, and remove that keyword on undo.

::: toolkit/components/places/PlacesTransactions.jsm
@@ +934,2 @@
>          if ("keyword" in aItem)
> +          info.keyword = aItem.keyword;

unfortunately this has gone (no more keywords support in Bookmarks.jsm) so you'll have to revert to the old API, or wait the landing of the new one.

@@ +967,5 @@
> +          info.dateAdded = info.dateAdded * 1000;
> +          info.lastModified = info.lastModified * 1000;
> +          if (typeof(aItem.title) == "string")
> +            info.title = aItem.title;
> +          guid = (yield PlacesUtils.livemarks.addLivemark(info)).guid;

ugh, we should file a follow-up bug to make livemarks take parentGuid

@@ +1004,5 @@
>  }
>  
> +function* promiseIsBookmarked(aUrl) {
> +  let info = (yield Bookmarks.fetch({ url: aUrl }));
> +  return info != null;

nit: just return !!(yield ...?

@@ +1032,5 @@
> +  *execute({ parentGuid, url, index, title, keyword,
> +             postData, annotations, tags }) {
> +    let info = { type: Bookmarks.TYPE_BOOKMARK, parentGuid, index, url, title };
> +    if (keyword)
> +      info.keyword = keyword;

ditto (I won't comment further about info.keywords since it's clearly an open issue to be fixed globally)

@@ +1155,4 @@
>        let livemark = yield PlacesUtils.livemarks.addLivemark(livemarkInfo);
> +      if (annotations.length > 0) {
> +        PlacesUtils.setAnnotationsForItem(livemark.id, annotations);
> +        // TODO set lastModified

what's the problem doing this?
The old code was also setting dateAdded here, not sure if that's still needed, looks like you are doing that below (also for lastModified?) or am I missing something?

@@ +1190,2 @@
>  
> +    let updateInfo = { guid, parentGuid: newParentGuid, index: newIndex }

missing semicolon

@@ +1190,4 @@
>  
> +    let updateInfo = { guid, parentGuid: newParentGuid, index: newIndex }
> +    let { index, lastModified } = yield Bookmarks.update(updateInfo);
> +    [updateInfo.index, updateInfo.lastModified] = [index, lastModified];

nit: I'd prefer plain assignments. This goes a bit over the top :)

@@ +1195,5 @@
> +    let undoInfo = { guid
> +                   , parentGuid: originalInfo.parentGuid
> +                   , index: originalInfo.index };
> +
> +    // Moving down in the same parent takes in count removal of the item

nit: into account (I surely wrote that!)

@@ +1281,3 @@
>        }
>      };
> +    // TODO: optimized redo

so, looks like missing a piece?

@@ +1374,5 @@
> +    let preSepNodes = []; // nodes
> +
> +    let contents =
> +      PlacesUtils.getFolderContents((yield PlacesUtils.promiseItemId(guid)), false, false)
> +                 .root;

can we use promiseBookmarksTree here?

@@ +1442,3 @@
>        }
> +      yield fixKeywordsInBookmarksTree(tree);
> +      return tree; 

trailing space

Comment 8

[Asaf Romano (gone)]

Attachment: patch

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8598591 [details] [diff] [review]
patch

Review of attachment 8598591 [details] [diff] [review]:
-----------------------------------------------------------------

this is mostly good! Some details to fix

::: toolkit/components/places/PlacesTransactions.jsm
@@ +711,4 @@
>    if (url instanceof Components.interfaces.nsIURI)
> +    return new URL(url.spec);
> +  if (typeof(url) == "string")
> +    return new URL(url);

fwiw, Bookmarks.jsm also accepts strings, as well as the new keywords API.

@@ +919,5 @@
> +    let info = { parentGuid: aParentGuid, index: aIndex };
> +    if (aRestoring) {
> +      info.guid = aItem.guid;
> +      info.dateAdded = new Date(aItem.dateAdded / 1000);
> +      info.lastModified = new Date(aItem.lastModified / 1000);

please copy toDate() and toPRTime() from Bookmarks.jsm and use those utils around. Or move them to PlacesUtils, I don't care.

It's possible we arrive here with an undefined dateAdded or lastModified?
new Date(undefined / 1000) doesn't throw, it instead generates "Invalid Date"
It might be worth to add protection to the utils.

@@ +929,5 @@
>        case PlacesUtils.TYPE_X_MOZ_PLACE: {
> +        info.type = Bookmarks.TYPE_BOOKMARK;
> +        info.url = aItem.uri;
> +        if (typeof(aItem.title) == "string")
> +          info.title = aItem.title;

FYI: undefined is a valid value (a property set to undefined will be ignored), as well as null

@@ +967,5 @@
> +          info.parentId = yield PlacesUtils.promiseItemId(aParentGuid);
> +          info.feedURI = feedURI;
> +          info.siteURI = siteURI;
> +          info.dateAdded = info.dateAdded * 1000;
> +          info.lastModified = info.lastModified * 1000;

as well as here, using a toPRTime would be nice.
and the undefined story is also valid here (undefined * 1000 is NaN)

Soon or later I will make livemarks accept both Date or PRTime...

@@ +969,5 @@
> +          info.siteURI = siteURI;
> +          info.dateAdded = info.dateAdded * 1000;
> +          info.lastModified = info.lastModified * 1000;
> +          if (typeof(aItem.title) == "string")
> +            info.title = aItem.title;

ditto about title accepting undefined

@@ +1007,5 @@
>  }
>  
> +function* promiseIsBookmarked(aUrl) {
> +  let info = (yield Bookmarks.fetch({ url: aUrl }));
> +  return info != null;

info doesn't seem useful, just return !!(yield Bookmarks.fetch...

@@ +1047,5 @@
> +      // The keyword might have been set for another url, so we need to check both
> +      // ways.
> +      oldKeywordEntry =
> +        yield PlacesUtils.keywords.fetch({ url });
> +        yield PlacesUtils.keywords.fetch({ keyword });

something is wrong here... I guess you didn't want a semicolon in the middle
(fwiw you can just .fetch(keyword) without an object)

@@ +1060,5 @@
> +      info = yield Bookmarks.insert(info);
> +      if (postData || annotations.length > 0) {
> +        let itemId = yield PlacesUtils.promiseItemId(info.guid);
> +        if (postData)
> +          PlacesUtils.setPostDataForBookmark(itemId, postData);

This is deprecated :(

@@ +1082,5 @@
> +      if (keywordChangeRequested) {
> +        if (oldKeywordEntry)
> +          yield PlacesUtils.keywords.insert(oldKeywordEntry);
> +        else
> +          yield PlacesUtils.keywords.remove(keyword);

The problem here is that there might be other bookmarks with the same uri and thus still using this keyword... but you're removing it.... Note that if the last bookmark holding a keyword is removed, the keyword will be removed, asynchronously though.

@@ +1178,2 @@
>      let createItem = function* () {
> +      livemarkInfo.parentId = yield PlacesUtils.promiseItemId(parentGuid);

This is no more needed, now the livemarks API accepts and returns parentGuid (in addition to parentId)

@@ +1309,3 @@
>        }
>      };
> +    // TODO: optimized redo

Please file a bug, or resolve, or explain...

@@ +1366,5 @@
> +  *execute({ guid, keyword, postData }) {
> +    let { url } = yield Bookmarks.fetch(guid);
> +    let oldKeywordEntry =
> +      yield PlacesUtils.keywords.fetch({ url }) ||
> +      keyword ? yield PlacesUtils.keywords.fetch({ keyword }) : null;

this needs some indentation fix

@@ +1367,5 @@
> +    let { url } = yield Bookmarks.fetch(guid);
> +    let oldKeywordEntry =
> +      yield PlacesUtils.keywords.fetch({ url }) ||
> +      keyword ? yield PlacesUtils.keywords.fetch({ keyword }) : null;
> +    

trailing spaces

@@ +1373,5 @@
> +        oldKeywordEntry.url != url || oldKeywordEntry.keyword != keyword) {
> +      if (keyword)
> +        yield PlacesUtils.keywords.insert({ url, keyword, postData});
> +      else
> +        yield PlacesUtils.keywords.remove(keyword);

As I said, it's not that easy to remove a keyword, since it might be shared by multiple bookmarks...

@@ +1413,5 @@
> +    let preSepNodes = []; // nodes
> +
> +    let contents =
> +      PlacesUtils.getFolderContents((yield PlacesUtils.promiseItemId(guid)), false, false)
> +                 .root;

We will likely want (for future) a fetchTree option to stop at the first level...

@@ +1467,2 @@
>        }
> +      return tree; 

trailing space

@@ +1472,5 @@
>      let removeThem = Task.async(function* () {
> +      for (let info of removedItems) {
> +        if (info.annos && info.annos.some(anno => anno.name == PlacesUtils.LMANNO_FEEDURI)) {
> +          yield PlacesUtils.livemarks.removeLivemark({ guid: info.guid });
> +        } 

trailing space

::: toolkit/components/places/nsLivemarkService.js
@@ +232,5 @@
>          livemark.writeSiteURI(aLivemarkInfo.siteURI);
>        }
>  
> +      if (aLivemarkInfo.lastModified) {
> +        yield PlacesUtils.bookmarks.update({ guid: folder.guid, lastModified: toDate(aLivemarkInfo.lastModified) });

I actually didn't do this cause I'm not sure if it's worth the additional cost of a query... should lastModified really be so precise?
If you think it's worth it, though, here you should also set livemark.lastModified, or it will be wrong.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

friendly ping- this is blocking some intermittents- not sure if there is a small amount of work to land this, or if it requires a day or two to finish up.

Comment 11

[Asaf Romano (gone)]

Patch coming; trying to address the keyword issues.

Comment 12

[Marco Bonardo [:mak]]

note I landed the livemarks lastModified fix in bug 1158887.

Comment 13

[Asaf Romano (gone)]

Getting back to this tomorrow to finish it up.

Comment 14

[Marco Bonardo [:mak]]

note, test_async_transactions has temporarily been disabled on WinXP due to failures, I suspect the problem is related to PlacesTransactions.jsm trying to set a lastModified < dateAdded, causing the update call in livemarks service to fail

Comment 15

[Naveed Ihsanullah [:naveed]]

Marco - Is this bug directly impacting user performance still? If so it probably should be fixed as part of Quantum Flow prioritization.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Naveed Ihsanullah [:naveed] from comment #15)
> Marco - Is this bug directly impacting user performance still? If so it
> probably should be fixed as part of Quantum Flow prioritization.

It is impacting main-thread I/O, provided bug 1071513 is also fixed.

Comment 17

[Marco Bonardo [:mak]]

ops, wrong Marco! Btw, maybe it was useful regardless.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

I'm 100% certain that the needinfo was actually supposed to be directed at you, mak, so thanks!

Comment 19

[Marco Bonardo [:mak]]

Attachment: Bug 1095425 - Convert PlacesTransactions to the new Bookmarks.jsm API.

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

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8882306 [details]
Bug 1095425 - Convert PlacesTransactions to the new Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/153398/#review158612

::: toolkit/components/places/PlacesTransactions.jsm:926
(Diff revision 1)
> -async function createItemsFromBookmarksTree(aBookmarksTree, aRestoring = false,
> -                                       aExcludingAnnotations = []) {
> -  function extractLivemarkDetails(aAnnos) {
> +// TODO: Replace most of this with insertTree.
> +async function createItemsFromBookmarksTree(tree, restoring = false,
> +                                            excludingAnnotations = []) {
> +  function extractLivemarkDetails(annos) {
>      let feedURI = null, siteURI = null;
> -    aAnnos = aAnnos.filter(
> +    annos = annos.filter(anno => {

Not sure why we're assigning to itself here, seeing as it doesn't get used later.

Although, why are we doing filter at all?

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8882306 [details]
Bug 1095425 - Convert PlacesTransactions to the new Bookmarks.jsm API.

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

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 8882306 [details]
Bug 1095425 - Convert PlacesTransactions to the new Bookmarks.jsm API.

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

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 8882306 [details]
Bug 1095425 - Convert PlacesTransactions to the new Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/153398/#review158690

::: toolkit/components/places/PlacesTransactions.jsm:975
(Diff revision 3)
>          }
>          break;
>        }
>        case PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER: {
>          // Either a folder or a livemark
> -        let [feedURI, siteURI] = extractLivemarkDetails(annos);
> +        let [feedURI, siteURI, annos] = extractLivemarkDetails(annos);

`annos` already exists, so you need to not have the `let` here.

```
ReferenceError: can't access lexical declaration `annos' before initialization
```

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 8882306 [details]
Bug 1095425 - Convert PlacesTransactions to the new Bookmarks.jsm API.

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

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 8882306 [details]
Bug 1095425 - Convert PlacesTransactions to the new Bookmarks.jsm API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153398/diff/4-5/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f216794fa139
Convert PlacesTransactions to the new Bookmarks.jsm API. r=standard8

Comment 27

[Phil Ringnalda (:philor)]

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