Bug 150548 – Web Inspector: Cleanup sidebar panels, reduce `delete` and use Maps instead of objects

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
Cleanup sidebar panels, reduce `delete` and use Maps instead of objects.

[Matt Baker]

(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).

[Matt Baker]

Created attachment 264029 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

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.

[Matt Baker]

(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.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 264089 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 264089 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 264089

Committed r191612: <http://trac.webkit.org/changeset/191612>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.