Fix remaining eslint errors in devtools/client/inspector

RESOLVED FIXED in Firefox 49

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: pbro, Assigned: pbro)

Tracking

(Blocks 1 bug)

Trunk
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
No description provided.
Assignee

Updated

3 years ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Assignee

Comment 2

3 years ago
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 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. :)
Attachment #8755432 - Flags: review?(jryans) → review+
Thanks for working on this! :)
Assignee

Comment 6

3 years ago
(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 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/00f9eb3e72e8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.