988341 - Convert code before switching Task.jsm to use Promise.jsm

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[:Paolo Amadini]

Attachment: The patch

This is a dependency of bug 887923, including the simpler code conversions.

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

Comment 1

[Marco Bonardo [:mak] (Away 10-21 August)]

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

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

::: browser/modules/test/browser_UITour.js
@@ +124,2 @@
>      gContentAPI.showMenu("appMenu");
> +    shownPromise.then(() => {

while I'd have preferred to change the test runner to run these as Task(s) (and thus save a lot of blame with a simple yield), I don't think I care that much. just move on.

::: browser/modules/test/head.js
@@ +82,5 @@
> +function promisePanelElementShown(win, aPanel) {
> +  let deferred = Promise.defer();
> +  let timeoutId = win.setTimeout(() => {
> +    deferred.reject("Panel did not show within 20 seconds.");
> +  }, 20000);

I'd probably reduce this, 5 seconds should be enough. 20 seconds is going too close to the test harness timeout, and tests are already taking too much, so if we find this taking more than 5 seconds we should seriously look at the code...

@@ +88,5 @@
> +    aPanel.removeEventListener("popupshown", onPanelOpen);
> +    win.clearTimeout(timeoutId);
> +    deferred.resolve();
> +  };
> +  aPanel.addEventListener("popupshown", onPanelOpen);

really nit: I'd probably inline the function here, to save a line of code

Comment 2

[:Paolo Amadini]

Attachment: Updated patch

Comment 3

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/76b762bfd5c6

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76b762bfd5c6