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)
DevTools
Inspector
Tracking
(firefox49 fixed)
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: gl, Assigned: gl)
Details
Attachments
(1 file, 1 obsolete file)
63.96 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4906357814d4
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8745479 -
Attachment is obsolete: true
Attachment #8745488 -
Flags: review?(pbrosset)
Comment 4•8 years ago
|
||
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+
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4dc48f3688f0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•