Closed Bug 1321229 Opened 3 years ago Closed 3 years ago

Remove legacy generator from toolkit/.

Categories

(Toolkit :: General, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file, 1 obsolete file)

just replaced legacy generator with ES6 generator.

some notes:

* toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
  |reference_compare_files| wasn't used

* toolkit/content/tests/chrome/test_arrowpanel.xul
  ES6 generator doesn't throw StopIteration, so we can just remove final yield
Attachment #8815644 - Flags: review?(dteller)
Comment on attachment 8815644 [details] [diff] [review]
Remove legacy generator from toolkit/.

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

r=me with a quick question

::: toolkit/content/tests/chrome/findbar_events_window.xul
@@ +64,5 @@
>      function* onDocumentLoaded() {
>        gFindBar.open();
>        gFindBar.onFindCommand();
>  
>        yield testFind();

Mmmhhh... why not `yield*` here, too?

::: toolkit/content/tests/chrome/test_arrowpanel.xul
@@ -210,5 @@
>      yield;
>    }
>  
>    SimpleTest.finish()
> -  yield;

That looks like a dirty hack. I assume you have tested that it still works without that hack.
Attachment #8815644 - Flags: review?(dteller) → review+
Thank you for reviewing :D

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #1)
> Comment on attachment 8815644 [details] [diff] [review]
> Remove legacy generator from toolkit/.
> 
> Review of attachment 8815644 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with a quick question
> 
> ::: toolkit/content/tests/chrome/findbar_events_window.xul
> @@ +64,5 @@
> >      function* onDocumentLoaded() {
> >        gFindBar.open();
> >        gFindBar.onFindCommand();
> >  
> >        yield testFind();
> 
> Mmmhhh... why not `yield*` here, too?

yeah, apparently we can use more `yield*` in the test.
(or maybe we can just use async)
will fix them too.


> ::: toolkit/content/tests/chrome/test_arrowpanel.xul
> @@ -210,5 @@
> >      yield;
> >    }
> >  
> >    SimpleTest.finish()
> > -  yield;
> 
> That looks like a dirty hack. I assume you have tested that it still works
> without that hack.

yes, legacy generator throws StopIteration without it.
the final yield was there to prevent it.
ES6 generator doesn't need it because it doesn't throw when returning.
Changed to use async functions in findbar_events_window.xul
Attachment #8815644 - Attachment is obsolete: true
Attachment #8815675 - Flags: review?(dteller)
Comment on attachment 8815675 [details] [diff] [review]
Remove legacy generator from toolkit/.

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

I haven't used await/async yet, so I can't review that part of the patch. But you have my r+ for the previous patch if you want to land it.
Attachment #8815675 - Flags: review?(dteller) → review+
okay, I'll use previous patch and leave async one to other bug :)
https://hg.mozilla.org/mozilla-central/rev/fc40e02bb78e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.