776863 - Remove calls to addVisit from the "autocomplete" folder

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Paolo Amadini]

This removes more addVisit calls in Places tests.

Comment 1

[:Paolo Amadini]

Attachment: The patch

I made the framework in head_autocomplete.js asynchronous, so that the
individual tests could continue to work without modifications.

I added a title argument to markTyped because the "addVisits" helper doesn't
provide a way to keep the existing title for the page.

Comment 2

[:Paolo Amadini]

Attachment: Updated patch

This hasn't changed significantly since comment 1.

Comment 3

[Marco Bonardo [:mak]]

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

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

::: toolkit/components/places/tests/autocomplete/head_autocomplete.js
@@ +160,5 @@
>  // Store the page info for each uri
>  let gPages = [];
>  
> +// Initialization tasks to be run before the next test
> +let gInitTasks = [];

nit: gNextTestSetupTasks ? Init may be misinterpreted as global init tasks

@@ +265,5 @@
>  
> +  Task.spawn(function () {
> +    // Iterate over all tasks and execute them
> +    for (let [, [fn, args]] in Iterator(gInitTasks)) {
> +      yield fn.apply(this, args);

ok at this point I assume Task just continues if yield is not a promise

@@ +270,5 @@
> +    };
> +    
> +    // Clean up to allow tests to register more functions.
> +    gInitTasks = [];
> +  

trailing spaces before and after

@@ +304,5 @@
> +  for each (let uri in aURIs) {
> +    yield promiseAddVisits({
> +      uri: toURI(kURIs[uri]),
> +      transition: TRANSITION_TYPED,
> +      title: kTitles[aTitle]

it's unfortunate AddVisits makes up a title if we don't provide one.
IIRC, updatePlaces allows you to not pass a title, and if so keeps whatever is in the database, but AddVisits covers that... We could pass a special const for title KEEP_EXISTING_TITLE(-1) and AddVisit could just not pass a title at all if such value is found.  This should allow you to avoid having to modify the markTyped signature.

Also, don't use for each...in with arrays, use for...of

Comment 4

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #3)
> it's unfortunate AddVisits makes up a title if we don't provide one.
> IIRC, updatePlaces allows you to not pass a title, and if so keeps whatever
> is in the database, but AddVisits covers that... We could pass a special
> const for title KEEP_EXISTING_TITLE(-1) and AddVisit could just not pass a
> title at all if such value is found.  This should allow you to avoid having
> to modify the markTyped signature.

I don't remember whether the behavior of AddVisit and updatePlaces with regard to
titles was consistent, I remember that I tried the solution you suggest, and in
some tests I had to make changes anyway, so I went for specifying the title in
all the tests across all folders (except, of course, the tests for the behavior
of updatePlaces with empty titles, for which updatePlaces would be called
directly in any case, since it is the function being tested).

So, all the tests now provide titles. I don't think modifying the markTyped
signature here is a big deal, maybe it's also better.

Comment 5

[:Paolo Amadini]

Attachment: Revised patch

Comment 6

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9459220c39

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/6a9459220c39