1168088 - Remove SpiderMonkey specific JS syntax in toolkit/devtools

Status: RESOLVED FIXED

Description

[Patrick Brosset <:pbro>]

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

Comment 1

[Patrick Brosset <:pbro>]

Created attachment 8610115 [details] [diff] [review]
bug1168088-WIP-spidermonkey-toolkit-devtools.diff

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.

Comment 2

[Tom Tromey :tromey]

I've got it.

Comment 3

[Tom Tromey :tromey]

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).

Comment 4

[Tom Tromey :tromey]

Filed as https://github.com/eslint/eslint/issues/2616

Comment 5

[Tom Tromey :tromey]

Created attachment 8611325 [details] [diff] [review]
remove SpiderMonkey-specific syntax from toolkit/devtools

Here's the patch.

Comment 6

[Patrick Brosset <:pbro>]

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.

Comment 7

[Tom Tromey :tromey]

(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.

Comment 8

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f11bf9e4217

Comment 9

[Tom Tromey :tromey]

Created attachment 8612361 [details] [diff] [review]
remove SpiderMonkey-specific syntax from toolkit/devtools

Some updates from re-inspecting the changes and running more tests.

Comment 10

[Tom Tromey :tromey]

Created attachment 8612455 [details] [diff] [review]
remove SpiderMonkey-specific syntax from toolkit/devtools

One spot needed to use next to send a value to an es6 generator.

Comment 11

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08cc7889f2be

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/157f93335ed1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/157f93335ed1