WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
150548
Web Inspector: Cleanup sidebar panels, reduce `delete` and use Maps instead of objects
https://bugs.webkit.org/show_bug.cgi?id=150548
Summary
Web Inspector: Cleanup sidebar panels, reduce `delete` and use Maps instead o...
Matt Baker
Reported
2015-10-25 20:12:12 PDT
* SUMMARY Cleanup sidebar panels, reduce `delete` and use Maps instead of objects.
Attachments
[Patch] Proposed Fix
(18.17 KB, patch)
2015-10-25 20:21 PDT
,
Matt Baker
no flags
Details
Formatted Diff
Diff
[Patch] Proposed Fix
(18.17 KB, patch)
2015-10-26 15:55 PDT
,
Matt Baker
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Matt Baker
Comment 1
2015-10-25 20:15:17 PDT
(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
Comment 2
2015-10-25 20:21:53 PDT
Created
attachment 264029
[details]
[Patch] Proposed Fix
Timothy Hatcher
Comment 3
2015-10-26 08:12:45 PDT
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
Comment 4
2015-10-26 15:03:36 PDT
(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
Comment 5
2015-10-26 15:05:45 PDT
(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
Comment 6
2015-10-26 15:55:09 PDT
Created
attachment 264089
[details]
[Patch] Proposed Fix
WebKit Commit Bot
Comment 7
2015-10-26 16:54:40 PDT
Comment on
attachment 264089
[details]
[Patch] Proposed Fix Clearing flags on attachment: 264089 Committed
r191612
: <
http://trac.webkit.org/changeset/191612
>
WebKit Commit Bot
Comment 8
2015-10-26 16:54:45 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug