Remove calls to addVisit from the "autocomplete" folder

RESOLVED FIXED in mozilla20

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
This removes more addVisit calls in Places tests.
(Assignee)

Comment 1

5 years ago
Created attachment 645236 [details] [diff] [review]
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.
Attachment #645236 - Flags: feedback?(mak77)

Updated

5 years ago
Blocks: 700250
(Assignee)

Updated

5 years ago
Attachment #645236 - Flags: feedback?(mak77) → feedback?(dteller)
Attachment #645236 - Flags: feedback?(dteller)
(Assignee)

Updated

5 years ago
Depends on: 776855
(Assignee)

Updated

5 years ago
Blocks: 776872
(Assignee)

Comment 2

5 years ago
Created attachment 683514 [details] [diff] [review]
Updated patch

This hasn't changed significantly since comment 1.
Attachment #645236 - Attachment is obsolete: true
Attachment #683514 - Flags: review?(mak77)
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

Updated

5 years ago
Attachment #683514 - Flags: review?(mak77)
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
Created attachment 687068 [details] [diff] [review]
Revised patch
Attachment #683514 - Attachment is obsolete: true
Attachment #687068 - Flags: review?(mak77)

Updated

5 years ago
Attachment #687068 - Flags: review?(mak77) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9459220c39
Target Milestone: --- → mozilla20

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6a9459220c39
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.