1069410 - CloudSync should not be using setItemIndex

Status: RESOLVED FIXED

(Cloud Services Graveyard :: cloudSync, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So we have a problem with CloudSyncBookmarks.jsm that is using setItemIndex bookmarks API.

that API is very dangerous and should only be used to completely sort all children of a container. moveItem should instead be used to move bookmarks.

We are about to deprecate that API and will remove it as soon as we have an alternative in the new asynchronous bookmarking API we are building.

Is that method really needed here? what is the code using it trying to do?

Comment 1

[Alan K [:ack]]

setItemIndex is used to reorder items in a container. I think it can be changed to moveItem instead without a problem.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes moveItem should be fine for now. We will introduce a better reorder API in bug 519514, but then every bookmarks API call will change and be asynchronous. But I'll file a bug in the next weeks about that, once we have some implementation.

Is cloudSync already wrapping any calls so that it's easy to make them async? It looks so, but I'd like confirmation.

Finally, do you have cycles to make this change?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

or otherwise, could you mentor someone to make this change?

Comment 4

[Alan K [:ack]]

I can do this today or tomorrow. It should be landed before uplifting to aurora.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

thanks!

Comment 6

[Alan K [:ack]]

Attachment: 0001-B-1069410-CloudSync-should-not-be-using-setItemIndex.patch

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8492695 [details] [diff] [review]
0001-B-1069410-CloudSync-should-not-be-using-setItemIndex.patch

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

in future with then new guid based api this will be much simpler.

::: services/cloudsync/CloudSyncBookmarks.jsm
@@ +501,5 @@
> +                      )
> +                    .then(deferred.resolve, deferred.reject);
> +                }
> +              )
> +          promises.push(deferred.promise);

could you please use Task.jsm here? Something like this should work:

promises.push(Task.spawn(function* () {
  let item = (yield getLocalItemsById([items.id]))[0];
  let parent = yield PlacesWrapper.guidToLocalId(item.parent);
  let index = item.index;
  if (CS_FOLDER & item.type) {
    folderCache.setParent(localId, parent);
  }
  yield PlacesWrapper.moveItem(localId, parent, index);
}));

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

probably I've put one ) more than needed, but I didn't test it.

Comment 9

[Alan K [:ack]]

Could you elaborate on why Task.jsm should be used here?

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Alan K [:ack] from comment #9)
> Could you elaborate on why Task.jsm should be used here?

it makes the code readable without adding any downside.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

We use Task.jsm extensively for Firefox Desktop. It makes async code and tests indeed much more readable. The patch you attached is a great example - using Task.jsm it simply reads like a list of statements that are executed, return results, and we additionally get a promise for the whole operation (comment #7). Doing this with only promises brings back the nested callbacks that we were trying to avoid in the first place.

Looking at CloudSyncBookmarks.jsm, you could have easily defined methods here using Task.async(), we should probably file a bug to come back later and clean this up.

Comment 12

[Alan K [:ack]]

Attachment: 0001-B-1069410-CloudSync-should-not-be-using-setItemIndex.patch

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/6c0248e2e44b

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #11)
> Looking at CloudSyncBookmarks.jsm, you could have easily defined methods
> here using Task.async(), we should probably file a bug to come back later
> and clean this up.

Filed bug 1072374.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c0248e2e44b