Bug 161567 - Web Inspector: Enable and enforce some recommended and stylistic ESLint rules
Summary: Web Inspector: Enable and enforce some recommended and stylistic ESLint rules
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks:
 
Reported: 2016-09-02 21:11 PDT by Joseph Pecoraro
Modified: 2016-09-04 22:15 PDT (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (35.55 KB, patch)
2016-09-02 21:12 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-09-02 21:11:50 PDT
Enable and enforce some stylistic ESLint rules
Comment 1 Joseph Pecoraro 2016-09-02 21:12:53 PDT
Created attachment 287853 [details]
[PATCH] Proposed Fix
Comment 2 Darin Adler 2016-09-03 07:14:07 PDT
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.
Comment 3 BJ Burg 2016-09-03 14:18:06 PDT
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.
Comment 4 Joseph Pecoraro 2016-09-04 21:48:30 PDT
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.
Comment 5 Joseph Pecoraro 2016-09-04 22:15:20 PDT
<https://trac.webkit.org/changeset/205426>