Closed Bug 775495 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 3 obsolete files)

waitForClearHistory(callback) ==> promiseClearHistory().then(callback)
Attached patch The patch (obsolete) — Splinter Review
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.
Attachment #643819 - Flags: feedback?(mak77)
Depends on: 774816
(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.
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?
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.
Blocks: 775580
Depends on: 756542
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 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|)
Attachment #643819 - Flags: feedback+
(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.
(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.
Attachment #655306 - Flags: feedback?(dteller)
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.
Attachment #655306 - Flags: feedback?(dteller) → feedback+
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.
Attachment #655306 - Flags: feedback+
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.
Attachment #643819 - Flags: feedback?(mak77) → feedback+
(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.
Summary: Replace usage of waitForClearHistory with promiseClearHistory → Replace waitForClearHistory and waitForAsyncUpdates with versions that return promises, and remove waitForFrecency
Attached patch The patch (obsolete) — Splinter Review
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?
Attachment #643819 - Attachment is obsolete: true
Attachment #655306 - Attachment is obsolete: true
Attachment #657401 - Flags: review?(mak77)
(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 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)
Attachment #657401 - Flags: review?(mak77) → review+
Flags: needinfo?(paolo.mozmail)
Attached patch Final patchSplinter Review
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.
Attachment #657401 - Attachment is obsolete: true
Flags: needinfo?(paolo.mozmail)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d785c6a18a3e
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/d785c6a18a3e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.