1095418 - Convert xpcshell-tests in toolkit/components/places/tests/favicons to Bookmarks.jsm API

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

tests should be converted

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1095418-Convert-xpcshell-tests-in-toolkit-compon.patch

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8576646 [details] [diff] [review]
0001-Bug-1095418-Convert-xpcshell-tests-in-toolkit-compon.patch

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

::: toolkit/components/places/tests/favicons/head_favicons.js
@@ +95,5 @@
>        aCallback();
>      });
>  }
> +
> +function promiseCheckFaviconMissingForPage(aPageURI) {

I'd just name this promiseFaviconMissingForPage

@@ +99,5 @@
> +function promiseCheckFaviconMissingForPage(aPageURI) {
> +  return new Promise(resolve => checkFaviconMissingForPage(aPageURI, resolve));
> +}
> +
> +function promiseWaitForFaviconChanged(aExpectedPageURI, aExpectedFaviconURI) {

and PromiseFaviconChanged

::: toolkit/components/places/tests/favicons/test_expireAllFavicons.js
@@ +31,4 @@
>  });
> +
> +let expireAllFavicons = Task.async(function* () {
> +  let promise = promiseTopicObserved(PlacesUtils.TOPIC_FAVICONS_EXPIRED);

why did you split this test from the previous one? looks like the final expireAllFavicons() call there is a good hook for this already.
My fear is that here we're not expiring anything since the previous call already did. In future we might decide to not notify in this case...

::: toolkit/components/places/tests/favicons/test_query_result_favicon_changed_on_child.js
@@ +51,3 @@
>    // Open the container and wait for containerStateChanged.
>    result.root.containerOpen = true;
> +  yield promiseWaitForFaviconChanged(PAGE_URI, SMALLPNG_DATA_URI);

I'd prefer a clearer dependency here, this notification is caused by the setAndFetch call that happens with containerStateChanged. So I'd prefer if we'd start observing before the containerOpen call and just yield here.
Could even be worth a comment to clarify the order of events.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/82c30bfc45fd

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82c30bfc45fd