775495 - Replace waitForClearHistory and waitForAsyncUpdates with versions that return promises, and remove waitForFrecency

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Paolo Amadini]

waitForClearHistory(callback) ==> promiseClearHistory().then(callback)

Comment 1

[:Paolo Amadini]

Attachment: The patch

The advantage of promises is that you can decide how to wait for them to be
resolved, case by case: directly using "then" (like in this first search and
replace), or with alternate solutions like a Task (bug 763311), or in the worst
case with a function that spins an event loop. But you don't need to hardcode
that choice in the called function itself.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Paolo Amadini [:paolo] from comment #1)
> Created attachment 643819 [details] [diff] [review]
> The patch
> 
> The advantage of promises is that you can decide how to wait for them to be
> resolved, case by case: directly using "then" (like in this first search and
> replace), or with alternate solutions like a Task (bug 763311), or in the
> worst
> case with a function that spins an event loop. But you don't need to hardcode
> that choice in the called function itself.

Do we need that choice here? When every consumer just wants to run a callback, this doesn't seem like a useful change, i.e. this patch doesn't seem to simplify anything.

Comment 3

[Marco Bonardo [:mak]]

yeah, while I see the point, I'm not sure what's the benefit in changing all the tests... do you have a case where the old function is actually creating issues or blocking some change?

Comment 4

[:Paolo Amadini]

I'm modifying some of the consumers so they won't invoke "then" directly on the
returned promise.

I've also considered keeping the old function around for all the other places that
invoke the callback directly, but in this case it seemed to me that there's no
point in maintaining two ways of doing the same thing, when one is so similar to
the other.

By the way this can definitely wait to land until there's the next pacth ready.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Paolo, I am quite willing to review your patches, but they are too long. Could you at least separate the utilities from the tests?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 643819 [details] [diff] [review]
The patch

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

Patch looks good to me.

Now, two remarks. Firstly, in the future, could you please make smaller patches? You could for instance, split them by (sub)directory.

Secondly, while I am a big fan of promises, I am not sure that they add anything in this context. Is this for harmonization with the patches in which you port the rest of the testsuite and make it use the asynchronous API?

::: toolkit/components/places/tests/head_common.js
@@ +377,2 @@
>   *
> + * @return Promise resolved once clear history has finished.

In other patches, I have started adopting the following convention:
@return {Promise}
@resolves The resolution (if not undefined)
@rejects The rejection (if any meaningful rejection)

Maybe we could standardize this?

::: toolkit/components/places/tests/unit/test_removeVisitsByTimeframe.js
@@ +394,5 @@
>  function run_next_test() {
>    if (gTests.length) {
>      let test = gTests.shift();
>      print("\n ***Test: " + test.desc);
> +    promiseClearHistory().then(function() {

You might take this opportunity to give a name to the function.

Maybe something like |onClearHistory|.

(this goes also for other functions passed to |then|)

Comment 7

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> Now, two remarks. Firstly, in the future, could you please make smaller
> patches? You could for instance, split them by (sub)directory.

The rest of the patches are, it actually didn't make sense on this one since
it's only a global search and replace.

> Secondly, while I am a big fan of promises, I am not sure that they add
> anything in this context. Is this for harmonization with the patches in
> which you port the rest of the testsuite and make it use the asynchronous
> API?

Sure, this is preliminary to the work in other patches.

> In other patches, I have started adopting the following convention:
> @return {Promise}
> @resolves The resolution (if not undefined)
> @rejects The rejection (if any meaningful rejection)
> 
> Maybe we could standardize this?

Looks good! Will do that in the following iterations.

> > +    promiseClearHistory().then(function() {
> 
> You might take this opportunity to give a name to the function.

We sometimes use named functions in Places tests, especially when the functions
are long, but we concluded that it's less useful than production code because in
tests we have full stack traces with meaningful line numbers.

Comment 8

[:Paolo Amadini]

Attachment: Helper functions for using promises in Places tests

(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> Could you at least separate the utilities from the tests?

This is the final version of the helper functions (not yet updated with your
comments), should help in following the other patches in dependent bugs.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 655306 [details] [diff] [review]
Helper functions for using promises in Places tests

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

Still looks good to me.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 655306 [details] [diff] [review]
Helper functions for using promises in Places tests

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

::: toolkit/components/places/tests/head_common.js
@@ +63,5 @@
> +/**
> + * Like "add_test", but takes a generator function that runs as a task.
> + * "run_next_test" is automatically called when the task terminates.
> + */
> +function add_test_task(aTestFn) {

Maybe we should not introduce a new add_test variant (the original one is in the global harness and well known), but rather a wrapper, so use it like add_test(new TaskWrapper(test_function)); or a wrapAsTask helper.

@@ +415,2 @@
>   */
> +function promiseClearHistory(aCallback) {

aCallback is now unused

@@ +585,5 @@
> +function promiseFrecency(aURI, aValidator) {
> +  let deferred = Promise.defer();
> +  waitForFrecency(aURI, aValidator, deferred.resolve);
> +  return deferred.promise;
> +}

why did you not convert waitForFrecency at this point, just to separate work?
Btw, I think we should just kill waitForFrecency (and this new helper) and replace its only call with waitForUpdates (or promiseAsyncUpdates in this case)... does that work properly? if so just do that.

@@ +609,2 @@
>    let frecency = stmt.getInt32(0);
>    stmt.finalize();

just move finalize() into a finally?

@@ +691,5 @@
>    commit.finalize();
>  }
>  
> +function promiseAsyncUpdates()
> +{

so, does this exist just to avoid having to replace all waitForAsyncUpdates callpoints here?
I mean, that'd be fine, just asking to be sure.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 643819 [details] [diff] [review]
The patch

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

yes, I thought about the change quite a bit, while there isn't an immediate benefit, I think it's a good move forward and to support future changes. These new modules look promising and having some internal use can bring benefits more globally, I think we won't regret it.

Comment 12

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #11)
> yes, I thought about the change quite a bit, while there isn't an immediate
> benefit, I think it's a good move forward and to support future changes.
> These new modules look promising and having some internal use can bring
> benefits more globally, I think we won't regret it.

Thanks, then I'll write a new patch where I'll replace also waitForAsyncUpdates
and waitForFrecency in addition to waitForClearHistory. Maybe this patch will be
ready by tomorrow.

(In reply to Marco Bonardo [:mak] from comment #10)
> Maybe we should not introduce a new add_test variant (the original one is in
> the global harness and well known), but rather a wrapper, so use it like
> add_test(new TaskWrapper(test_function)); or a wrapAsTask helper.

I'm fine with a wrapAsTask helper here.

> Btw, I think we should just kill waitForFrecency (and this new helper) and
> replace its only call with waitForUpdates (or promiseAsyncUpdates in this
> case)... does that work properly? if so just do that.

I'll check if it works as expected, and I'll do that if it works.

Comment 13

[:Paolo Amadini]

Attachment: The patch

Here is the patch to replace waitForClearHistory and waitForAsyncUpdates with
versions that return promises, while removing waitForFrecency.

Is there a reason why browser-chrome tests don't import head_common.js?

Comment 14

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini [:paolo] from comment #13)
> Is there a reason why browser-chrome tests don't import head_common.js?

Well it's another harness (with any complication that may add, regarding globals) and may be complex to figure which one is built/run before.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 657401 [details] [diff] [review]
The patch

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

Assuming try is happy (please do a run and then retrigger xpcshell at least 10 times to check for evident random failures)

Comment 16

[:Paolo Amadini]

Attachment: Final patch

Missed a few call points, this version passes all tests. I've also verified that
there doesn't seem to be any random failures for the waitForFrecency removal.

Comment 17

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d785c6a18a3e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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