Closed
Bug 1168088
Opened 10 years ago
Closed 10 years ago
Remove SpiderMonkey specific JS syntax in toolkit/devtools
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: pbro, Assigned: tromey)
References
Details
Attachments
(1 file, 3 obsolete files)
88.69 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
There doesn't seem to be a whole lot of non-standard JS in toolkit/devtools, so it's ok to remove them all in one bug. Here's the list I found:
toolkit/devtools/DevToolsUtils.js
505:18 error Unexpected token if
toolkit/devtools/client/connection-manager.js
103:15 error Unexpected token for
toolkit/devtools/event-emitter.js
145:68 error Unexpected identifier
toolkit/devtools/gcli/commands/calllog.js
102:18 error Unexpected identifier
toolkit/devtools/gcli/commands/paintflashing.js
106:24 error Unexpected identifier
toolkit/devtools/gcli/commands/tools.js
24:18 error Unexpected identifier
toolkit/devtools/security/auth.js
397:13 error Unexpected token for
toolkit/devtools/security/tests/unit/testactors.js
49:38 error Unexpected token for
toolkit/devtools/server/actors/child-process.js
41:21 error Unexpected token true
toolkit/devtools/server/actors/common.js
283:9 error Unexpected identifier
toolkit/devtools/server/actors/director-manager.js
486:31 error Unexpected token for
toolkit/devtools/server/actors/gcli.js
263:22 error Unexpected identifier
toolkit/devtools/server/actors/highlighter.js
146:14 error Unexpected token this
toolkit/devtools/server/actors/inspector.js
216:14 error Unexpected token this
toolkit/devtools/server/actors/root.js
263:31 error Unexpected token for
toolkit/devtools/server/actors/script.js
501:18 error Unexpected token this
toolkit/devtools/server/actors/storage.js
888:27 error Unexpected token for
toolkit/devtools/server/actors/styleeditor.js
48:16 error Unexpected token this
toolkit/devtools/server/actors/styles.js
131:14 error Unexpected token this
toolkit/devtools/server/actors/stylesheets.js
52:16 error Unexpected token this
toolkit/devtools/server/actors/utils/TabSources.js
338:13 error Unexpected token for
toolkit/devtools/server/actors/webapps.js
568:15 error Unexpected identifier
toolkit/devtools/server/actors/webbrowser.js
88:10 error Illegal yield expression
toolkit/devtools/server/actors/webgl.js
123:21 error Unexpected token this
toolkit/devtools/server/main.js
50:20 error Unexpected identifier
toolkit/devtools/server/protocol.js
199:45 error Unexpected token for
toolkit/devtools/server/tests/mochitest/inspector-helpers.js
128:64 error Unexpected token for
toolkit/devtools/server/tests/unit/test_frameactor-04.js
65:11 error Unexpected identifier
toolkit/devtools/server/tests/unit/test_frameactor-05.js
60:39 error Unexpected token for
toolkit/devtools/server/tests/unit/test_protocol_children.js
32:24 error Unexpected string
toolkit/devtools/server/tests/unit/testactors.js
50:38 error Unexpected token for
toolkit/devtools/styleinspector/css-logic.js
232:11 error Unexpected identifier
toolkit/devtools/transport/stream-utils.js
120:15 error Unexpected token if
toolkit/devtools/transport/tests/unit/test_bulk_error.js
18:11 error Unexpected identifier
toolkit/devtools/transport/tests/unit/test_client_server_bulk.js
19:11 error Unexpected identifier
toolkit/devtools/transport/tests/unit/test_dbgsocket.js
91:13 error Unexpected token if
toolkit/devtools/transport/tests/unit/test_delimited_read.js
11:11 error Unexpected identifier
toolkit/devtools/transport/tests/unit/test_no_bulk.js
19:11 error Unexpected identifier
toolkit/devtools/transport/tests/unit/test_queue.js
18:11 error Unexpected identifier
toolkit/devtools/transport/tests/unit/test_transport_bulk.js
14:11 error Unexpected identifier
toolkit/devtools/transport/tests/unit/testactors.js
49:38 error Unexpected token for
toolkit/devtools/transport/transport.js
277:15 error Unexpected token if
toolkit/devtools/webconsole/network-monitor.js
98:15 error Unexpected token if
toolkit/devtools/webconsole/test/common.js
205:25 error Illegal yield expression
toolkit/devtools/webconsole/utils.js
296:17 error Unexpected token if
Reporter | ||
Comment 1•10 years ago
|
||
This isn't all of them yet, but I've been removing spidermonkey specific js from all over the devtools code for 2 days, I'm sick of it now. If someone wants to take over from here, I'd be thankful.
Otherwise, I'll return to this bug in a few days.
Assignee | ||
Comment 3•10 years ago
|
||
Mostly done, but eslint doesn't like this from script.js:
const steppingHookState = {
pauseAndRespond: (aFrame, onPacket=(k)=>k) => {
return this._pauseAndRespond(aFrame, { type: "resumeLimit" }, onPacket);
},
Specifically it errors on the second arrow:
toolkit/devtools/server/actors/script.js
897:50 error Unexpected token =>
Seems like a bug in eslint, I'm filing it there.
A workaround seems to be to remove the parens around (k).
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Here's the patch.
Attachment #8610115 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8611325 -
Flags: review?(pbrosset)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8611325 [details] [diff] [review]
remove SpiderMonkey-specific syntax from toolkit/devtools
Review of attachment 8611325 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, this looks mainly good to me. I've just picked up a couple of potential problems with replacing foreach loops with for..of or [x for()] array constructions with map() which I fear may fail. But I don't know the code in details enough to be sure. Feel free to ignore me if you already checked. Anyway, I guess only a full try run will tell us if something is wrong or not (make sure you include mochitest-other, xpcshell and mochitest-devtools tests).
Marking as R+ because there's not much sense in me checking again after you've corrected these or verified that they were correct in the first place. Just make sure you have a green try build before landing.
And thanks for doing this!
::: toolkit/devtools/server/tests/mochitest/inspector-helpers.js
@@ +124,5 @@
> let serverWalker = serverConnection.getActor(walker.actorID);
>
> return {
> root: serverOwnershipSubtree(serverWalker, serverWalker.rootDoc ),
> + orphaned: serverWalker._orphaned.map(o => serverOwnershipSubtree(serverWalker, o.rawNode)),
If I'm not mistaken, the _orphaned property is a Set, not an Array, so map won't work, you could wrap it with [...serverWalker._orphaned] to map on it.
::: toolkit/devtools/styleinspector/css-logic.js
@@ +407,5 @@
> * will be the this object when aCallback executes.
> */
> forEachSheet: function CssLogic_forEachSheet(aCallback, aScope)
> {
> + for (let sheets of this._sheets) {
Isn't this._sheets an object (I see `this._sheets = {}` in CssLogic_reset). If it is then we need a for..in loop, not a for..of loop.
@@ +436,5 @@
> * otherwise false is returned.
> */
> forSomeSheets: function CssLogic_forSomeSheets(aCallback, aScope)
> {
> + for (let sheets of this._sheets) {
Same here, but maybe I'm mistaken.
Attachment #8611325 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #6)
> Just make sure you have a green try build before landing.
Yeah, don't worry about that :)
> > + for (let sheets of this._sheets) {
>
> Isn't this._sheets an object (I see `this._sheets = {}` in CssLogic_reset).
> If it is then we need a for..in loop, not a for..of loop.
I will double check the ones you pointed out.
I went through everything by hand and ran the tookit/devtools tests to
isolate earlier mistakes along these lines. But it's possible that the
tests don't provide coverage in some areas.
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Some updates from re-inspecting the changes and running more tests.
Attachment #8611325 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
One spot needed to use next to send a value to an es6 generator.
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8612361 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8612455 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
Keywords: checkin-needed
Comment 13•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•