* SUMMARY Cleanup sidebar panels, reduce `delete` and use Maps instead of objects.
(In reply to comment #0) > * SUMMARY > Cleanup sidebar panels, reduce `delete` and use Maps instead of objects. Also using Symbol() when setting external properties, and switching to const/let (in the vicinity of the main changes).
Created attachment 264029 [details] [Patch] Proposed Fix
Comment on attachment 264029 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=264029&action=review > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:245 > - delete this._finalAttemptToRestoreViewStateTimeout; > + this._finalAttemptToRestoreViewStateTimeout = undefined; I think we should prefer 0 for timeouts, since it will be a number normally. > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:720 > +WebInspector.NavigationSidebarPanel.WasExpandedDuringFilteringSymbol = Symbol("was-expanded-during-filtering"); Great! > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:164 > + for (const type in WebInspector.ScopeChainNode.Type) I don't think const should be used in a loop, I am surprised this works. > Source/WebInspectorUI/UserInterface/Views/StorageSidebarPanel.js:275 > + let manifest = frameManifest.manifest; > + const manifestURL = manifest.manifestURL; Why const for the URL and let for the manifest? I think we should use const only for strictly const things.
(In reply to comment #3) > Comment on attachment 264029 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264029&action=review > > > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:245 > > - delete this._finalAttemptToRestoreViewStateTimeout; > > + this._finalAttemptToRestoreViewStateTimeout = undefined; > > I think we should prefer 0 for timeouts, since it will be a number normally. The IDs returned by setTimeout, setInterval, and requestAnimationFrame are similar (non-zero, unique), but in the last case we've been using `undefined` instead of zero when resetting identifiers. The consensus is that this improves clarity. > > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:720 > > +WebInspector.NavigationSidebarPanel.WasExpandedDuringFilteringSymbol = Symbol("was-expanded-during-filtering"); > > Great! > > > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:164 > > + for (const type in WebInspector.ScopeChainNode.Type) > > I don't think const should be used in a loop, I am surprised this works. AFAIK this works because for-in and for-of style loops redefine the loop variable every iteration. I'm fine switching to `let`. > > > Source/WebInspectorUI/UserInterface/Views/StorageSidebarPanel.js:275 > > + let manifest = frameManifest.manifest; > > + const manifestURL = manifest.manifestURL; > > Why const for the URL and let for the manifest? I think we should use const > only for strictly const things.
(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 264029 [details] > > [Patch] Proposed Fix > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=264029&action=review > > > > > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:245 > > > - delete this._finalAttemptToRestoreViewStateTimeout; > > > + this._finalAttemptToRestoreViewStateTimeout = undefined; > > > > I think we should prefer 0 for timeouts, since it will be a number normally. > > The IDs returned by setTimeout, setInterval, and requestAnimationFrame are > similar (non-zero, unique), but in the last case we've been using > `undefined` instead of zero when resetting identifiers. The consensus is > that this improves clarity. The name `_finalAttemptToRestoreViewStateTimeout` could be confusing here, since it holds a timeout ID, not a delay value.
Created attachment 264089 [details] [Patch] Proposed Fix
Comment on attachment 264089 [details] [Patch] Proposed Fix Clearing flags on attachment: 264089 Committed r191612: <http://trac.webkit.org/changeset/191612>
All reviewed patches have been landed. Closing bug.