Closed Bug 1321216 Opened 4 years ago Closed 4 years ago

Remove legacy generator from devtools/.

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file, 2 obsolete files)

If the code fits into async function and also it's test code, replaced legacy generator with async function.
for other cases, just replaced legacy generator with ES6 generator.

some notes:

* devtools/client/canvasdebugger/test/browser_profiling-webgl.js
  |testFunctionCallTimestamp| contained unnecessary code that won't be executed.
  so just removed.

* devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-new-script.js
  Task.spawn does same effect as (async function() {...})().
  so replaced.

* devtools/client/debugger/test/mochitest/browser_dbg_host-layout.js
  |testHosts| partially does Task.spawn, that can also be done by
  just replacing whole function to async.

* devtools/client/storage/test/storage-listings.html
  yielding |cacheGenerator| has same effect as returning promise that is resolved
  when whole |cacheGenerator| and |fetchPut| are done,
  so replaced both to async.

* devtools/shared/qrcode/tests/mochitest/test_decode.html
  |Task|, |promise|, and |defer| are no more used
Attachment #8815631 - Flags: review?(jryans)
Comment on attachment 8815631 [details] [diff] [review]
Remove legacy generator from devtools/.

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

Thanks for working on this!  Looks close, but I am unsure about one part.

Also commit message needs bug number and reviewer.

::: devtools/shared/qrcode/tests/mochitest/test_decode.html
@@ +35,4 @@
>      result = QR.decodeFromCanvas(canvas);
>      is(result, "HELLO", "Decoded canvas result matches");
> +
> +    SimpleTest.finish(false);

I don't think `finish` takes any args.  Also, it looks like the previous version would always call `ok(false)` on error.  What's the equivalent for async / await code?
Attachment #8815631 - Flags: review?(jryans)
Thank you for reviewing :D

removed wrong |false| parameter, and also added try-catch with ok(false) in catch.
Attachment #8815631 - Attachment is obsolete: true
Attachment #8815921 - Flags: review?(jryans)
Comment on attachment 8815921 [details] [diff] [review]
Remove legacy generator from devtools/. r=jryans

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

Thanks for working on this clean up!

::: devtools/shared/qrcode/tests/mochitest/test_decode.html
@@ +39,5 @@
> +      SimpleTest.finish();
> +    } catch (e) {
> +      ok(false);
> +    }
> +  })();

Reading more about async / await, it seems the result of calling the async function is a promise, just like with `Task.spawn`.  Would it work to keep the `.then(..., ...)` from the previous version just after the `()` that calls the async function?

That removes the need for a `try` and the extra indentation, which seems nice to preserve.  (This is my first encounter with async / await code, so I haven't formed a strong opinion on style yet.)

If the `try` is required, we can go with that, but I'd like to know why for future cases!
Attachment #8815921 - Flags: review?(jryans) → review+
IMO, the merit of async/await is that we can write asynchronous code in synchronous way, without Promise chain.
so, I think try-catch is more intuitive than handling raw Promise by .then() or .catch()
(of course try is *not* required tho)
(In reply to Tooru Fujisawa [:arai] from comment #4)
> IMO, the merit of async/await is that we can write asynchronous code in
> synchronous way, without Promise chain.
> so, I think try-catch is more intuitive than handling raw Promise by .then()
> or .catch()
> (of course try is *not* required tho)

Overall, yes, the "await" feature (just like "yield" with Task.spawn) is a big improvement over .then() chains generally.

With error handling though, I am less sure what to prefer...  The try / catch version you posted would also have worked with Task.spawn, so it's not really a "new" thing enabled by async / await.

I guess for the purposes of this bug, probably making the smallest change is best, so let's keep the .then() if we can.  We've used that as a common pattern in DevTools with Task.spawn to catch rejections, and potentially we'd still like it with async / await (possibly, hasn't been discussed yet).  I'll keep thinking which style I want to use for new code! :)
okay, reverted |}).then(SimpleTest.finish, ok.bind(null, false));| line.
Attachment #8815921 - Attachment is obsolete: true
Attachment #8815947 - Flags: review?(jryans)
Comment on attachment 8815947 [details] [diff] [review]
Remove legacy generator from devtools/. r=jryans

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

Thanks!
Attachment #8815947 - Flags: review?(jryans) → review+
https://hg.mozilla.org/mozilla-central/rev/7dc7e58f62f4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
test-bug-632347-iterators-generators.html still has one legacy generator. Is this left intentionally?
Flags: needinfo?(arai.unmht)
(In reply to Masatoshi Kimura [:emk] from comment #10)
> test-bug-632347-iterators-generators.html still has one legacy generator. Is
> this left intentionally?

yes.  it's testing legacy generator itself,
and that should be kept until we actually remove legacy generator from JS engine.
Flags: needinfo?(arai.unmht)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.