Closed Bug 1267750 Opened 8 years ago Closed 8 years ago

[ESLint] Fix ESLint issues in devtools/server/actors/inspector.js

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: gl, Assigned: gl)

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Attached patch 1267750.patch (obsolete) — Splinter Review
Attached patch 1267750.patchSplinter Review
Attachment #8745479 - Attachment is obsolete: true
Attachment #8745488 - Flags: review?(pbrosset)
Comment on attachment 8745488 [details] [diff] [review]
1267750.patch

Review of attachment 8745488 [details] [diff] [review]:
-----------------------------------------------------------------

A few comments about code formatting, especially when splitting lines. We tend to vertically align things a lot of the times.

::: devtools/server/actors/inspector.js
@@ +238,5 @@
>    isDocumentElement: function() {
>      return this.rawNode.ownerDocument &&
>             this.rawNode.ownerDocument.documentElement === this.rawNode;
>    },
> +  destroy: function() {

Actually, with bug 1263258, we're moving to 
destroy: function () {
So you should revert this.

@@ +722,5 @@
>      for (let change of modifications) {
>        if (change.newValue == null) {
>          if (change.attributeNamespace) {
> +          rawNode.removeAttributeNS(change.attributeNamespace,
> +            change.attributeName);

I think it would look better here if you vertically aligned the 2 arguments:

rawNode.removeAttributeNS(change.attributeNamespace,
                          change.attributeName);

@@ +728,5 @@
>            rawNode.removeAttribute(change.attributeName);
>          }
> +      } else if (change.attributeNamespace) {
> +        rawNode.setAttributeNS(change.attributeNamespace, change.attributeName,
> +          change.newValue);

Same here.

@@ +895,5 @@
>      return this._form.nodeName;
>    },
>    get doctypeString() {
> +    return "<!DOCTYPE " + this._form.name +
> +     (this._form.publicId ? " PUBLIC '" + this._form.publicId + "'" : "") +

So this change (and the next line) will end up displaying DOCTYPE nodes in the inspector differently, which isn't what we want I think.
Today (on bmo for example), we have this:

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
                      "http://www.w3.org/TR/html4/loose.dtd">

And with this change, it will become:

<!DOCTYPE html PUBLIC '-//W3C//DTD HTML 4.01 Transitional//EN'
                      'http://www.w3.org/TR/html4/loose.dtd'>

So please either use \" or keep using single quotes. Our eslint rule allows using single quote for strings whenever there are double quotes in them.

@@ +1681,5 @@
>          break;
>        }
>        if (options.sameTypeRootTreeItem &&
> +            nodeDocshell(cur).sameTypeRootTreeItem !=
> +            nodeDocshell(node.rawNode).sameTypeRootTreeItem) {

Why indent these 2 lines more than the first one (options.sameTypeRootTreeItem &&) ?
I don't think this helps legibility.

@@ +1965,5 @@
>      if (isNodeDead(node)) {
>        return { hasFirst: true, hasLast: true, nodes: [] };
>      }
> +    let parentNode = this.getDocumentWalker(node.rawNode, options.whatToShow)
> +      .parentNode();

Maybe vertical alignment would be better here. I think it's especially useful when splitting dot notations on several lines:

let parentNode = this.getDocumentWalker(this.rawNode, options.whatToShow)
                     .parentNode();

@@ +2276,1 @@
>              ...this.getSuggestionsForQuery(null, completing, "id").suggestions

This would probably look better if it was also split on 2 lines like the previous array item, and if .suggestions was vertically aligned with .getSuggestionsForQuery on the previous line.

@@ +3708,5 @@
>      let nodeActor = walkerActor._ref(rawNode);
>      // Pass the node through a read/write pair to create the client side actor.
>      let nodeType = types.getType("domnode");
> +    let returnNode = nodeType.read(nodeType
> +      .write(nodeActor, walkerActor), this);

It might be more natural, more consistent with the rest to split after the first ( so you have the function call on the first line, and all arguments on the second.

let returnNode = nodeType.read(
  nodeType.write(nodeActor, walkerActor), this);

@@ +3788,5 @@
>    initialize: function(conn, tabActor) {
>      protocol.Actor.prototype.initialize.call(this, conn);
>      this.tabActor = tabActor;
>    },
> +  destroy: function() {

Same comment about keeping the space I made earlier.

@@ +4037,5 @@
>    let doc = node ? nodeDocument(node) : null;
>    let win = doc ? doc.defaultView : null;
>    if (win) {
> +    return win.QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDocShell);

These long queryInterface chains are normally always written vertically aligned, with the dot on the next line:

return win.QueryInterface(Ci.nsIInterfaceRequestor)
          .getInterface(Ci.nsIDocShell);
Attachment #8745488 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/4dc48f3688f0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: