Closed
Bug 1314176
Opened 8 years ago
Closed 7 years ago
convert AddonManager to ES6 classes
Categories
(Toolkit :: Add-ons Manager, defect, P3)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: rhelmer, Assigned: rhelmer)
References
Details
(Whiteboard: triaged)
Attachments
(1 file, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
Details |
The AddonManager code in toolkit/mozapps/extensions defines many custom objects using prototype-based OO, we could reduce the amount of code and make these more readable by using ES6 classes instead.
Assignee | ||
Comment 2•8 years ago
|
||
Converting to classes requires rewriting Task.async over to Task.span (plus binding (this)), so let's just convert over to async/await while we're at it.
Summary: convert AddonManager to ES6 classes → convert AddonManager to ES6 classes and Task.* to ES7 async/await
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Robert Helmer [:rhelmer] from comment #2) > Converting to classes requires rewriting Task.async over to Task.span Or Task.spawn, if you want it to actually work ;)
Comment hidden (mozreview-request) |
Comment 5•8 years ago
|
||
Just a heads up - I was asking around in #jsapi, and while async/await is enabled for all branches, they suggest we wait a cycle before doing mass conversions to let the feature percolate. Waldo said we should try to avoid a String.prototype.contains-type snafu.
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Mike Conley (:mconley) - (digging out of review / needinfo backlog) from comment #5) > Just a heads up - I was asking around in #jsapi, and while async/await is > enabled for all branches, they suggest we wait a cycle before doing mass > conversions to let the feature percolate. Waldo said we should try to avoid > a String.prototype.contains-type snafu. Makes sense, I can just keep a branch rebased and ready to go for when we feel comfortable.
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8806446 [details] Bug 1314176 - convert AddonManager from Task.{async,spawn} to async/await Actually don't bother reviewing this yet - I see some test failures, and I can simplify this further too.
Attachment #8806446 -
Flags: review?(kmaglione+bmo)
Comment 8•8 years ago
|
||
marking triaged and P3 - as rhelmer already took. priority could be changed - but setting removes from untriaged list.
Priority: -- → P3
Updated•8 years ago
|
Whiteboard: triaged
Assignee | ||
Comment 9•7 years ago
|
||
Since classes are ready to use now and we're holding off on async/await, let's just switch to classes now and do the Task.* -> async/await switch later.
No longer depends on: 1185106
Summary: convert AddonManager to ES6 classes and Task.* to ES7 async/await → convert AddonManager to ES6 classes
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8806446 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8810437 [details] Bug 1314176 - convert AddonManager to ES6 classes Removing r? while I work through some test failures.
Attachment #8810437 -
Flags: review?(aswan)
Assignee | ||
Comment 14•7 years ago
|
||
After looking at this more, I think it's probably not worth doing this as one big change. It'd make more sense to do things as-needed like bug 1314177, especially considering that we should be able to remove a fair amount of code post-57.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•