Bug 175065

Summary: Web Inspector: fix ESLint errors
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: 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 Flags
Patch
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch
none
Patch none

Description Devin Rousso 2017-08-01 23:12:00 PDT
I noticed some errors while working on <https://webkit.org/b/175058> Web Inspector: simplify WebInspector with WI
Comment 1 Devin Rousso 2017-08-01 23:24:51 PDT
Created attachment 316935 [details]
Patch
Comment 2 Build Bot 2017-08-02 00:49:43 PDT
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
Comment 3 Build Bot 2017-08-02 00:49:44 PDT
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 4 Devin Rousso 2017-08-02 01:01:42 PDT
Comment on attachment 316935 [details]
Patch

Unrelated test failures.
Comment 5 Joseph Pecoraro 2017-08-02 01:30:33 PDT
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 `""`
Comment 6 Joseph Pecoraro 2017-08-02 01:31:14 PDT
> I think this one hurts readability. In fact I think most of the parents
> being removed hurt readability...

parenthesis*
Comment 7 Build Bot 2017-08-02 08:44:02 PDT
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
Comment 8 Build Bot 2017-08-02 08:44:03 PDT
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 9 Devin Rousso 2017-08-02 11:03:28 PDT
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 10 BJ Burg 2017-08-02 11:15:06 PDT
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 11 Timothy Hatcher 2017-10-13 09:35:28 PDT
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.
Comment 12 Devin Rousso 2017-10-13 15:32:16 PDT
Created attachment 323755 [details]
Patch
Comment 13 Joseph Pecoraro 2017-10-13 16:07:27 PDT
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
Comment 14 Devin Rousso 2017-10-13 16:11:33 PDT
Created attachment 323764 [details]
Patch
Comment 15 WebKit Commit Bot 2017-10-13 16:52:38 PDT
Comment on attachment 323764 [details]
Patch

Clearing flags on attachment: 323764

Committed r223308: <https://trac.webkit.org/changeset/223308>
Comment 16 WebKit Commit Bot 2017-10-13 16:52:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2017-10-13 16:53:02 PDT
<rdar://problem/34989647>