Closed Bug 1069410 Opened 6 years ago Closed 6 years ago

CloudSync should not be using setItemIndex

Categories

(Cloud Services :: cloudSync, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: mak, Assigned: akligman)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(akligman)
setItemIndex is used to reorder items in a container. I think it can be changed to moveItem instead without a problem.
Flags: needinfo?(akligman)
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?
Flags: needinfo?(akligman)
or otherwise, could you mentor someone to make this change?
I can do this today or tomorrow. It should be landed before uplifting to aurora.
Flags: needinfo?(akligman)
thanks!
Assignee: nobody → akligman
Status: NEW → ASSIGNED
Attachment #8492695 - Flags: review?(mak77)
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);
}));
Attachment #8492695 - Flags: review?(mak77)
probably I've put one ) more than needed, but I didn't test it.
Could you elaborate on why Task.jsm should be used here?
Flags: needinfo?(mak77)
(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.
Flags: needinfo?(mak77)
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.
Attachment #8492695 - Attachment is obsolete: true
Attachment #8493822 - Flags: review?(mak77)
Attachment #8493822 - Flags: review?(mak77) → review+
Keywords: checkin-needed
(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.
https://hg.mozilla.org/mozilla-central/rev/6c0248e2e44b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla35
Whiteboard: [cloudsync]
Component: Places → cloudSync
Product: Toolkit → Mozilla Services
Whiteboard: [cloudsync]
Blocks: 1235054
You need to log in before you can comment on or make changes to this bug.