Web Inspector: Enable and enforce some recommended and stylistic ESLint rules

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Enable and enforce some stylistic ESLint rules

[Joseph Pecoraro]

Created attachment 287853 [details]
[PATCH] Proposed Fix

[Darin Adler]

Comment on attachment 287853 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=287853&action=review

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:597
> -            let timerDetails = {timerId: recordPayload.data.timerId, timeout: recordPayload.data.timeout, repeating: !recordPayload.data.singleShot};
> +            var timerDetails = {timerId: recordPayload.data.timerId, timeout: recordPayload.data.timeout, repeating: !recordPayload.data.singleShot};

I don’t understand why this semantic change would be needed to satisfy a style rule.

[BJ Burg]

Comment on attachment 287853 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=287853&action=review

r=me but have questions about let->var in case statements.

> Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:-107
> -        "image/jpeg": "jpeg",

This seems unrelated to the style change.

> Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:-111
> -        "image/tiff": "tif",

Ditto.

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:597
>> +            var timerDetails = {timerId: recordPayload.data.timerId, timeout: recordPayload.data.timeout, repeating: !recordPayload.data.singleShot};
> 
> I don’t understand why this semantic change would be needed to satisfy a style rule.

Yeah, this does not seem necessary to me.

> Source/WebInspectorUI/UserInterface/Models/SourceCodeLocation.js:335
> +            var displayURL = sourceCode.displayURL;

Same thing as Darin's comment here.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineDataGridNode.js:58
> +            var titleElement = fragment.appendChild(document.createElement("span"));

Ditto.

[Joseph Pecoraro]

Comment on attachment 287853 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=287853&action=review

>> Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:-107
>> -        "image/jpeg": "jpeg",
> 
> This seems unrelated to the style change.

This is a duplicate key. I'm removing the bad duplicate.

>> Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:-111
>> -        "image/tiff": "tif",
> 
> Ditto.

This is a duplicate key. I'm removing the bad duplicate.

>>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:597
>>> +            var timerDetails = {timerId: recordPayload.data.timerId, timeout: recordPayload.data.timeout, repeating: !recordPayload.data.singleShot};
>> 
>> I don’t understand why this semantic change would be needed to satisfy a style rule.
> 
> Yeah, this does not seem necessary to me.

I have been making this review comment on patches for a while. We should not use `let` or `const` inside switch statements. This is a recommended ESLint warning:
<http://eslint.org/docs/rules/no-case-declarations>

`let` inside `switch` is a tricky subject. It often leads to unexpected runtime unused declaration warnings. Using `var` is safer.

>> Source/WebInspectorUI/UserInterface/Models/SourceCodeLocation.js:335
>> +            var displayURL = sourceCode.displayURL;
> 
> Same thing as Darin's comment here.

Same.

>> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineDataGridNode.js:58
>> +            var titleElement = fragment.appendChild(document.createElement("span"));
> 
> Ditto.

Same.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/205426>