Closed Bug 1242278 Opened 4 years ago Closed 4 years ago

Eslint cleanup of Inspector markup view


(DevTools :: Inspector, defect)

Not set


(firefox47 fixed)

Firefox 47
Tracking Status
firefox47 --- fixed


(Reporter: gl, Assigned: gl)



(1 file, 3 obsolete files)

No description provided.
Attached patch 1242278.patch [1.0] (obsolete) — Splinter Review
Attachment #8711518 - Attachment is obsolete: true
Attachment #8711518 - Flags: review?(pbrosset)
Attachment #8711551 - Flags: review?(pbrosset)
Comment on attachment 8711551 [details] [diff] [review]
1242278.patch [1.0]

Review of attachment 8711551 [details] [diff] [review]:

Awesome, thanks for cleaning these up.
I have a few comments:

In markup.js, in get dropTargetNodes() {..} there's a long if conditional block that's longer than 80 characters (line 1678).
It's to fix if you replace the 2 instances of this._lastDropTarget by target in this block.

Line 1497, maybe replace:
let container = this.importNode(child, flash);
let childContainer = this.importNode(child, flash);

In parseAttributeValues, line 3075, the condition with the 3 calls to parseFromString can be made to fit within 80 if you export the long part to a helper function like:
  let parseAndGetNode = str => {
    return DOMParser.parseFromString(str).body.childNodes[0];
And then:
  let el = parseAndGetNode("<svg " + attr + "></svg>", "text/html") ||
           parseAndGetNode("<svg " + attr + "\"></svg>", "text/html") ||
           parseAndGetNode("<svg " + attr + "'></svg>", "text/html");

For the ""template" is not defined" error line 380, I believe our best option (for the time being) is to add a comment at the top of the file:
/* globals template */

There are a few more errors here and there in this file.

I also see 2 errors in html-editor.js on these lines (54, 55):
  config.extraKeys["F2"] = this.hide;
  config.extraKeys["Esc"] = this.hide.bind(this, false);
eslint complains about using the dot notation not being used here.

I know you've cleaned up a lot already, but I'm suggesting you do these extra things because we're so close to having the whole thing eslint-clean.
Once you do that, you'll be able to un-ignore them from the global .eslintignore file, and therefore prevent us from writing non-eslint compliant code again.

In .eslintignore, you can replace this:
with this:

::: devtools/client/inspector/markup/markup.js
@@ +465,5 @@
> +      "nodeselected",
> +      "test"
> +    ];
> +    let isHighlitNode = this._hoveredNode ===
> +                        this._inspector.selection.nodeFront;

Splitting this equality on 2 lines looks a bit weird to me.
It looks like this line is just 2 characters too long, so it should be easy to keep it all on one line by renaming the isHighlitNode to something shorter.

@@ +2415,5 @@
>    getChildContainers: function() {
>      return [...this.children.children].map(node => node.container);
>    },
> +  setExpanded: function(value) {}

ESLint complains about 'value' not being used.
There are discussions about removing this particular rule, I don't think it catches anything useful anyway.
But, in the meantime, I would suggest just removing the parameter, and adding a jsdoc comment that explains that a parameter can be passed.

Talking about this, you forgot one aValue, line 1848, in
setExpanded: function(aValue) {

@@ +3089,5 @@
>      // Prevents InvalidCharacterError - "String contains an invalid character".
>      try {
>        div.setAttribute(name, value);
>        attributes.push({ name, value });
> +    } catch (e) {}

In order to prevent eslint from warning about this, the "right" pattern to adopt here is to put the comment inside the catch:

// Try to set on an element in the document.
try {
  div.setAttribute(name, value);
  attributes.push({ name, value });
} catch (e) {
  // This may throw exceptions on bad input.
  // Prevents InvalidCharacterError - "String contains an invalid character".
Attachment #8711551 - Flags: review?(pbrosset) → review+
Attachment #8711744 - Attachment is obsolete: true
Attachment #8711816 - Flags: review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.