Bug 149760 - Web Inspector: Cleanup DebuggerManager, reduce `delete` and use Maps instead of objects
Summary: Web Inspector: Cleanup DebuggerManager, reduce `delete` and use Maps instead ...
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: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-10-02 12:44 PDT by Matt Baker
Modified: 2015-10-02 19:28 PDT (History)
8 users (show)

See Also:


Attachments
[Patch] Proposed Fix (12.01 KB, patch)
2015-10-02 18:06 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] For Landing (12.17 KB, patch)
2015-10-02 18:40 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2015-10-02 12:44:58 PDT
* SUMMARY
Cleanup DebuggerManager, reduce `delete` and use Maps instead of objects.
Comment 1 Radar WebKit Bug Importer 2015-10-02 12:46:04 PDT
<rdar://problem/22954630>
Comment 2 Matt Baker 2015-10-02 18:06:24 PDT
Created attachment 262371 [details]
[Patch] Proposed Fix
Comment 3 Matt Baker 2015-10-02 18:17:19 PDT
No instances of delete to remove (other than those used to remove hashmap object properties), just the switch to using Map.
Comment 4 Joseph Pecoraro 2015-10-02 18:22:31 PDT
Comment on attachment 262371 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=262371&action=review

r=me

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:280
> +        if (this._breakpointURLMap.has(sourceCode.url)) {
> +            let urlBreakpoints = this._breakpointURLMap.get(sourceCode.url);

I tend to think we should reduce "has" && "get" to just a get.

    let urlBreakpoints = this._breakpointURLMap.get(sourceCode.url);
    if (urlBreakpoints) { ... }

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:286
> +        if (sourceCode instanceof WebInspector.Script && this._breakpointScriptIdentifierMap.has(sourceCode.id)) {
> +            let scriptIdentifierBreakpoints = this._breakpointScriptIdentifierMap.get(sourceCode.id);

Same here, but it would involve more rewriting.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:296
> -        return this._breakpointIdMap[id];
> +        return this._breakpointIdMap.get(id);

We probably should do `|| null` here too.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:301
> +        return this._scriptIdMap.get(id);

This should still have `|| null` to return `null` for missing values instead of `undefined`.
Comment 5 Matt Baker 2015-10-02 18:40:15 PDT
Created attachment 262374 [details]
[Patch] For Landing
Comment 6 WebKit Commit Bot 2015-10-02 19:28:49 PDT
Comment on attachment 262374 [details]
[Patch] For Landing

Clearing flags on attachment: 262374

Committed r190540: <http://trac.webkit.org/changeset/190540>
Comment 7 WebKit Commit Bot 2015-10-02 19:28:54 PDT
All reviewed patches have been landed.  Closing bug.