I noticed some errors while working on <https://webkit.org/b/175058> Web Inspector: simplify WebInspector with WI
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.
<rdar://problem/34989647>