This "pilot" bug removes one instance of nested asynchronous addVisit calls. The same approach could be applied in various other places as well.

This includes new promiseAsyncUpdates and promiseAddVisits helpers.

Comment on attachment 643873 [details] [diff] [review] The patch Review of attachment 643873 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks.

This adds the final version of the promiseAddVisits function.

Comment on attachment 683501 [details] [diff] [review] Updated patch Review of attachment 683501 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/tests/head_common.js @@ +942,5 @@ > + * on success, or reports a test error on failure. > + * > + * @see promiseAddVisits > + */ > +function addVisits(aPlaceInfo, aCallback, aStack) please file a follow-up bug to get rid of this duplicated helper, we should just switch everything to the promiseAddVisits version. Add a "@note deprecated, please use promiseAddVisits instead." ::: toolkit/components/places/tests/queries/test_sorting.js @@ +1267,2 @@ > // sorting reversed, usually SORT_BY have ASC and DESC > + yield test.check_reverse(); most setup(), all of check() and all of check_reverse() calls are synchronous and don't return anything, doesn't yield require to get a promise? Is this voodoo magic from Task?

(In reply to Marco Bonardo [:mak] from comment #4) > most setup(), all of check() and all of check_reverse() calls are > synchronous and don't return anything, doesn't yield require to get a > promise? Is this voodoo magic from Task? OK I think I got an answer by the fact Task just continues if yield is not a promise. Though looks strange to use it in check and check_reverse, I couldn't find an async case there.

(In reply to Marco Bonardo [:mak] from comment #5) > OK I think I got an answer by the fact Task just continues if yield is not a > promise. Though looks strange to use it in check and check_reverse, I > couldn't find an async case there. Yeah, this is done on purpose (see the Task.jsm documentation) to avoid pitfalls that may happen when you batch-remove a call that is not needed anymore, and doing that you also remove the last yield call. For example, before: add_task(function test() { yield prepareLegacyDataStructures(); do_check_true(featureIsOk); }); And after: add_task(function test() { do_check_true(featureIsOk); }); This is trivial here, but may not be so obvious in longer test functions. In this case it's the other way around, in case you need to add a yield statement.

(In reply to Paolo Amadini [:paolo] from comment #6) > (In reply to Marco Bonardo [:mak] from comment #5) > > OK I think I got an answer by the fact Task just continues if yield is not a > > promise. Though looks strange to use it in check and check_reverse, I > > couldn't find an async case there. > > Yeah, this is done on purpose (see the Task.jsm documentation) to avoid > pitfalls > that may happen when you batch-remove a call that is not needed anymore, and > doing that you also remove the last yield call. For example, before: the test timeouts in such a case, doesn't it? So it's a detectable issue once you do the change and see the test fails.