Closed Bug 778699 Opened 8 years ago Closed 8 years ago

Remove calls to addVisit from the "unit" folder

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: Paolo, Assigned: Paolo)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

This removes more nested asynchronous addVisit calls.
Attached patch The patch (obsolete) — Splinter Review
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.
Attachment #647125 - Flags: feedback?(dteller)
Note that some tests from this folder are still missing, a new patch will follow.
Attached patch Remove all calls (obsolete) — Splinter Review
This removes all the synchronous calls, and works.

We can decide whether we want to do more cleanup on a case-by-case basis.
Attachment #647125 - Attachment is obsolete: true
Attachment #647125 - Flags: feedback?(dteller)
Attachment #647618 - Flags: feedback?(dteller)
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 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.
Attachment #647618 - Flags: feedback?(dteller)
(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.
(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.
Depends on: 778694
(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.
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #647618 - Attachment is obsolete: true
Attachment #683528 - Flags: review?(mak77)
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
Attachment #683528 - Flags: review?(mak77) → review+
(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?
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.
Attached patch Revised patchSplinter Review
Attachment #683528 - Attachment is obsolete: true
Attachment #687071 - Flags: feedback?(mak77)
Attachment #687071 - Flags: feedback?(mak77) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d11f7d9114
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/94d11f7d9114
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 821781
Depends on: 824074
You need to log in before you can comment on or make changes to this bug.