Closed Bug 1027246 Opened 8 years ago Closed 6 years ago

Use ES6 generators (function*) instead of old spidermonkey generators (without *)

Categories

(DevTools :: General, defect)

32 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 887895

People

(Reporter: lviknesh, Assigned: lviknesh, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36
Replace fuction with function* for generators in

1)http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/stylesheets.js

2)http://dxr.mozilla.org/mozilla-central/source/browser/devtools/projecteditor/lib/stores/local.js

3)http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shadereditor/shadereditor.js
Component: Untriaged → Developer Tools
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [good-first-bug][lang=js][mentor=fitzgen@mozilla.com]
Assignee: nobody → lviknesh
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: replace function with function* for generators → Use ES6 generators (function*) instead of old spidermonkey generators (without *)
Attached patch generators.patch (obsolete) — Splinter Review
Attachment #8442332 - Flags: review?(nfitzgerald)
Mentor: nfitzgerald
Whiteboard: [good-first-bug][lang=js][mentor=fitzgen@mozilla.com] → [good first bug][lang=js]
Comment on attachment 8442332 [details] [diff] [review]
generators.patch

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

Looks good :)

Try push: https://tbpl.mozilla.org/?tree=Try&rev=0b052237f30a
Attachment #8442332 - Flags: review?(nfitzgerald) → review+
That Try push looks really orange....
Keywords: checkin-needed
Vikneshwar, did you run the tests locally? Can you reproduce the failures? You likely have a syntax error or something.
Rebased patch, review carried forward
Attachment #8442332 - Attachment is obsolete: true
Attachment #8561115 - Flags: review+
Seemed a shame for vikneshwar's patch to go unused.  All it needed was the "throw new Task.Result()"s replacing with simple returns.
 	
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06ddec3c3a31
Attachment #8561118 - Flags: review?(nfitzgerald)
Attachment #8561118 - Flags: review?(nfitzgerald) → review+
Let's get this bit in before it bit-rots again.
https://hg.mozilla.org/integration/fx-team/rev/46cfe0a6aa6c
https://hg.mozilla.org/integration/fx-team/rev/f4240674fd6c
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
had to be backedout first for the suspicion of causing a dt test bustage but turned out to be innocent and so relanded as

remote:   https://hg.mozilla.org/integration/fx-team/rev/087d9b31dca4
remote:   https://hg.mozilla.org/integration/fx-team/rev/cc4794a3d869
https://hg.mozilla.org/mozilla-central/rev/087d9b31dca4
https://hg.mozilla.org/mozilla-central/rev/cc4794a3d869
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
I think this can be closed, yeah?
Flags: needinfo?(nfitzgerald)
I think we still haven't removed all the old generators, so I think it should stay open.
Flags: needinfo?(nfitzgerald)
I believe we completed this in bug 887895 as part of starting to use ESLint, since it failed to parse functions with yield without *.

Patrick should know for sure.
Flags: needinfo?(pbrosset)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15)
> I believe we completed this in bug 887895 as part of starting to use ESLint,
> since it failed to parse functions with yield without *.
Yes we did, so we should close this bug I believe.
The thing is we're, as of now, running eslint automatically as part of a build process, so nothing prevents people from re-introducing spidermonkey-only syntax (and in fact this has happened already a couple of times), and this usually goes un-noticed at review time.
However, because a lot of us run eslint as part of our editor, this is usually discovered pretty quickly and fixed.
Flags: needinfo?(pbrosset)
we're *not* running eslint automatically is what I meant to say.
Okay, then I believe we can consider this bug completed by bug 887895.

Even though it's still possible to add one again, we at least removed them all once, and that was task this bug aimed for.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 887895
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.