1321839 - make devtools/shared eslint-clean

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Tom Tromey :tromey]

I noticed a buglet in devtools/shared that would have been caught by eslint,
so I went ahead and fixed all the files there.

Most of the patch is uninteresting but there are a couple of latent bugs in there.

Comment 1

[Tom Tromey :tromey]

Attachment: Bug 1321839 - make devtools/shared eslint-clean;

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

Comment 2

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

Comment on attachment 8816503 [details]
Bug 1321839 - make devtools/shared eslint-clean;

https://reviewboard.mozilla.org/r/97230/#review97554

Looks good, thanks for the cleanup!  Should be fine without another round.

::: devtools/shared/DevToolsUtils.js:34
(Diff revision 1)
>  /**
>   * Waits for the next tick in the event loop to execute a callback.
>   */
> -exports.executeSoon = function executeSoon(aFn) {
> +exports.executeSoon = function executeSoon(fn) {
>    if (isWorker) {
> -    setImmediate(aFn);
> +    setImmediate(fn);           // eslint-disable-line no-undef

Maybe a /* global */ block with this at the top of the file?  Not sure which is better since it's conditional...

::: devtools/shared/DevToolsUtils.js:349
(Diff revision 1)
>   *        The URL used to obtain the module.
> - * @param aSymbol
> + * @param symbol
>   *        The name of the symbol exported by the module.
> - *        This parameter is optional and defaults to aName.
> + *        This parameter is optional and defaults to name.
>   */
> -exports.defineLazyModuleGetter = function defineLazyModuleGetter(aObject, aName,
> +exports.defineLazyModuleGetter = function defineLazyModuleGetter(object, name,

Does this function need to be named, or can everyone just reference it from `exports` / `DevToolsUtils`?  Removing the name gives room for params on one line.

::: devtools/shared/DevToolsUtils.js:579
(Diff revision 1)
>    }
>  }
>  
>  // Fetch is defined differently depending on whether we are on the main thread
>  // or a worker thread.
>  if (!this.isWorker) {

Maybe reverse these blocks?

::: devtools/shared/DevToolsUtils.js:586
(Diff revision 1)
>  } else {
>    // Services is not available in worker threads, nor is there any other way
>    // to fetch a URL. We need to enlist the help from the main thread here, by
>    // issuing an rpc request, to fetch the URL on our behalf.
>    exports.fetch = function (url, options) {
> -    return rpc("fetch", url, options);
> +    return rpc("fetch", url, options); // eslint-disable-line no-undef

Global block?

::: devtools/shared/protocol.js:363
(Diff revision 1)
>   */
>  types.addActorDetail = function (name, actorType, detail) {
>    actorType = types.getType(actorType);
>    if (!actorType._actor) {
> -    throw Error("Details only apply to actor types, tried to add detail '" + detail + "'' to " + actorType.name + "\n");
> +    throw Error("Details only apply to actor types, tried to add detail '" +
> +                detail + "'' to " + actorType.name + "\n");

One too many single quotes (copied from original)?  Also, a template string should make this simpler to read.

::: devtools/shared/protocol.js:412
(Diff revision 1)
>   *    An actor type
>   */
>  types.addLifetimeType = function (lifetime, subtype) {
>    subtype = types.getType(subtype);
>    if (!subtype._actor) {
> -    throw Error("Lifetimes only apply to actor types, tried to apply lifetime '" + lifetime + "'' to " + subtype.name);
> +    throw Error("Lifetimes only apply to actor types, tried to apply lifetime '" +

One too many single quotes (copied from original)?  Also, a template string should make this simpler to read.

::: devtools/shared/protocol.js:1404
(Diff revision 1)
>      if (name in frontProto) {
>        let custom = frontProto[spec.name]._customFront;
>        if (custom === undefined) {
> -        throw Error("Existing method for " + spec.name + " not marked customFront while processing " + actorType.typeName + ".");
> +        throw Error("Existing method for " + spec.name +
> +                    " not marked customFront while processing " + actorSpec.typeName +
> +                    ".");

I'm crying. :)

::: devtools/shared/system.js:84
(Diff revision 1)
>      brandName = null;
>    }
>  
>    if (win) {
> -    let utils = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +    let utils = win.QueryInterface(Ci.nsIInterfaceRequestor)
> +        .getInterface(Ci.nsIDOMWindowUtils);

I usually align the dot to the dot of the previous line...  But either way, it seems like you're not too consistent, this line uses 4 space indent, but the next one uses 2 space indent (or it looks that way in mozreview).

::: devtools/shared/system.js:330
(Diff revision 1)
>      } catch (e) {
>        return promise.reject(e);
>      }
>  
> -    let req = settingsService.createLock().get(name, {
> -      handle: (name, value) => deferred.resolve(value),
> +    settingsService.createLock().get(name, {
> +      handle: (ignore, value) => deferred.resolve(value),

Maybe `_`?

Comment 3

[Tom Tromey :tromey]

Comment on attachment 8816503 [details]
Bug 1321839 - make devtools/shared eslint-clean;

https://reviewboard.mozilla.org/r/97230/#review97554

> Maybe a /* global */ block with this at the top of the file?  Not sure which is better since it's conditional...

I went with global, it seems fine.

> Does this function need to be named, or can everyone just reference it from `exports` / `DevToolsUtils`?  Removing the name gives room for params on one line.

I removed the names from all the exported functions in this file.  I think none of them were needed.

> I usually align the dot to the dot of the previous line...  But either way, it seems like you're not too consistent, this line uses 4 space indent, but the next one uses 2 space indent (or it looks that way in mozreview).

I think it depends on the context.  However I went ahead and just aligned things.

Comment 4

[Tom Tromey :tromey]

Comment on attachment 8816503 [details]
Bug 1321839 - make devtools/shared eslint-clean;

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f120c4b9c96
make devtools/shared eslint-clean; r=jryans

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f120c4b9c96