Closed Bug 1095418 Opened 6 years ago Closed 5 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox39 --- fixed

People

(Reporter: mak, Assigned: ttaubert)

References

Details

Attachments

(1 file)

tests should be converted
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
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.
Attachment #8576646 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/82c30bfc45fd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.