Eslint cleanup of Inspector markup view

RESOLVED FIXED in Firefox 47

Status

DevTools
Inspector
RESOLVED FIXED
2 years ago
5 days ago

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 47

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 2

2 years ago
Created attachment 8711551 [details] [diff] [review]
1242278.patch [1.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc61035cf17f
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);
with
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.
config.extraKeys.F2
config.extraKeys.Esc

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:
devtools/client/inspector/markup/**
with this:
devtools/client/inspector/markup/test/**

::: 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+
(Assignee)

Comment 5

2 years ago
Created attachment 8711816 [details] [diff] [review]
1242278.patch [3.0]
Attachment #8711744 - Attachment is obsolete: true
Attachment #8711816 - Flags: review+

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1bd84f34814d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

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