896193 - Adopt Promises in mozIAsyncLivemarks

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Comment 2

[Marco Bonardo [:mak]]

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

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

The change is ok, but you missed a lot of consumers, I don't think we should deprecate the callback until all of the internal consumers are converted:
http://mxr.mozilla.org/mozilla-central/search?string=.getLivemark
http://mxr.mozilla.org/mozilla-central/search?string=.addLivemark
http://mxr.mozilla.org/mozilla-central/search?string=.removeLivemark

So, either you convert all of them, or you delay deprecation to a follow-up bug.

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +389,5 @@
>                  if (aData.dateAdded)
>                    PlacesUtils.bookmarks.setItemDateAdded(id, aData.dateAdded);
>                  if (aData.annos && aData.annos.length)
>                    PlacesUtils.setAnnotationsForItem(id, aData.annos);
> +              });

nit: I'm not sure about the indentation here

::: toolkit/components/places/PlacesUtils.jsm
@@ +1212,5 @@
>              }
>            }, this);
>  
>            if (feedURI) {
> +            Task.spawn(function() {

why spawning a Task here, instead of just using .then() ?

@@ +2313,5 @@
>    __proto__: BaseTransaction.prototype,
>  
>    doTransaction: function CLTXN_doTransaction()
>    {
> +    Task.spawn(function() {

ditto

@@ +2331,5 @@
>    },
>  
>    undoTransaction: function CLTXN_undoTransaction()
>    {
> +    // This is expected to fail, but  it is used just to serialize, so doesn't matter.

double spacing after but

@@ +2332,5 @@
>  
>    undoTransaction: function CLTXN_undoTransaction()
>    {
> +    // This is expected to fail, but  it is used just to serialize, so doesn't matter.
> +    Task.spawn(function() {

ditto

@@ +2378,5 @@
>    __proto__: BaseTransaction.prototype,
>  
>    doTransaction: function RLTXN_doTransaction()
>    {
> +    Task.spawn(function() {

ditto

@@ +2390,5 @@
>    },
>  
>    undoTransaction: function RLTXN_undoTransaction()
>    {
> +    Task.spawn(function() {

this is probably the only case where it improves things since there are multiple yield.

::: toolkit/components/places/nsLivemarkService.js
@@ +16,5 @@
>                                    "resource://gre/modules/PlacesUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
>                                    "resource://gre/modules/NetUtil.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +                                  "resource://gre/modules/commonjs/sdk/core/promise.js");

Please use the new Promise implementation at "resource://gre/modules/Promise.jsm" instead

::: toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js
@@ +170,5 @@
>      onEndUpdateBatch: function onEndUpdateBatch() {},
>      onItemRemoved: function onItemRemoved() {},
>      onItemChanged: function onItemChanged() {},
>      onItemVisited: function onItemVisited() {},
>      onItemMoved: function onItemMoved() {},

please use __proto__: NavBookmarkObserver.prototype, even if you dislike it :)

@@ +178,5 @@
> +    { title: "test"
> +    , parentId: PlacesUtils.unfiledBookmarksFolderId
> +    , index: PlacesUtils.bookmarks.DEFAULT_INDEX
> +    , feedURI: FEED_URI });
> +  do_check_true(onItemAddedCalled);

I'd prefer if you'd make a deferred here, yield on deferred.promise and resolve at the end of the callback, it's more future proof, as it is, it relies on synchronous callbacks.

@@ +342,2 @@
>      do_check_eq(aLivemark.guid, "1234567890AB");
> +    do_check_guid_for_bookmark(aLivemark.id, "1234567890AB");  

trailing spaces

@@ +496,5 @@
> +    yield PlacesUtils.livemarks.getLivemark({ guid: "34567890ABCD" },
> +      (aStatus, aLivemark) => {
> +        callbackCalled = true;
> +        do_check_false(Components.isSuccessCode(aStatus));
> +        do_check_eq(aLivemark, null); 

trailing space

@@ +522,5 @@
> +    do_check_true(aLivemark.feedURI.equals(FEED_URI));
> +    do_check_eq(aLivemark.siteURI, null);
> +    do_check_eq(aLivemark.guid, "34567890ABCD");
> +  };
> +   

trailing spaces

@@ +564,5 @@
> +      checkLivemark(aLivemark);
> +    } );
> +
> +  do_check_true(callbackCalled);
> +  checkLivemark(livemark); 

trailing space

@@ +624,5 @@
> +    { title: "test"
> +    , parentId: PlacesUtils.unfiledBookmarksFolderId
> +    , index: PlacesUtils.bookmarks.DEFAULT_INDEX
> +    , feedURI: FEED_URI } );
> +  

trailing spaces

Comment 3

[Asaf Romano (gone)]

> I'd prefer if you'd make a deferred here, yield on deferred.promise and resolve at the end > of the callback, it's more future proof, as it is, it relies on synchronous callbacks.

The callbacks are going to be removed, so there's no point in complicating this code, imo.

Comment 4

[Marco Bonardo [:mak]]

(In reply to Mano from comment #3)
> > I'd prefer if you'd make a deferred here, yield on deferred.promise and resolve at the end > of the callback, it's more future proof, as it is, it relies on synchronous callbacks.
> 
> The callbacks are going to be removed, so there's no point in complicating
> this code, imo.

I think my point was that we can't rely on onItemAddedCalled being invoked as soon as the method is invoked.

Comment 5

[Asaf Romano (gone)]

Attachment: patch

I'll deprecate and fix the consumers in a follow up. Here I'll just do what the undo manager needs asap.

Comment 6

[Marco Bonardo [:mak]]

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

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

first: remember the SR

::: toolkit/components/places/nsLivemarkService.js
@@ +310,3 @@
>      }
>      finally {
> +      this._onCacheReady( () => {

nit: in some case you add a whitespace before (), in some case you don't, I'd appreciate coherent styling, whatever of the two you pick.

@@ +357,5 @@
>        !aForceUpdate;       // The caller didn't request a forced update.
>      if (this._reloading && notWorthRestarting) {
>        // Ignore this call.
>        return;
>      } 

nit: while here please remove the trailing space here?

Comment 7

[Asaf Romano (gone)]

Gavin: ping.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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

typo: optioanl

The promise-or-callback API design seems kind of confusing, but I guess getting rid of the callbacks would be a compat nightmare.

Is there a strong need for moving this API to promises? I understand that it's nicer, but not sure how much, and I'm not sure that justifies the extra complexity.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mano from comment #3)
> The callbacks are going to be removed

Is this realistic? On what timeframe?

Comment 10

[Asaf Romano (gone)]

Well, it certainly makes consumer code cleaner, especially because they are (or they are going to) return promises.

I don't think it's all that unrealistic. The livemarks API was always a moving target. We land it now; We fix our consumers in the next release and deprecate the callbacks, half a year later we remove them on trunk.

Comment 11

[Raymond Lee [:raymondlee]]

Is this ready to check-in? Coz I am waiting for this in order to fix Bug 886054 :-)

Comment 12

[Asaf Romano (gone)]

Attachment: for checkin

As expected, there are very few addons that use this API, so I decided to land it. I'll file another bug/s for deprecating the callbacks API and fix our own consumers.

I'll land this once the tree reopens

Comment 13

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0879fec4cfc3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent Linux xpcshell failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c6f5d02db22

https://tbpl.mozilla.org/php/getParsedLog.php?id=30828872&tree=Mozilla-Inbound

Comment 15

[Marco Bonardo [:mak]]

I guess I will try to reproduce the failure on Linux, unfortunately the log got lost, would be good to paste the error along with the link to the log :(

Comment 16

[Marco Bonardo [:mak]]

I was able to reproduce once a failure in test_addLivemark_noSiteURI_callback_succeeds though I was dumb and didn't mark it down, now after a rebuild I can't reproduce anymore. I pushed to try and will see if I have more luck there

Comment 17

[Marco Bonardo [:mak]]

https://tbpl.mozilla.org/?tree=Try&rev=76065f42428f

Comment 18

[Marco Bonardo [:mak]]

ok, there's something wrong with the guid setter in test_addLivemark_forceGuid_callback_succeeds

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js | "" == "1234567890AB" 

plus a strange (and much less frequent)

00:56:25     INFO -  JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 233
00:56:25     INFO -  JS frame :: resource://gre/modules/Promise.jsm :: Handler.prototype.process :: line 767
00:56:25     INFO -  JS frame :: resource://gre/modules/Promise.jsm :: this.PromiseWalker.walkerLoop :: line 531
00:56:25     INFO -  native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
00:56:25     INFO -  TEST-INFO | (xpcshell/head.js) | exiting test
00:56:25  WARNING -  TEST-UNEXPECTED-FAIL | resource://gre/modules/commonjs/sdk/core/promise.js | Unexpected exception 2147500036

Comment 19

[Marco Bonardo [:mak]]

the problem is related to the fact the guid is set through an async statement, and we don't wait for its completion before calling back from addLivemark... we should instead wait for that

Comment 20

[Marco Bonardo [:mak]]

ok, it's even simpler, Mano just wrongly removed the second param from the _onCacheReady call in addLivemark, that was making us always enqueue to any async statement. This enqueuing could now be done better with Task and Promises, but since it can be fixed by merely adding back the second param and was not the scope of this bug, I'll just repush the same identical patch with a ",true" as it was before

Comment 21

[Marco Bonardo [:mak]]

Attachment: fixed patch

Comment 22

[Marco Bonardo [:mak]]

Attachment: fixed patch

the interdiff pointed out some whitespace difference, this fixes the meaningful ones

Comment 23

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c3303aa3d40

adding addon-compat, even if actually I don't expect any real issue here, js consumers will keep working as before, we are not aware of cpp consumers as of now.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c3303aa3d40

Comment 25

[Bob Copeland]

Sorry to post on a closed bug, so maybe this won't be seen, but as one of the (unfortunate) developers of an addon that uses livemarks, my google fu is lacking -- how do we tell which versions work with the callback api and which return promises, without trying and getting deprecation warnings?  We already use our own promise API internally to convert the callback to a promise, so it's not hard to support old and new versions with the same code, so long as we know what to look for.

Comment 26

[Bob Copeland]

(In reply to Bob Copeland from comment #25)
> is lacking -- how do we tell which versions work with the callback api and
> which return promises, without trying and getting deprecation warnings?  We

Just to clarify, I guess something like this would work as detection of which browsers support returning promises, but would like to know if there's a better / official way.  Obviously trying with a callback results in an ugly deprecation warning.  For older browsers we would continue using the callbacks.

   var promise = moziasynclivemarks.getLivemark({id: 1});
   this.UseLMPromises = promise != null;

Comment 27

[Marco Bonardo [:mak]]

you can just detect firefox version and write conditional code based on that, promises were introduced in Firefox 29