Closed
Bug 1329045
Opened 7 years ago
Closed 7 years ago
Rewrite mochitest test_use_with_hsts.html to use async/await
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(2 files)
The mochitest test_use_with_hsts.html does some loads inside of an iframe, and as a result, it involves some callback hell. (It's written as a chain of callbacks.) Now that we have async/await (in Firefox 52 according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function#Browser_compatibility ), we can rewrite this code to be more direct & avoid the ugly callback chaining.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
So this bug is basically condensing code patterns like the following: > function() Step1() { > iframe.onload = Step2; > iframe.src = someURI; > } > function() Step2() { > // inspect iframe > // ... > } ...to this: > await LoadIframeAsync(someURI); > // inspect iframe ...using my provided LoadIframeAsync helper and the magic of Async/Await. One note: some online sources recomment wrapping "await" expressions in try{}catch{}, since await will throw if the promise is rejected. I haven't bothered with that here, since: (a) the only promise in this test (the one returned by LoadIframeAsync) is never rejected under any conditions -- it's only resolved. (b) if it happens to be rejected, then an exception is fine -- it'll just trigger a test failure, which is what we want anyway. (If this were common then we could add try/catch for more graceful reporting, but I'm not expecting it to be possible in the first place.)
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8824268 [details] Bug 1329045 part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html. https://reviewboard.mozilla.org/r/102794/#review103248 ::: dom/svg/test/test_use_with_hsts.html:70 (Diff revision 1) > > - // TEST CODE BEGINS HERE. > - // Execution basically proceeds top-to-bottom through the functions > - // from this point on, via a chain of iframe onload-callbacks. > - function runTest() { > + // Convenience helper which makes |iframe| load the given |uri|. Returns > + // a promise that resolves when the load completes. This makes it handy to > + // use with 'await', to avoid onload callback hell. > + async function LoadIframeAsync(uri) { > + return new Promise(function(resolve, reject) { Instead of `function(resolve, reject) {`, you can probably just do `resolve => {` given you are not using `reject` at all. ::: dom/svg/test/test_use_with_hsts.html:72 (Diff revision 1) > + iframe.onload = function() { > + iframe.onload = null; // (only invoke this callback for this one load) > + resolve(); > + }; You can do `iframe.addEventListener('load', resolve, {once: true});` instead to avoid resetting the event handler. ::: dom/svg/test/test_use_with_hsts.html:110 (Diff revision 1) > - // Point iframe at insecure (HTTP) version of testcase (which should get > - // automatically upgraded to secure (HTTPS) under the hood), & wait for > + // Load insecure (http) version of testcase: > + await LoadIframeAsync(insecureURI); The piece of text inside the parens ("which should get ... under the hood") doesn't seem to related to this simplification, so it probably shouldn't be removed.
Attachment #8824268 -
Flags: review?(xidorn+moz) → review+
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8824269 [details] Bug 1329045 part 2: Convert some global variables to local variables, in test_use_with_hsts.html. https://reviewboard.mozilla.org/r/102796/#review103254
Attachment #8824269 -
Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8824268 [details] Bug 1329045 part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html. https://reviewboard.mozilla.org/r/102794/#review103248 > Instead of `function(resolve, reject) {`, you can probably just do `resolve => {` given you are not using `reject` at all. Done -- that does work & makes this more compact. Thanks! > You can do `iframe.addEventListener('load', resolve, {once: true});` instead to avoid resetting the event handler. Indeed! Thanks, fixed. > The piece of text inside the parens ("which should get ... under the hood") doesn't seem to related to this simplification, so it probably shouldn't be removed. Good point -- I've added it back. (I also reverted a few other needless wording tweaks, too.)
Assignee | ||
Comment 11•7 years ago
|
||
(Note: my "v2" of these patches (comment 6) included some bonus commit-message cruft left over from "qfold". I noticed that just after pushing, & cleaned it up in "v3".)
Comment 12•7 years ago
|
||
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aa001860e4ab part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html. r=xidorn https://hg.mozilla.org/integration/autoland/rev/1a3de277a1fc part 2: Convert some global variables to local variables, in test_use_with_hsts.html. r=xidorn
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aa001860e4ab https://hg.mozilla.org/mozilla-central/rev/1a3de277a1fc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•