728143 - Replace old synchronous favicons calls in Places tests

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[:Paolo Amadini]

See bug 713642 for background.

Comment 1

[:Paolo Amadini]

Attachment: Work in progress

I started with the favicons tests in Places. Things done until now:

- Moved the 25 test and data files to their own folder. The tests can now be
  run independently with this command:

  SOLO_FILE="favicons" make -C toolkit/components/places/tests/ check-one

- Moved one common function to the new head_favicons.js.

- Rewrote "test_doSetAndLoadFaviconForPage_failures.js". The original test had
  a hard-coded pause of 500ms per test case, which is now unnecessary because
  of how the functions are implemented. Now the test has the same structure as
  "test_download_history.js". Faster tests and less chances of random orange.

- Simplified "test_history_observer.js" a bit as a consequence of the move.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 598671 [details] [diff] [review]
Work in progress

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

yes, I like the fact you moved the tests to a separate folder

::: toolkit/components/places/tests/unit/head_bookmarks.js
@@ +53,5 @@
> +
> +// This error icon must stay in sync with FAVICON_ERRORPAGE_URL in
> +// nsIFaviconService.idl, aboutCertError.xhtml and netError.xhtml.
> +const FAVICON_ERRORPAGE_URL = "chrome://global/skin/icons/warning-16.png";
> +const FAVICON_ERRORPAGE_URI = NetUtil.newURI(FAVICON_ERRORPAGE_URL);

do we really need both, can we just provide the nsIURI and use .spec?

@@ +57,5 @@
> +const FAVICON_ERRORPAGE_URI = NetUtil.newURI(FAVICON_ERRORPAGE_URL);
> +
> +// TODO: Make async (replace call with getFaviconDataForPage),
> +//       rename to "checkFaviconDataForPage"
> +function checkAddSucceeded(pageURI, mimetype, data) {

and a callback :)

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js
@@ +18,5 @@
> + */
> +function setFavicon(aPageURI, aFaviconURI)
> +{
> +  PlacesUtils.favicons.setAndFetchFaviconForPage(aPageURI, aFaviconURI, true);
> +}

this hides what the logic imo

@@ +25,5 @@
> + * Listens for the last and only onPageChanged notification we should receive.
> + */
> +let historyObserver = {
> +  __proto__: NavHistoryObserver.prototype,
> +  onPageChanged: function HO_onPageChanged(aURI, aWhat, aValue, aGUID) {

maybe this could be incorporated into a check function helper in head, sounds like useful for more than one test.

::: toolkit/components/places/tests/unit/test_history_observer.js
@@ +147,4 @@
>  
> +  PlacesUtils.favicons.setAndFetchFaviconForPage(testuri,
> +                                                 NetUtil.newURI(SMALLPNG),
> +                                                 false, null);

doesn't this change the scope of the test by testing a datauri instead of an actual file? sure this is not the primary scope of this test. It's fine provided the file path is tested elsewhere in the favicons tests.

Comment 3

[:Paolo Amadini]

Marco, is there any way to remove the timeout in this test that you know of?

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/tests/unit/test_451499.js#127

By the way, this seems a test for bookmarks code, and can be implemented using the
data: URI as in "test_history_observer.js". Maybe it can also be moved to the
bookmarks test folder?

Comment 4

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini from comment #3)
> Marco, is there any way to remove the timeout in this test that you know of?
> 
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/
> tests/unit/test_451499.js#127

Actually, that exists because in the past we added visits and favicon with a lazy 3s timer instead of using async Storage (didn't exist yet). You should absolutely be able of getting rid of it when converting to the new api.

> By the way, this seems a test for bookmarks code, and can be implemented
> using the
> data: URI as in "test_history_observer.js". Maybe it can also be moved to the
> bookmarks test folder?

this looks like a test checking live update of the icon in a result, not exactly specific to bookmarks service, so I don't think makes much sense to move it there.

Comment 5

[:Paolo Amadini]

Attachment: Work in progress, continued

While working on it, it turned out that test_451499.js wasn't actually testing
anything, because the name of the notification method changed meanwhile. I had
to rewrite using the asynchronous API, and now it fails as expected when
removing the fix in bug 451499.

I'm also done converting many other tests to the asynchronous API.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 600691 [details] [diff] [review]
Work in progress, continued

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

it still looks fine.
so, in the end, test_451499.js was correct, just that it was testing in a parallel dimension.

::: toolkit/components/places/tests/unit/head_bookmarks.js
@@ +63,5 @@
> + *
> + * @param aCallback
> + *        This function is called with the same arguments of onPageChanged.
> + */
> +function waitForFaviconChanged(aCallback) {

I think this should also get a uri argument and either:
- get a uri and check to return the notification for that uri
- throw loudly if it gets a notification for another page

this may save us from some orange on unexpected ordering of events.

::: toolkit/components/places/tests/unit/test_doReplaceFaviconDataFromDataURL.js
@@ +134,5 @@
> +        pageURI, firstFavicon.mimetype, firstFavicon.data,
> +        function test_replaceFaviconDataFromDataURL_replaceExisting_firstCallback() {
> +          iconsvc.replaceFaviconDataFromDataURL(firstFavicon.uri, createDataURLForFavicon(secondFavicon));
> +          waitForAsyncUpdates(function() {
> +            checkFaviconDataForPage(

Do you actually need to wait here?
checkFaviconDataForPage is async and serialized to any previous async change

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
@@ +42,5 @@
> +                            run_next_test);
> +  });
> +
> +  PlacesUtils.bookmarks.insertBookmark(
> +                        PlacesUtils.bookmarks.unfiledBookmarksFolder, pageURI,

never use direct getters from the service, always use PlacesUtils getters like PlacesUtils.unfiledBookmarksFolderId.
please fix cases you may hit while doing the move

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js
@@ +30,5 @@
> +    stmt.executeAsync({
> +      handleResult: function(aResultSet) {
> +        let row;
> +        while (row = aResultSet.getNextRow())
> +        {

use the for() version, and don't newline the brace in loops

Comment 7

[:Paolo Amadini]

Attachment: The patch

This should be the complete asynchronous conversion of the Toolkit favicon
tests. The new asynchronous tests can be a good reference for updating other
uses of the synchronous API in the codebase.

I left out a few synchronous calls in an existing test, that apparently is
checking the interactions between the two APIs. I also removed a bit of code
in the queries tests that was unused, and is superseded by the favicon tests.

Comment 8

[:Paolo Amadini]

Attachment: The patch

Fixed one test that still referenced a moved file.

Comment 9

[:Paolo Amadini]

Tryserver build looks successful:

https://tbpl.mozilla.org/?tree=Try&rev=30c4ef78125a

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 601911 [details] [diff] [review]
The patch

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

almost there!

::: toolkit/components/places/tests/unit/head_bookmarks.js
@@ +53,5 @@
> +
> +// This error icon must stay in sync with FAVICON_ERRORPAGE_URL in
> +// nsIFaviconService.idl, aboutCertError.xhtml and netError.xhtml.
> +const FAVICON_ERRORPAGE_URI =
> +      NetUtil.newURI("chrome://global/skin/icons/warning-16.png");

nit: indent just 2 spaces

@@ +109,5 @@
> +      do_check_true(compareArrays(aExpectedData, aData));
> +      do_check_guid_for_uri(aPageURI);
> +      aCallback();
> +    }
> +  });

this is overkill, may just be a function

@@ +127,5 @@
> +  PlacesUtils.favicons.getFaviconURLForPage(aPageURI, {
> +    onFaviconDataAvailable: function CFMFP_onFaviconDataAvailable() {
> +      notificationReceived = true;
> +    }
> +  });

this is overkill, may just be a function

@@ +130,5 @@
> +    }
> +  });
> +  // Wait enough cycles for the notification to be processed.
> +  waitForAsyncUpdates(function CFMFP_asyncUpdates() {
> +    do_execute_soon(function CFMFP_soon() {

this looks "random", may we wait for something more meaningful?

::: toolkit/components/places/tests/favicons/test_expireAllFavicons.js
@@ +20,5 @@
> +
> +add_test(function test_expireAllFavicons() {
> +  // Set up an observer to wait for favicons expiration to finish.
> +  Services.obs.addObserver(function EAF_observer(aSubject, aTopic, aData) {
> +    Services.obs.removeObserver(arguments.callee, aTopic);

don't use arguments.callee

@@ +31,5 @@
> +    });
> +  }, PlacesUtils.TOPIC_FAVICONS_EXPIRED, false);
> +
> +  // Add a visited page.
> +  let visitId = PlacesUtils.history.addVisit(

use updatePlaces instead

::: toolkit/components/places/tests/unit/test_favicons.js
@@ +87,3 @@
>      },
>      QueryInterface: XPCOMUtils.generateQI([Ci.nsIFaviconDataCallback])
>    });

this is overkill, as well, may just be a function

@@ +143,5 @@
> +      do_check_eq(aDataLen, aData.length);
> +      run_next_test();
> +    },
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIFaviconDataCallback])
> +  });

this is overkill, may just be a function

@@ +281,5 @@
>  
> +    let forceReload = false;
> +    do_log_info("Asynchronously setting page favicon.");
> +    PlacesUtils.favicons.setAndFetchFaviconForPage(
> +      pageURI, iconURI, forceReload, faviconDataCallback

no reason to assign false to forceReload, just pass it (yes I know this was existing code)

::: toolkit/components/places/tests/favicons/test_favicons_conversions.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

you can't convert code from MPL1 to PD without consent of each author, even if it's a test, though you can convert it to MPL2.

@@ +40,5 @@
> +  PlacesUtils.favicons.replaceFaviconData(faviconURI, fileData, fileData.length,
> +                                          aFileMimeType);
> +  PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, faviconURI, true, {
> +    onFaviconDataAvailable: function CFDC_verify(aURI, aDataLen, aData,
> +                                                 aMimeType) {

the callback interface has the function annotation, so you don't need an object

::: toolkit/components/places/tests/favicons/test_query_result_favicon_changed_on_child.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

you can't convert code from MPL1 to PD without consent of each author, even if it's a test, though you can convert it to MPL2.

@@ +25,5 @@
> +  let testBookmark = PlacesUtils.bookmarks.insertBookmark(
> +                                           PlacesUtils.bookmarksMenuFolderId,
> +                                           PAGE_URI,
> +                                           PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                           "test_bookmark");

nit: indent 2 more spaces, or just 2.

@@ +46,5 @@
> +               " not for the containing query.");
> +    },
> +    containerStateChanged: function(){},
> +    invalidateContainer: function(){}
> +  }

I think xpcom will complain since this observer is missing a bunch of methods... please add them (should be nsINavHistoryResultObserver)

@@ +53,5 @@
> +  waitForFaviconChanged(PAGE_URI, SMALLPNG_DATA_URI,
> +                        function QRFCOC_faviconChanged() {
> +    // Give the notifications some more cycles to be processed, to ensure that
> +    // nodeIconChanged is not invoked in the meantime.
> +    waitForAsyncUpdates(function QRFCOC_asyncUpdates() {

I'm not sure what this comment means

@@ +54,5 @@
> +                        function QRFCOC_faviconChanged() {
> +    // Give the notifications some more cycles to be processed, to ensure that
> +    // nodeIconChanged is not invoked in the meantime.
> +    waitForAsyncUpdates(function QRFCOC_asyncUpdates() {
> +      do_execute_soon(function QRFCOC_soon() {

this looks overkill... too much async. what are we waiting for exactly?

@@ +69,5 @@
> +  // the page must have data associated with it in order for the icon changed
> +  // notifications to be sent, so we use a valid image data URI.
> +  result.root.containerOpen = true;
> +  PlacesUtils.favicons.setAndFetchFaviconForPage(PAGE_URI, SMALLPNG_DATA_URI,
> +                                                 false, null);

just in case, do this in containerStateChanged when status is open.
the callback argument is optional, no need for null.

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

you can't convert code from MPL1 to PD without consent of each author, even if it's a test, though you can convert it to MPL2.

@@ +45,5 @@
> +  });
> +
> +  PlacesUtils.bookmarks.insertBookmark(
> +                        PlacesUtils.unfiledBookmarksFolderId, pageURI,
> +                        PlacesUtils.bookmarks.DEFAULT_INDEX, pageURI.spec);

nit: indent 2 spaces more, or just 2.

@@ +53,5 @@
> +add_test(function test_privateBrowsing_bookmarked()
> +{
> +  if (!"@mozilla.org/privatebrowsing;1" in Cc) {
> +    return;
> +  }

shouldn't this rather run_next_next()?

@@ +71,5 @@
> +  pb.privateBrowsingEnabled = true;
> +
> +  PlacesUtils.bookmarks.insertBookmark(
> +                        PlacesUtils.unfiledBookmarksFolderId, pageURI,
> +                        PlacesUtils.bookmarks.DEFAULT_INDEX, pageURI.spec);

nit: ditto on indent

@@ +94,5 @@
> +  Services.prefs.setBoolPref("places.history.enabled", false);
> +
> +  PlacesUtils.bookmarks.insertBookmark(
> +                        PlacesUtils.unfiledBookmarksFolderId, pageURI,
> +                        PlacesUtils.bookmarks.DEFAULT_INDEX, pageURI.spec);

nit: ditto on indent

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

you can't convert code from MPL1 to PD without consent of each author, even if it's a test, though you can convert it to MPL2.

@@ +19,5 @@
> +function run_test()
> +{
> +  // We run all the tests that follow, but only the last one should raise the
> +  // onPageChanged notification.
> +  waitForFaviconChanged(LAST_PAGE_URI, LAST_FAVICON_URI,

the comment talks about onPageChanged (correctly) but the relation with the following waitForFaviconChanged is weak.
Maybe "We run all the tests that follow, but only the last one should raise the onPageChanged notification, executing the waitForFaviconChanged callback."

@@ +87,5 @@
> +add_test(function test_privateBrowsing_nonBookmarkedURI()
> +{
> +  if (!"@mozilla.org/privatebrowsing;1" in Cc) {
> +    return;
> +  }

shouldn't this rather run_next_test()?

@@ +137,5 @@
> +  // notification to be sent.
> +  PlacesUtils.favicons.setAndFetchFaviconForPage(
> +                       LAST_PAGE_URI,
> +                       LAST_FAVICON_URI, true);
> +});

add a comment that this doesn't need to call run_next_test since will cause onPageChanged and enter waitForFaviconChange, bla bla.

::: toolkit/components/places/tests/unit/test_history_observer.js
@@ +142,5 @@
> +  // The new favicon for the page must have data associated with it in order to
> +  // receive the onPageChanged notification.  To keep this test self-contained,
> +  // we use an URI representing the smallest possible PNG file.
> +  PlacesUtils.favicons.setAndFetchFaviconForPage(testuri, SMALLPNG_DATA_URI,
> +                                                 false, null);

the callback param is optional, so you don't have to pass null

::: toolkit/components/places/tests/unit/test_preventive_maintenance.js
@@ +1214,5 @@
>      this._separatorId = bs.insertSeparator(bs.unfiledBookmarksFolder,
>                                             bs.DEFAULT_INDEX);
>      do_check_true(this._separatorId > 0);
>      ts.tagURI(this._uri1, ["testtag"]);
> +    fs.setAndFetchFaviconForPage(this._uri2, SMALLPNG_DATA_URI, true);

do you really need to set the third argument to true here?

@@ +1241,5 @@
> +      onFaviconDataAvailable: function AC_onFaviconDataAvailable(aURI) {
> +        do_check_true(aURI.equals(SMALLPNG_DATA_URI));
> +        aCallback();
> +      }
> +    });

nsIFaviconDataCallback has the function annotation, so you don't need to build an object.

Comment 11

[:Paolo Amadini]

Attachment: Revised patch

Revised patch.

(In reply to Marco Bonardo [:mak] from comment #10)
> you can't convert code from MPL1 to PD without consent of each author, even
> if it's a test, though you can convert it to MPL2.

Exactly, and for those tests whose header I changed to PD, I checked that the
copyright holder was Mozilla, that already gave consent for all tests where
there is no other copyright holder involved. Other tests on which you
commented were already in the public domain before I moved them.

Comment 12

[:Paolo Amadini]

The greenest tryrun ever :-)

https://tbpl.mozilla.org/?tree=Try&rev=0b05dd085ea8

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 606162 [details] [diff] [review]
Revised patch

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

::: toolkit/components/places/tests/unit/test_preventive_maintenance.js
@@ +1215,4 @@
>  
> +        if (tests.length) {
> +          current_test = tests.shift();
> +          dump("\nExecuting test: " + current_test.name + "\n" + "*** " + current_test.desc + "\n");

do_log_info please

Comment 14

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #13)
> do_log_info please

Pushed to inbound with this change:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c85ad7fd3e6

Comment 15

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/7c85ad7fd3e6

Comment 16

[Serge Gautherie (:sgautherie)]

Attachment: (AAv1) Fix check for "@mozilla.org/privatebrowsing;1" in 2 tests

To fix SeaMonkey perma-orange:
{
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/head.js | TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined - See following stack:

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/head.js | TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined - See following stack:
}

Comment 17

[:Paolo Amadini]

Comment on attachment 608234 [details] [diff] [review]
(AAv1) Fix check for "@mozilla.org/privatebrowsing;1" in 2 tests

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

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
@@ +52,5 @@
>  
>  add_test(function test_privateBrowsing_bookmarked()
>  {
> +  if (!("@mozilla.org/privatebrowsing;1" in Cc)) {
> +    todo(false, "PB service is not available, bail out of test_privateBrowsing_bookmarked()");

Thanks for the patch. Breaking comm-central on PB checks is not new, unfortunately. Sorry for that :-(

But I remember from bug 672681 comment 20 that the "todo" function is not available in Thunderbird xpcshell tests, so we should do without the explanatory line (unless things have changed meanwhile).

Comment 18

[Marco Bonardo [:mak]]

Paolo can review this trivial change, feel free to ask him for review once you answer his question.

Comment 19

[Mark Banner (:standard8)]

I've just landed a test-only bustage fix for the private browsing issue:

https://hg.mozilla.org/mozilla-central/rev/06a8c2979321

This just fixes the brackets and the broken function name, hence I thought reasonable for an r=bustage fix.

I didn't see the patch here as I was looking for follow-up bugs, not patches on existing bugs (IMO in theory a todo would be invalid for Thunderbird as I don't see the app getting private browsing at any stage, I could be wrong, but that's digressing anyway).

Comment 20

[Marco Bonardo [:mak]]

Thank you Mark, looks good.

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (AAv2) Document skipping "@mozilla.org/privatebrowsing;1" tests [Moved to bug 741175]

(In reply to Paolo Amadini [:paolo] from comment #17)
> "todo" function is not available in [...] xpcshell tests

Oh, right :-<

Comment 22

[:Paolo Amadini]

Comment on attachment 608593 [details] [diff] [review]
(AAv2) Document skipping "@mozilla.org/privatebrowsing;1" tests
[Moved to bug 741175]

Filed the separate bug 741175 to fix this and test_download_history.js also.
Thanks for your input on how to document the skipped tests in a compatible way.