Summary: | Web Inspector: fix ESLint errors | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bburg, buildbot, commit-queue, inspector-bugzilla-changes, joepeck, rniwa, timothy, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Devin Rousso
2017-08-01 23:12:00 PDT
Created attachment 316935 [details]
Patch
Comment on attachment 316935 [details] Patch Attachment 316935 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4238546 New failing tests: imported/w3c/web-platform-tests/fetch/api/basic/mode-no-cors-worker.html imported/w3c/web-platform-tests/fetch/api/basic/mode-no-cors.html Created attachment 316945 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 316935 [details]
Patch
Unrelated test failures.
Comment on attachment 316935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316935&action=review > Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:64 > - var match = securityOrigin.match(/^([^:]+):\/\/([^\/:]*)(?::([\d]+))?$/i); > + var match = securityOrigin.match(/^([^:]+):\/\/([^/:]*)(?::([\d]+))?$/i); Hmm, I don't really like this change. I think we should always escape a '/' character inside of a regex, even if it is inside of a character set. > Source/WebInspectorUI/UserInterface/Models/DOMNode.js:605 > - return `[id=\"${id}\"]`; > + return `[id="${id}"]`; Nice! > Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:569 > - var startChar = line === range.start.line ? range.start.ch : (lineContent.length - lineContent.trimLeft().length); > + var startChar = line === range.start.line ? range.start.ch : lineContent.length - lineContent.trimLeft().length; I think this one hurts readability. In fact I think most of the parents being removed hurt readability... > Source/WebInspectorUI/UserInterface/Views/DatabaseTableContentView.js:72 > - return name.replace(/\"/g, "\"\""); > + return name.replace(/"/g, "\"\""); Might as well cleanup the other side to be `""` > I think this one hurts readability. In fact I think most of the parents
> being removed hurt readability...
parenthesis*
Comment on attachment 316935 [details] Patch Attachment 316935 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4240110 New failing tests: fast/dom/StyleSheet/detached-sheet-owner-node-link.html Created attachment 316961 [details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 316935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316935&action=review >> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:569 >> + var startChar = line === range.start.line ? range.start.ch : lineContent.length - lineContent.trimLeft().length; > > I think this one hurts readability. In fact I think most of the parents being removed hurt readability... What's your criteria for a parenthesis improving readability? I think that some of the cases here are valid to remove, such as Main.js and ProfileNode.js. Comment on attachment 316935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316935&action=review >>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:569 >>> + var startChar = line === range.start.line ? range.start.ch : lineContent.length - lineContent.trimLeft().length; >> >> I think this one hurts readability. In fact I think most of the parents being removed hurt readability... > > What's your criteria for a parenthesis improving readability? I think that some of the cases here are valid to remove, such as Main.js and ProfileNode.js. Ditto on Joe. If I have to stop and think about precedence where I didn't before, then it hurts readability. Comment on attachment 316935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316935&action=review >> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:64 >> + var match = securityOrigin.match(/^([^:]+):\/\/([^/:]*)(?::([\d]+))?$/i); > > Hmm, I don't really like this change. I think we should always escape a '/' character inside of a regex, even if it is inside of a character set. I agree. Created attachment 323755 [details]
Patch
Comment on attachment 323755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323755&action=review r=me! > Source/WebInspectorUI/.eslintrc:129 > + // DOM Utilties Typo: Utilities Created attachment 323764 [details]
Patch
Comment on attachment 323764 [details] Patch Clearing flags on attachment: 323764 Committed r223308: <https://trac.webkit.org/changeset/223308> All reviewed patches have been landed. Closing bug. |