Closed
Bug 1168088
Opened 7 years ago
Closed 7 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•7 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•7 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•7 years ago
|
||
Filed as https://github.com/eslint/eslint/issues/2616
Assignee | ||
Comment 5•7 years ago
|
||
Here's the patch.
Attachment #8610115 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8611325 -
Flags: review?(pbrosset)
Reporter | ||
Comment 6•7 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•7 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•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f11bf9e4217
Assignee | ||
Comment 9•7 years ago
|
||
Some updates from re-inspecting the changes and running more tests.
Attachment #8611325 -
Attachment is obsolete: true
Assignee | ||
Comment 10•7 years ago
|
||
One spot needed to use next to send a value to an es6 generator.
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08cc7889f2be
Assignee | ||
Updated•7 years ago
|
Attachment #8612361 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8612455 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 12•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/157f93335ed1
Keywords: checkin-needed
Comment 13•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/157f93335ed1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•