Bug 175065 – Web Inspector: fix ESLint errors

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

I noticed some errors while working on <https://webkit.org/b/175058> Web Inspector: simplify WebInspector with WI

[Devin Rousso]

Created attachment 316935 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

Comment on attachment 316935 [details]
Patch

Unrelated test failures.

[Joseph Pecoraro]

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 `""`

[Joseph Pecoraro]

> I think this one hurts readability. In fact I think most of the parents
> being removed hurt readability...

parenthesis*

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

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.

[Blaze Burg]

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.

[Timothy Hatcher]

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.

[Devin Rousso]

Created attachment 323755 [details]
Patch

[Joseph Pecoraro]

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

[Devin Rousso]

Created attachment 323764 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323764 [details]
Patch

Clearing flags on attachment: 323764

Committed r223308: <https://trac.webkit.org/changeset/223308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34989647>