Enable and enforce some stylistic ESLint rules
Created attachment 287853 [details] [PATCH] Proposed Fix
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 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 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.
<https://trac.webkit.org/changeset/205426>