1274964 - Fix remaining eslint errors in devtools/client/inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Comment 1

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1274964 - ESLint cleanup of devtools/client/inspector; r=jryans

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

Comment 2

[Patrick Brosset <:pbro>]

Jryans: most of the patch should be simple enough (minor eslint changes in tests, mostly). But here are some comments about the trickier parts:

- The testActor registration and access functions aren't being exported in a common way, and so eslint can't find them automatically, so I've just added a /* globals */ comment for them.
- inspector-search.js uses a fallthrough in a switch/case that eslint doesn't like, so I ended up just disabling eslint for that line.
- inspector-panel.js is a long file that had many lines longer than 80 chars. Instead of wrapping them all and ending up with potentially weird formatting, I wanted to experiment with a 100 chars long lines, which is why there's a comment for this at the top of the file. I'd like us to have the same config for this rule everywhere in the codebase, but at the same time, this enables making files eslint-clean pretty easily. I once started to remove eslint errors in some of devtools/server/actors file, and all I was doing was wrapping lines at weird places. Just changing 80 to 100 in those cases helps a lot. And I don't think people mind too much what's the actual value, just that there is one.
- I completely rewrote browser_inspector_search-suggests-ids-and-classes.js because it looked weird and it made it easier to fix eslint errors that way. It dates from before we used tasks so much in tests and so it just didn't fit in with the other tests. The functionality is the same.

Comment 3

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac82c9954d9d&group_state=expanded

Comment 4

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

Comment on attachment 8755432 [details]
MozReview Request: Bug 1274964 - ESLint cleanup of devtools/client/inspector; r=jryans

https://reviewboard.mozilla.org/r/54608/#review51212

::: devtools/client/inspector/inspector-panel.js:1108
(Diff revision 1)
>        }
>  
> -      let hierarchical = aPseudo == ":hover" || aPseudo == ":active";
> -      return this.walker.addPseudoClassLock(node, aPseudo, {parents: hierarchical});
> +      let hierarchical = pseudo == ":hover" || pseudo == ":active";
> +      return this.walker.addPseudoClassLock(node, pseudo, {parents: hierarchical});
>      }
> +    return null;

The walker methods called here are RDP methods, right?  I would think they return promises, so this should return `promise.resolve()` or something equivalent so there is a promise returned by all paths.

::: devtools/client/inspector/inspector-panel.js:1159
(Diff revision 1)
>    /**
>     * Clear any pseudo-class locks applied to the current hierarchy.
>     */
>    clearPseudoClasses: function () {
>      if (!this.walker) {
> -      return;
> +      return null;

Should be resolved promise.

::: devtools/client/inspector/inspector-search.js:507
(Diff revision 1)
>      if (query.endsWith("*") || state === this.States.ATTRIBUTE) {
>        // Hide the popup if the query ends with * (because we don't want to
>        // suggest all nodes) or if it is an attribute selector (because
>        // it would give a lot of useless results).
>        this.hidePopup();
> -      return;
> +      return null;

Should this function have a return value at all?  It's only bound as a event handler it seems...

::: devtools/client/inspector/test/browser_inspector_menu-01-sensitivity.js:284
(Diff revision 1)
>  /**
>   * A helper that simulates a contextmenu event on the given chrome DOM element.
>   */
>  function contextMenuClick(element) {
>    let evt = element.ownerDocument.createEvent("MouseEvents");
> -  let button = 2;  // right click
> +  let button = 2;

I am not a big fan of this rule that blocks inline comments...  Just an aside. :)

Comment 5

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

Thanks for working on this! :)

Comment 6

[Patrick Brosset <:pbro>]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> ::: devtools/client/inspector/inspector-panel.js:1108
> (Diff revision 1)
> >        }
> >  
> > -      let hierarchical = aPseudo == ":hover" || aPseudo == ":active";
> > -      return this.walker.addPseudoClassLock(node, aPseudo, {parents: hierarchical});
> > +      let hierarchical = pseudo == ":hover" || pseudo == ":active";
> > +      return this.walker.addPseudoClassLock(node, pseudo, {parents: hierarchical});
> >      }
> > +    return null;
> 
> The walker methods called here are RDP methods, right?  I would think they
> return promises, so this should return `promise.resolve()` or something
> equivalent so there is a promise returned by all paths.
You're right. I have corrected this. Although in this case it made no difference because nothing was using this return value.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/00f9eb3e72e8

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00f9eb3e72e8