1325213 - Fix eslint errors in devtools/shared/(touch|qrcode|performance|security|shims|discovery|client|tests)

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Comment 1

[Tim Nguyen :ntim]

That's not all directories, but that's a lot :)

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1325213 - Fix lots of eslint errors in devtools/shared/.

Review commit: https://reviewboard.mozilla.org/r/100802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100802/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8821557 [details]
Bug 1325213 - Fix lots of eslint errors in devtools/shared/.

https://reviewboard.mozilla.org/r/100802/#review101724

Thanks for working on this cleanup, it's looking a lot better!  A few things I'd like to check again, so one more round.

::: devtools/shared/client/main.js:289
(Diff revision 1)
>        }
>      }, "DebuggerClient.requester request callback"));
>    }, "DebuggerClient.requester");
>  };
>  
> -function args(aPos) {
> +function args(pos) {

Maybe it's cleaner to rename this to `arg`, so we can avoid burning the name `args` and drop all the `args` to `argList` renames?  Using `...args` is fairly common idiom that seems worth preserving.

::: devtools/shared/client/main.js:465
(Diff revision 1)
> -      aOnResponse(aResponse, tabClient);
> -      return [aResponse, tabClient];
> +      onResponse(response, tabClient);
> +      return [response, tabClient];
>      });
>    },
>  
> -  attachWorker: function DC_attachWorker(aWorkerActor, aOnResponse = noop) {
> +  attachWorker: function attachWorker(workerActor, onResponse = noop) {

Maybe remove the function name to match others here?

::: devtools/shared/client/main.js:483
(Diff revision 1)
> -      if (aResponse.error) {
> -        aOnResponse(aResponse, null);
> -        return [aResponse, null];
> +      if (response.error) {
> +        onResponse(response, null);
> +        return [response, null];
>        }
>  
> -      let workerClient = new WorkerClient(this, aResponse);
> +      let newWorkerClient = new WorkerClient(this, response);

Hmm, it's probably fine to leave this as `workerClient` and remove the `let`.

::: devtools/shared/client/main.js:499
(Diff revision 1)
>     *        The actor ID for the addon to attach.
> -   * @param function aOnResponse
> +   * @param function onResponse
>     *        Called with the response packet and a AddonClient
>     *        (which will be undefined on error).
>     */
> -  attachAddon: function DC_attachAddon(aAddonActor, aOnResponse = noop) {
> +  attachAddon: function attachAddon(addonActor, onResponse = noop) {

Remove function name for parity.

::: devtools/shared/client/main.js:652
(Diff revision 1)
>    }),
>  
>    /**
>     * Send a request to the debugging server.
>     *
> -   * @param aRequest object
> +   * @param requestOpts object

Maybe `packet` is a better name, like the description says?  It's not really a set of options.

::: devtools/shared/client/main.js:869
(Diff revision 1)
>      let actor = request.actor;
>      this.expectReply(actor, request);
>  
>      if (request.format === "json") {
>        this._transport.send(request.request);
>        return false;

I don't believe this function is expected to return a value, so change this to just `return` instead of adding `return true` below.

::: devtools/shared/client/main.js:918
(Diff revision 1)
>     * greetings from new root actors, is the only case at the moment) we must be
>     * prepared for a "reply" that doesn't correspond to any request we sent.
>     */
> -  expectReply: function (aActor, aRequest) {
> -    if (this._activeRequests.has(aActor)) {
> -      throw Error("clashing handlers for next reply from " + uneval(aActor));
> +  expectReply: function (actor, request) {
> +    if (this._activeRequests.has(actor)) {
> +      throw Error("clashing handlers for next reply from " + uneval(actor));

I believe `actor` is just a string...  I think we can remove this `uneval` call, which is Gecko specific anyway.

::: devtools/shared/client/main.js:1381
(Diff revision 1)
>      }
>  
>      let packet = {
>        to: this._threadActor,
>        type: "attach",
> -      options: aOptions
> +      options: options

Just `options`

::: devtools/shared/client/main.js:1959
(Diff revision 1)
>    }),
>  
>    /**
>     * Enable or disable pausing when an exception is thrown.
>     *
> -   * @param boolean aFlag
> +   * @param boolean flag

Seems like these comments don't match the current args.

::: devtools/shared/client/main.js:3165
(Diff revision 1)
>  
>      if (root.traits.conditionalBreakpoints) {
>        let info = {
>          line: this.location.line,
>          column: this.location.column,
> -        condition: aCondition
> +        condition: condition

Just `condition`

::: devtools/shared/performance/test/head.js:4
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* exported require */

I believe we typically use:

```
/* eslint no-unused-vars: [2, {"vars": "local"}] */
```

in head files, since really everything is exported.  See `shared-head.js` etc.

::: devtools/shared/security/socket.js:362
(Diff revision 1)
>                .SSLStatus.serverCert;
>    let overrideBits = Ci.nsICertOverrideService.ERROR_UNTRUSTED |
>                       Ci.nsICertOverrideService.ERROR_MISMATCH;
> +  /* The last parameter is temporary */
>    certOverrideService.rememberValidityOverride(host, port, cert, overrideBits,
> -                                               true /* temporary */);
> +                                               true);

I strongly prefer inline comments for this case...  Please disable the rule for this line or something.  Just another reason I dislike this rule overall...  I filed bug 1326100 to consider allowing them in general.

::: devtools/shared/security/tests/unit/head_dbg.js:6
(Diff revision 1)
> -var Ci = Components.interfaces;
> -var Cu = Components.utils;
> -var Cr = Components.results;
> -var CC = Components.Constructor;
>  
> +/* exported defer, DebuggerClient, initTestDebuggerServer */

Use `/* eslint no-unused-vars: [2, {"vars": "local"}] */`

::: devtools/shared/security/tests/unit/head_dbg.js:8
(Diff revision 1)
> -var Cr = Components.results;
> -var CC = Components.Constructor;
>  
> +/* exported defer, DebuggerClient, initTestDebuggerServer */
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

Use spaces inside `{}` to match the rest of the file.

::: devtools/shared/security/tests/unit/head_dbg.js:55
(Diff revision 1)
> -  observe: function (aMessage) {
> -    errorCount++;
> +  observe: function (message) {
> +    let string;
>      try {
> -      // If we've been given an nsIScriptError, then we can print out
> -      // something nicely formatted, for tools like Emacs to pick up.
> -      var scriptError = aMessage.QueryInterface(Ci.nsIScriptError);
> +      dump(message.sourceName + ":" + message.lineNumber + ": " +
> +           scriptErrorFlagsToKind(message.flags) + ": " +
> +           message.errorMessage + "\n");

I believe we still want to call `QueryInterface` here to detect if message implements this interface (which will throw and fall to catch if it doesn't).

::: devtools/shared/tests/unit/head_devtools.js:4
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +/* exported DevToolsUtils, DevToolsLoader */

Use `/* eslint no-unused-vars: [2, {"vars": "local"}] */`

::: devtools/shared/tests/unit/head_devtools.js:29
(Diff revision 1)
> -var errorCount = 0;
>  var listener = {
> -  observe: function (aMessage) {
> -    errorCount++;
> +  observe: function (message) {
> +    let string;
>      try {
> -      // If we've been given an nsIScriptError, then we can print out
> +      dump(message.sourceName + ":" + message.lineNumber + ": " +

See comments in `devtools/shared/security/tests/unit/head_dbg.js`, need to keep the `QueryInterface`.

::: devtools/shared/tests/unit/test_async-utils.js:65
(Diff revision 1)
>  
>  // Test that the throwing from an async function rejects the promise.
>  function test_async_throw(async) {
>    let obj = {
>      method: async(function* () {
> -      throw "boom";
> +      let error = "boom";

This seems like a nonsensical fix, what is the actual problem?

::: devtools/shared/tests/unit/test_async-utils.js:149
(Diff revision 1)
>      do_check_eq(results[1], "bar");
>  
> -
>      // Test throwing from the function.
>      function thrower() {
> -      throw "boom";
> +      let error = "boom";

Same as above.

::: devtools/shared/tests/unit/test_defineLazyPrototypeGetter.js:1
(Diff revision 1)
>  /* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */

(Feel free to strip out these editor lines if you want to.)

::: devtools/shared/touch/simulator-core.js:348
(Diff revision 1)
>  
>      let utils = content.QueryInterface(Ci.nsIInterfaceRequestor)
>                         .getInterface(Ci.nsIDOMWindowUtils);
>  
>      let allowZoom = {},
> -        minZoom = {},
> +      minZoom = {},

I'd prefer `let` on each line, no commas.

::: testing/mochitest/browser.eslintrc.js:22
(Diff revision 1)
>      "Assert": false,
>      "BrowserTestUtils": false,
>      "content": false,
>      "ContentTask": false,
>      "ContentTaskUtils": false,
> +    "do_throw": false,

Hmm, are you sure it's available?  I thought it was xpcshell only...

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8821557 [details]
Bug 1325213 - Fix lots of eslint errors in devtools/shared/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100802/diff/1-2/

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8821557 [details]
Bug 1325213 - Fix lots of eslint errors in devtools/shared/.

https://reviewboard.mozilla.org/r/100802/#review101724

> Maybe it's cleaner to rename this to `arg`, so we can avoid burning the name `args` and drop all the `args` to `argList` renames?  Using `...args` is fairly common idiom that seems worth preserving.

Fixed

> Maybe remove the function name to match others here?

Removed

> Hmm, it's probably fine to leave this as `workerClient` and remove the `let`.

Fixed

> Remove function name for parity.

Removed

> Maybe `packet` is a better name, like the description says?  It's not really a set of options.

Renamed

> I don't believe this function is expected to return a value, so change this to just `return` instead of adding `return true` below.

Removed

> I believe `actor` is just a string...  I think we can remove this `uneval` call, which is Gecko specific anyway.

Removed

> Seems like these comments don't match the current args.

Updated the comment

> Just `condition`

Fixed

> I believe we typically use:
> 
> ```
> /* eslint no-unused-vars: [2, {"vars": "local"}] */
> ```
> 
> in head files, since really everything is exported.  See `shared-head.js` etc.

Filed bug 1326132

> I strongly prefer inline comments for this case...  Please disable the rule for this line or something.  Just another reason I dislike this rule overall...  I filed bug 1326100 to consider allowing them in general.

Fixed, even though I'm not a big fan of // eslint-disable-line

> Use spaces inside `{}` to match the rest of the file.

Fixed

> I believe we still want to call `QueryInterface` here to detect if message implements this interface (which will throw and fall to catch if it doesn't).

Readded

> This seems like a nonsensical fix, what is the actual problem?

Fixed.

> Hmm, are you sure it's available?  I thought it was xpcshell only...

Turns out it was a chrome test, so I've pointed to the relevant eslintrc file, and added the global there.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8821557 [details]
Bug 1325213 - Fix lots of eslint errors in devtools/shared/.

https://reviewboard.mozilla.org/r/100802/#review101826

Thanks for working on this, looks good to me with a few tweaks.

Thanks for allowing me to keep my inline comments. :)

::: devtools/shared/client/main.js:875
(Diff revisions 1 - 2)
>      }
>  
> -    this._transport.startBulkSend(request.request).then((...argList) => {
> -      request.emit("bulk-send-ready", ...argList);
> +    this._transport.startBulkSend(request.request).then((...args) => {
> +      request.emit("bulk-send-ready", ...args);
>      });
> -    return true;
> +    return;

I think this can be dropped, since it's already the end of the function and there's no value to return.

::: devtools/shared/security/socket.js:361
(Diff revisions 1 - 2)
>                .SSLStatus.serverCert;
>    let overrideBits = Ci.nsICertOverrideService.ERROR_UNTRUSTED |
>                       Ci.nsICertOverrideService.ERROR_MISMATCH;
> -  /* The last parameter is temporary */
>    certOverrideService.rememberValidityOverride(host, port, cert, overrideBits,
> -                                               true);
> +                                               true /* temporary */); // eslint-disable-line

You could also surround these two lines with eslint-disable and eslint-enable comments.  Maybe that's less visually noisy than eslint-disable-line?  Up to you!

::: devtools/shared/security/tests/chrome/.eslintrc.js:5
(Diff revisions 1 - 2)
>  "use strict";
>  
>  module.exports = {
>    // Extend from the shared list of defined globals for mochitests.
> -  "extends": "../../../../.eslintrc.mochitests.js"
> +  "extends": "../../../../../testing/mochitest/chrome.eslintrc.js"

This part seems logical, it is after all a mochitest chrome directory (fairly unusual for devtools).

::: testing/mochitest/chrome.eslintrc.js:16
(Diff revision 2)
>  
>    // All globals made available in the test environment.
>    "globals": {
>      "add_task": false,
>      "Assert": false,
> +    "do_throw": false,

I still felt unsure about this, so I had to check.  

The one test at issue here is `test_websocket-transport.html`.  There is one call to `do_throw` in an error path.  If you add `do_throw` to the regular flow of the test, you are told it's not defined, so using it is a bug in the test.

So, we don't want to add it here, but instead we should correct the test to do something else.  I think replacing `do_throw(<msg>)` with `ok(false, <msg>)` should be sufficient.

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8821557 [details]
Bug 1325213 - Fix lots of eslint errors in devtools/shared/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100802/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67611ffb1fef
Fix lots of eslint errors in devtools/shared/. r=jryans

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/b1e753b5d02b for https://treeherder.mozilla.org/logviewer.html#?job_id=65370906&repo=autoland

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8821557 [details]
Bug 1325213 - Fix lots of eslint errors in devtools/shared/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100802/diff/3-4/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51131741af7c
Fix lots of eslint errors in devtools/shared/. r=jryans

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51131741af7c