778694 - Remove calls to addVisit from the "queries" folder

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Comment 1

[:Paolo Amadini]

Attachment: The patch

From the moment we migrated away from addVisit towards updatePlaces, we cannot
add visits in batch mode anymore.

The current, legacy test situation is that basically all the tests in the
"query" folder operate with a synchronous addVisist call in batch mode.
This is done for performance reasons thatdon't apply to updatePlaces
(because it creates its own database transaction, without the need to
create one explicitly using batch mode).

Since we are switching to updatePlaces in tests also, now visits are not added in batch more anymore. This is closer to the real situation now.

Some tests were intended to test batch mode explicitly, but this is not
supported by the new API, thus I converted them to be similar to all the
other tests.

Comment 2

[:Paolo Amadini]

Attachment: Updated patch

Comment 3

[Marco Bonardo [:mak]]

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

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

Not sure which of these tests I hate more :)

These are lots of changes, once you have all the patches updated I'd like to see a try run with multiple runs of xpcshell, to be sure we are not causing random failures in some test).

::: toolkit/components/places/tests/queries/head_queries.js
@@ +70,5 @@
> +            print("Error while setting visit_count.");
> +          }
> +          finally {
> +            stmt.finalize();
> +          }

could you please convert this to createAsyncStatement and executeAsync while here? one less synchronous query to handle later

@@ +74,5 @@
> +          }
> +        }
> +      }
> +
> +      // TODO: Verify if the comments below are still true in asynchronous mode.

it still has to be done asynchronously so it's properly serialized to whatever happens before it, the comment below should just be changed to

 // This must be async to properly enqueue after the updateFrecency call
 // done by the visit addition.

@@ +104,5 @@
> +          uri: uri(qdata.uri),
> +          visitDate: qdata.lastVisit,
> +          title: qdata.title
> +        });
> +      }

you know what? I think isDetails is now pointless... Previously addvisit was unable to set the title, so isDetails was doing that... But now it looks useless. I think we need a follow-up bug to replace isDetails with isVisit, maybe you could try if changing if (qdata.isVisit) to if (qdata.isVisit || qdata.isDetails) and see if tests pass. Then the follow-up may look into removing it completely.

::: toolkit/components/places/tests/queries/test_abstime-annotation-domain.js
@@ +209,4 @@
>    // Adds http://foo.com/changeme2 to the result set and removes foo.com/begin.html
> +  // This used to be done all in batch mode, but now we use the asynchronous
> +  // updatePlaces API to update visits, that is not compatible with batch mode
> +  // (besides, all the synchronous updates are already done in batch mode).

We are just losing a test here, so you can likely remove all of this.
The fact is this was testing notifications incoming to a query during batch mode, when most notifications are ignored, if you remove runInBatchMode it's the same as removing all of this sub test.

::: toolkit/components/places/tests/queries/test_abstime-annotation-uri.js
@@ +207,4 @@
>    // Adds http://foo.com/changeme2 to the result set and removes foo.com/begin.html
> +  // This used to be done all in batch mode, but now we use the asynchronous
> +  // updatePlaces API to update visits, that is not compatible with batch mode
> +  // (besides, all the synchronous updates are already done in batch mode).

ditto, you can just remove this subtest

::: toolkit/components/places/tests/queries/test_async.js
@@ +359,5 @@
>      print("------ Running test: " + test.desc);
>      test.run();
> +
> +    // Wait for the promise object that indicates that the next test can be run.
> +    yield test.deferNextTest.promise;

could run() return a promise?

::: toolkit/components/places/tests/queries/test_redirects.js
@@ +175,5 @@
>    return numProds;
>  }
>  
> +// Main
> +function run_test()

If you meet these "// Main" comments feel free to kick them out of the door

::: toolkit/components/places/tests/queries/test_results-as-tag-contents-query.js
@@ +105,5 @@
>  
> +  // Add one by adding a tag, remove one by removing search term.
> +  // This used to be done all in batch mode, but now we use the asynchronous
> +  // updatePlaces API to update visits, that is not compatible with batch mode
> +  // (besides, all the synchronous updates are already done in batch mode).

ditto

::: toolkit/components/places/tests/queries/test_results-as-visit.js
@@ +86,5 @@
> +   // Update some visits - add one and take one out of query set, and simply
> +   // change one so that it still applies to the query.
> +   // This used to be done all in batch mode, but now we use the asynchronous
> +   // updatePlaces API to update visits, that is not compatible with batch mode
> +   // (besides, all the synchronous updates are already done in batch mode).

ditto!

::: toolkit/components/places/tests/queries/test_searchterms-domain.js
@@ +104,5 @@
> +  // Add one and take one out of query set, and simply change one so that it
> +  // still applies to the query.
> +  // This used to be done all in batch mode, but now we use the asynchronous
> +  // updatePlaces API to update visits, that is not compatible with batch mode
> +  // (besides, all the synchronous updates are already done in batch mode).

and here

::: toolkit/components/places/tests/queries/test_searchterms-uri.js
@@ +100,5 @@
> +   // Add one and take one out of query set, and simply change one so that it
> +   // still applies to the query.
> +   // This used to be done all in batch mode, but now we use the asynchronous
> +   // updatePlaces API to update visits, that is not compatible with batch mode
> +   // (besides, all the synchronous updates are already done in batch mode).

and here

::: toolkit/components/places/tests/queries/test_sorting.js
@@ +899,5 @@
>        this._unsortedData[2],
>      ];
>  
>      // This function in head_queries.js creates our database with the above data
> +    yield task_populateDB(this._unsortedData);                  

trailing spaces

@@ +981,5 @@
>        this._unsortedData[2],
>      ];
>  
>      // This function in head_queries.js creates our database with the above data
> +    yield task_populateDB(this._unsortedData);                  

trailing spaces

@@ +1063,5 @@
>        this._unsortedData[2],
>      ];
>  
>      // This function in head_queries.js creates our database with the above data
> +    yield task_populateDB(this._unsortedData);                  

trailing spaces

@@ +1145,5 @@
>        this._unsortedData[2],
>      ];
>  
>      // This function in head_queries.js creates our database with the above data
> +    yield task_populateDB(this._unsortedData);                  

trailing spaces

@@ +1233,5 @@
>        this._unsortedData[5],
>      ];
>  
>      // This function in head_queries.js creates our database with the above data
> +    yield task_populateDB(this._unsortedData);                  

trailing spaces

@@ +1266,5 @@
> +{
> +  for (let [, test] in Iterator(tests)) {
> +    yield test.setup();
> +    yield promiseAsyncUpdates();
> +    yield test.check();

I didn't see any check() in need of async waiting... but the diff is not that clear, if it's synchronous I'd prefer to use it like so.

Comment 4

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #3)
> maybe you could try if changing if (qdata.isVisit) to if (qdata.isVisit ||
> qdata.isDetails) and see if tests pass. Then the follow-up may look into
> removing it completely.

It seems this doesn't work out of the box, filed bug 816925.

Comment 5

[:Paolo Amadini]

Attachment: Revised patch

Comment 6

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d63ccf27c5

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a1d63ccf27c5