820764 - Stop using addvisit() in browser tests

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

The following tests are still using addVisit:

browser/components/privatebrowsing/test/browser/obsolete
browser/components/places/tests/browser/browser_library_infoBox.js
browser/components/places/tests/browser/browser_sidebarpanels_click.js
browser/components/places/tests/browser/browser_history_sidebar_search.js
browser/components/places/tests/browser/browser_library_panel_leak.js
browser/components/places/tests/browser/browser_library_downloads.js
browser/components/places/tests/browser/browser_library_search.js
browser/components/places/tests/browser/browser_410196_paste_into_tags.js
browser/components/places/tests/browser/browser_forgetthissite_single.js
browser/components/places/tests/browser/browser_library_left_pane_commands.js
browser/components/places/tests/browser/browser_bookmarksProperties.js
browser/components/places/tests/chrome/test_bug549192.xul
browser/components/places/tests/chrome/test_treeview_date.xul
browser/components/places/tests/chrome/test_bug549491.xul
browser/components/places/tests/unit/test_clearHistory_shutdown.js
browser/components/preferences/tests/browser_chunk_permissions.js
browser/components/preferences/tests/browser_permissions.js
browser/components/thumbnails/test/browser_thumbnails_storage.js
browser/base/content/test/browser_sanitizeDialog.js
browser/base/content/test/browser_sanitizeDialog_treeView.js

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Looks good on try so far
https://tbpl.mozilla.org/?tree=Try&rev=c7d6a8acfc6c

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 705294 [details] [diff] [review]
v1

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

lots of changes!
nothing blocking, just some comments

::: browser/base/content/test/browser_sanitizeDialog.js
@@ +59,3 @@
>      for (let i = 0; i < 30; i++) {
> +      pURI = makeURI("http://" + i + "-minutes-ago.com/");
> +      places.push({uri: pURI, visitDate: now_uSec - i * kUsecPerMin});

I'd prefer if you could retain a visitTimeForMinutesAgo helper along all of the test, it's easier to read visitTimeForMinutesAgo(i) than now_uSec - i * kUsecPerMin, that just have to return a visit time instead of the whole visit like it was doing before

::: browser/base/content/test/browser_sanitizeDialog_treeView.js
@@ +40,3 @@
>      for (let i = 0; i < 30; i++) {
> +      pURI = makeURI("http://" + i + "-minutes-ago.com/");
> +      places.push({uri: pURI, visitDate: now_uSec -(i * 60 * 1000 * 1000)});

ditto also for this test

::: browser/components/places/tests/browser/browser_410196_paste_into_tags.js
@@ +39,3 @@
>                     tests.copyHistNode,
>                     onClipboardReady,
>                     PlacesUtils.TYPE_X_MOZ_PLACE);

please fix indentation

@@ +66,5 @@
>      // need to add a history object
>      let testURI1 = NetUtil.newURI(MOZURISPEC);
>      isnot(testURI1, null, "testURI is not null");
> +    addVisits(
> +      {uri: testURI1, transition: PlacesUtils.history.TRANSITION_TYPED}, 

trailing space

::: browser/components/places/tests/browser/browser_bookmarksProperties.js
@@ +421,5 @@
> +      window,
> +      function() {
> +        // Sanity check.
> +        var gh = PlacesUtils.history.QueryInterface(Ci.nsIGlobalHistory2);
> +        ok(gh.isVisited(PlacesUtils._uri(TEST_URL)), TEST_URL + " is a visited url.");

we should remove this and instead pass success/failure to the addVisits callback and check success here... if possible.

::: browser/components/places/tests/browser/browser_forgetthissite_single.js
@@ +17,3 @@
>    TEST_URIs.forEach(function(TEST_URI) {
> +    addVisits(
> +      {uri: PlacesUtils._uri(TEST_URI), 

trailing space

@@ +19,5 @@
> +      {uri: PlacesUtils._uri(TEST_URI), 
> +        transition: PlacesUtils.history.TRANSITION_TYPED},
> +      window,
> +      function() {
> +        if (++callbackCount == uriCount) {

wouldn't be simpler to pass an array of places to addVisits and get the completion callback just once?

::: browser/components/places/tests/browser/browser_history_sidebar_search.js
@@ +44,5 @@
> +      {uri: uri(pages[i]), visitDate: (time - i) * 1000,
> +        transition: hs.TRANSITION_TYPED},
> +      window,
> +      function() {
> +        if (++callbackCount == pagesLength)

ditto

::: browser/components/places/tests/browser/browser_library_infoBox.js
@@ +26,5 @@
> +      {uri: PlacesUtils._uri(TEST_URI), visitDate: Date.now() * 1000,
> +        transition: PlacesUtils.history.TRANSITION_TYPED},
> +      window,
> +      function() {
> +        var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory);

instead of reindenting everything you may probably split the original code to a new method in this object, and call that method as the addVisits callback (generally better to avoid indentation changes if possible, so we preserve hg blame)

::: browser/components/places/tests/browser/browser_library_left_pane_commands.js
@@ +22,5 @@
> +      {uri: PlacesUtils._uri(TEST_URI), visitDate: Date.now() * 1000,
> +        transition: PlacesUtils.history.TRANSITION_TYPED},
> +      window,
> +      function() {
> +        var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory);

ditto

::: browser/components/places/tests/browser/browser_library_search.js
@@ +177,5 @@
> +      addVisits(
> +        {uri: PlacesUtils._uri(TEST_DOWNLOAD_URL), visitDate: Date.now() * 1000,
> +          transition: PlacesUtils.history.TRANSITION_DOWNLOAD},
> +        window,
> +        function() {

these can be merged to a single addVisits call, I think

::: browser/components/places/tests/browser/browser_sidebarpanels_click.js
@@ +57,5 @@
> +        {uri: uri, visitDate: Date.now() * 1000,
> +          transition: PlacesUtils.history.TRANSITION_TYPED},
> +        window,
> +        function() {
> +          ok(PlacesUtils.ghistory2.isVisited(uri), "Item is visited");

as above, would be better to pass a success/failure and check that, rather than checking isVisited

::: browser/components/places/tests/chrome/Makefile.in
@@ +10,5 @@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
>  MOCHITEST_CHROME_FILES = \
> +	test_common.js \

I'd be fine to call this head.js looks like we do the same in other mochitest-chrome folders (even if it's not imported automatically)

::: browser/components/places/tests/chrome/test_bug549192.xul
@@ +58,5 @@
> +           visitDate: vtime, transition: ttype},
> +         {uri: Services.io.newURI("http://example2.tld/", null, null),
> +           visitDate: vtime++, transition: ttype},
> +         {uri: Services.io.newURI("http://example3.tld/", null, null),
> +           visitDate: vtime++, transition: ttype}], 

trailing space

::: browser/components/places/tests/chrome/test_treeview_date.xul
@@ +77,5 @@
>        midnight.setMinutes(0);
>        midnight.setSeconds(0);
>        midnight.setMilliseconds(0);
>  
> +      // Add a visit 1ms before midnight, a visit at midnight, and 

trailing space

@@ +94,5 @@
> +          // add a bookmark to the midnight visit
> +          var itemId = bs.insertBookmark(bs.toolbarFolder,
> +                                         uri("http://at.midnight.com/"),
> +                                         bs.DEFAULT_INDEX,
> +                                         "A bookmark at midnight");

if possible please avoid reindentation by splitting to a new function

@@ +153,5 @@
> +                  is(text, timeStr, "Date format is correct");
> +                  break;
> +                case "visitCount":
> +                  is(text, 1, "Visit count is correct");
> +                  break;         

trailing spaces

::: browser/components/places/tests/unit/head_bookmarks.js
@@ +134,5 @@
> +    }
> +  );
> +
> +  return deferred.promise;
> +}

there should be no need to change this head_bookmarks.js file at all, head_common.js in toolkit/components/places/tests/ is also imported in browser/components/places/tests/ with a small make trick :)
Whatever is in head_common will also be visible in these tests.

::: browser/components/privatebrowsing/test/browser/obsolete/browser_privatebrowsing_forgetthissite.js
@@ +43,5 @@
> +  PlacesUtils.asyncHistory.updatePlaces(place, {
> +    handleError: function () ok(false, "couldn't add visit"),
> +    handleResult: function () {},
> +    handleCompletion: function () {
> +      ok(true, TEST_URI + " successfully marked visited");

if possible check success/failure, as said.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Updated the patch based on comments

Pushed to try and looks good so far
https://tbpl.mozilla.org/?tree=Try&rev=3adaa23fc3a9

Comment 4

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8edae7afe6b0

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8edae7afe6b0

Comment 6

[Frank Wein [:mcsmurf]]

Raymond: I have a question on the patch, why did you use aWindow.PlacesUtils instead of PlacesUtils directly (like the code at http://hg.mozilla.org/mozilla-central/annotate/fa5d5fccbc11/browser/components/places/tests/browser/head.js#l68 in the same file)? Just wondering as that question came up when I copied your code ;)

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to Frank Wein [:mcsmurf] from comment #6)
> Raymond: I have a question on the patch, why did you use aWindow.PlacesUtils
> instead of PlacesUtils directly (like the code at
> http://hg.mozilla.org/mozilla-central/annotate/fa5d5fccbc11/browser/
> components/places/tests/browser/head.js#l68 in the same file)? Just
> wondering as that question came up when I copied your code ;)

It doesn't have to use aWindow as PlacesUtils.asyncHistory is using mozIAsyncHistory interface and it's not window specified.  I remember that I also copied this method from toolkit/components/places/tests/browser/head.js. If you want to create your own method, you don't really need aWindow part