778699 - Remove calls to addVisit from the "unit" folder

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Paolo Amadini]

This removes more nested asynchronous addVisit calls.

Comment 1

[:Paolo Amadini]

Attachment: The patch

Nothing special here, except that promiseAddVisits now rejects on failure with a
descriptive exception, that includes the result code.

This makes it possible for test tasks to check expected exceptions resulting
from the asynchronous call, with the usual try/catch construct.

Comment 2

[:Paolo Amadini]

Note that some tests from this folder are still missing, a new patch will follow.

Comment 3

[:Paolo Amadini]

Attachment: Remove all calls

This removes all the synchronous calls, and works.

We can decide whether we want to do more cleanup on a case-by-case basis.

Comment 4

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

Code is a little too long for me to be able to grok it at once, but so far, I like what I see.

Comment 5

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

Comment on attachment 647618 [details] [diff] [review]
Remove all calls

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

Given the number of files in the patch, I had a quick look at a few of the files. Could you reply to these questions before I continue?

(clearing the flags to make space on my dashboard, do not hesitate to re-ask me for feedback once you have replied)

::: toolkit/components/places/tests/head_common.js
@@ +586,5 @@
> +  let deferred = Promise.defer();
> +  waitForFrecency(aURI, aValidator, deferred.resolve);
> +  return deferred.promise;
> +}
> +

I don't see |Promise| defined/imported anywhere.

@@ +940,5 @@
> +      do_throw(ex, stack);
> +    }
> +  );
> +}
> +

addVisits does not return the promise, is this normal?

::: toolkit/components/places/tests/unit/test_000_frecency.js
@@ +57,5 @@
>  var results = [];
>  var matchCount = 0;
>  var now = Date.now();
>  var prefPrefix = "places.frecency.";
> +

Given that most people are unfamiliary with Task.js, you should probably add at least some type documentation on this function (and explain somewhere that Task.spawn handles the waiting).

@@ +198,5 @@
>  }
>  
>  function run_test() {
>    do_test_pending();
> +  Task.spawn(function () {

As for |Promise|, I don't see where you import |Task|.

@@ +205,5 @@
> +    }
> +
> +    // sort results by frecency
> +    results.sort(function(a,b) a[1] - b[1]);
> +    results.reverse();

I realize that this was in the initial implementation, but why not reverse in the above function, e.g. by writing |b[1] - a[1]|?

@@ +209,5 @@
> +    results.reverse();
> +    // Make sure there's enough results returned
> +    prefs.setIntPref("browser.urlbar.maxRichResults", results.length);
> +
> +    //results.every(function(el) { dump("result: " + el[1] + ": " + el[0].spec + " (" + el[2] + ")\n"); return true; })

I _guess_ you can dump this commented-out line.

@@ +211,5 @@
> +    prefs.setIntPref("browser.urlbar.maxRichResults", results.length);
> +
> +    //results.every(function(el) { dump("result: " + el[1] + ": " + el[0].spec + " (" + el[2] + ")\n"); return true; })
> +
> +    yield promiseAsyncUpdates();

Where's |promiseAsyncUpdates| defined?

::: toolkit/components/places/tests/unit/test_248970.js
@@ +44,5 @@
>                         "http://www.google.de/redirect-temporary/"];
>  /**
>   * Function fills history, one for each transition type.
>   */
> +function task_fill_history_visitedURI() {

As for other tasks, mention that this is meant to be used with Task.spawn.

@@ +160,5 @@
> +{
> +  run_next_test();
> +}
> +
> +add_test_task(function test_execute()

Where's |add_test_task| defined?

@@ +268,5 @@
> +    }
> +  });
> +
> +  Services.prefs.clearUserPref("browser.privatebrowsing.keep_current_session");
> +});

I have not checked all of this in detail, but this looks pretty mechanical, so I'll just take your word on it.

::: toolkit/components/places/tests/unit/test_317472.js
@@ -54,5 @@
>    do_check_eq(histsvc.getCharsetForURI(TEST_URI), charset);
>    // get charset from bookmarked page
>    do_check_eq(histsvc.getCharsetForURI(TEST_BOOKMARKED_URI), charset);
>  
> -  promiseClearHistory().then(continue_test);

Wait, there were already promises in the previous version? Or is this based on another patch? I'm lacking some context here.

Comment 6

[:Paolo Amadini]

(In reply to David Rajchenbach Teller from comment #5)
> ::: toolkit/components/places/tests/unit/test_317472.js
> @@ -54,5 @@
> >    do_check_eq(histsvc.getCharsetForURI(TEST_URI), charset);
> >    // get charset from bookmarked page
> >    do_check_eq(histsvc.getCharsetForURI(TEST_BOOKMARKED_URI), charset);
> >  
> > -  promiseClearHistory().then(continue_test);
> 
> Wait, there were already promises in the previous version? Or is this based
> on another patch? I'm lacking some context here.

This is part of a series of patches that are basically ordered by bug number,
the first being bug 775495 and the second bug 775580, all blocking bug 775489.

Most helper functions are defined in "head_common.js".

> addVisits does not return the promise, is this normal?

Since addVisits continues to behave like it did before, it just invokes the
callback, while promiseAddVisits is the version that returns the promise.

> ::: toolkit/components/places/tests/unit/test_000_frecency.js
> Given that most people are unfamiliary with Task.js, you should probably add
> at least some type documentation on this function (and explain somewhere
> that Task.spawn handles the waiting).

Yes, I think we should add comments in strategic positions, and also redirect
to the Task.jsm documentation where appropriate.

> @@ +205,5 @@
> > +    }
> > +
> > +    // sort results by frecency
> > +    results.sort(function(a,b) a[1] - b[1]);
> > +    results.reverse();
> 
> I realize that this was in the initial implementation, but why not reverse
> in the above function, e.g. by writing |b[1] - a[1]|?

Makes sense :-) I've not touched the code because I wanted this conversion to
be as mechanical as possible, but I can definitely fix instances like this.

Comment 7

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

(In reply to Paolo Amadini [:paolo] from comment #6)
> (In reply to David Rajchenbach Teller from comment #5)
> > ::: toolkit/components/places/tests/unit/test_317472.js
> > @@ -54,5 @@
> > >    do_check_eq(histsvc.getCharsetForURI(TEST_URI), charset);
> > >    // get charset from bookmarked page
> > >    do_check_eq(histsvc.getCharsetForURI(TEST_BOOKMARKED_URI), charset);
> > >  
> > > -  promiseClearHistory().then(continue_test);
> > 
> > Wait, there were already promises in the previous version? Or is this based
> > on another patch? I'm lacking some context here.
> 
> This is part of a series of patches that are basically ordered by bug number,
> the first being bug 775495 and the second bug 775580, all blocking bug
> 775489.
> 
> Most helper functions are defined in "head_common.js".

In that case, could you add some dependencies? This will make the reviewer's life easier. I suspect the dependencies will have to go back to the Task.jsm and Promise bugs.

Comment 8

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> In that case, could you add some dependencies? This will make the reviewer's
> life easier. I suspect the dependencies will have to go back to the Task.jsm
> and Promise bugs.

I've added a dependency chain that starts from Promise and Task and leads up to
this patch.

Comment 9

[:Paolo Amadini]

Attachment: Updated patch

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 683528 [details] [diff] [review]
Updated patch

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

As for the other bug, try run with multiple X results please :)

::: toolkit/components/places/tests/unit/test_000_frecency.js
@@ +129,5 @@
>        }
>        else
>          matchTitle = calculatedURI.spec.substr(calculatedURI.spec.lastIndexOf("/")+1);
> +      if (visitType) {
> +        yield promiseAddVisits({

I don't understand the if (visitType)... the previous code was not doing that, it seems to hide a bug in the test?

@@ +209,5 @@
> +    results.reverse();
> +    // Make sure there's enough results returned
> +    prefs.setIntPref("browser.urlbar.maxRichResults", results.length);
> +
> +    //results.every(function(el) { dump("result: " + el[1] + ": " + el[0].spec + " (" + el[2] + ")\n"); return true; })

add a // DEBUG above this, otherwise looks like a bug. I'm sure I wrote this lot of time ago

@@ +212,5 @@
> +
> +    //results.every(function(el) { dump("result: " + el[1] + ": " + el[0].spec + " (" + el[2] + ")\n"); return true; })
> +
> +    yield promiseAsyncUpdates();
> +  }).then(continue_test, do_report_unexpected_exception);

so why don't you move all of this into an add_task, as well as continue_test may be just added with add_task?

::: toolkit/components/places/tests/unit/test_454977.js
@@ +9,5 @@
>  
> +// Returns the Place ID corresponding to an added visit.
> +function task_add_visit(aURI, aVisitType) {
> +
> +  // Add the visit asynchronously, and save its visit ID.

nit: remove useless newline?

::: toolkit/components/places/tests/unit/test_asyncExecuteLegacyQueries.js
@@ +13,5 @@
> +    let options = PlacesUtils.history.getNewQueryOptions();
> +    options.sortingMode = Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING;
> +    let query = PlacesUtils.history.getNewQuery();
> +  
> +    PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)

trailing spaces above

::: toolkit/components/places/tests/unit/test_history.js
@@ +184,5 @@
>    do_check_true(root.childCount > 0);
>    root.containerOpen = false;
>  
> +  // We used to test bug 400544 (a referrer that is not in the DB gets added),
> +  // but the asynchronous API does not add referrers if not present.

I doubt anybody will ever read this comment... just remove it. Sad we lose a test but this doesn't change anything.

::: toolkit/components/places/tests/unit/test_history_observer.js
@@ +62,5 @@
>      do_check_true(aURI.equals(testuri));
>      do_check_true(aVisitID > 0);
>      do_check_eq(aTime, testtime);
> +    // TODO: updatePlaces cannot add a visit with session ID equal to 0.
> +    // do_check_eq(aSessionID, 0);

I'm not sure why this test was checking 0, I'd expect 1 probably... what do we get? I'd be fine just checking it's a number > 0

::: toolkit/components/places/tests/unit/test_history_removeAllPages.js
@@ +23,5 @@
> +      PlacesUtils.history.removeObserver(this, false);
> +      deferred.resolve();
> +    },
> +  
> +    QueryInterface: XPCOMUtils.generateQI([

there are various trailing spaces in this object

@@ +34,2 @@
>  
> +let promiseInit;

add comment on what this is used for

@@ +63,5 @@
> +  ]);
> +
> +  // Note: We used to add an invalid transtion, but this cannot be done anymore
> +  // with the asynchronous history API.
> +  // { uri: uri("http://invalid.mozilla.org/"), transition: 0 }

win win, get rid of this :)

@@ +96,3 @@
>  
> +  // Clear history and wait for the onClearHistory notification.
> +  let promiseWaitClearHistory = promiseOnClearHistoryObserved(); 

trailing space

::: toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js
@@ +154,5 @@
> +          }
> +        }, null);
> +        do_check_false(resultObserver.inBatchMode);
> +  
> +        // nsINavHistoryResultObserver.containerClosed

various trailing spaces were added in this test

::: toolkit/components/places/tests/unit/test_placesTxn.js
@@ +655,5 @@
> +  
> +    txn.redoTransaction();
> +    do_check_true(annosvc.pageHasAnnotation(testURI, TEST_ANNO));
> +  
> +    run_next_test();

trailing spaces in this test as well

::: toolkit/components/places/tests/unit/test_removeVisitsByTimeframe.js
@@ +72,5 @@
> +                             TEST_URI,
> +                             bmsvc.DEFAULT_INDEX,
> +                             "bookmark title");
> +  
> +        promiseAsyncUpdates().then(this.continue_run.bind(this));

trailing spaces

::: toolkit/components/places/tests/unit/test_result_sort.js
@@ +118,5 @@
> +      checkOrder(id2, id3, id1);
> +      result.sortingMode = NHQO.SORT_BY_FRECENCY_ASCENDING;
> +      checkOrder(id1, id3, id2);
> +  
> +      root.containerOpen = false;

trailing spaces

Comment 11

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #10)
> ::: toolkit/components/places/tests/unit/test_000_frecency.js
> @@ +129,5 @@
> >        }
> >        else
> >          matchTitle = calculatedURI.spec.substr(calculatedURI.spec.lastIndexOf("/")+1);
> > +      if (visitType) {
> > +        yield promiseAddVisits({
> 
> I don't understand the if (visitType)... the previous code was not doing
> that, it seems to hide a bug in the test?

AddVisits used to work with a transition type "0", updatePlaces doesn't. I'm not
sure what this test was meant to do, do you know something more?

Comment 12

[Marco Bonardo [:mak]]

I think transition 0 was a case supported originally in Places (before FF3), but fixed just after the first release, we likely don't care to test it.

Comment 13

[:Paolo Amadini]

Attachment: Revised patch

Comment 14

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94d11f7d9114

Comment 15

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/94d11f7d9114