Bug 196230 – Web Inspector: Debugger: modernize serialization of breakpoints and the maps that hold them

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

[Devin Rousso]

Leverage static `deserialize` functions, as well as `Multimap`s to hold the breakpoints.

[Radar WebKit Bug Importer]

<rdar://problem/49236485>

[Devin Rousso]

Created attachment 365921 [details]
Patch

[EWS Watchlist]

Comment on attachment 365921 [details]
Patch

Attachment 365921 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11666184

New failing tests:
inspector/debugger/probe-manager-add-remove-actions.html

[EWS Watchlist]

Created attachment 365930 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365921 [details]
Patch

Attachment 365921 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11666514

New failing tests:
inspector/debugger/probe-manager-add-remove-actions.html

[EWS Watchlist]

Created attachment 365932 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365921 [details]
Patch

Attachment 365921 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11666990

New failing tests:
inspector/debugger/probe-manager-add-remove-actions.html

[EWS Watchlist]

Created attachment 365937 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 365975 [details]
Patch

[EWS Watchlist]

Comment on attachment 365975 [details]
Patch

Attachment 365975 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11675250

New failing tests:
animations/resume-after-page-cache.html

[EWS Watchlist]

Created attachment 365996 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Joseph Pecoraro]

Comment on attachment 365975 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:223
> +            return Array.from(this.breakpointsForSourceCode(sourceCode.sourceMap.originalSourceCode))
> +            .filter((breakpoint) => breakpoint.sourceCodeLocation.displaySourceCode === sourceCode);

I don't think this is our usual style.

    $ ack '^\s+\.[^.]' **/*.js -B 1 --ignore External

It is vastly more common to keep this on one line or to at least put the function name on the same line:

    return Array.from(this.breakpointsForSourceCode(sourceCode.sourceMap.originalSourceCode)).filter((breakpoint) => {
        return breakpoint.sourceCodeLocation.displaySourceCode === sourceCode;
    });

> Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:-33
> -        if (sourceCodeLocationOrInfo instanceof WI.SourceCodeLocation) {
> -            var sourceCode = sourceCodeLocationOrInfo.sourceCode;

Some of the oldest ugliest code. Glad to see this cleaned up!

> Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:41
> +        this._sourceCodeLocation = sourceCodeLocation;

After this I'd put:

    this._contentIdentifier = null;

It used to be `null` and would otherwise be `undefined` with this patch for special breakpoints.

> Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:64
> +    // Static

We can make this an "Import / Export" section and put `deserialize` (or `fromJSON`) and `toJSON` next to each other to it is easy to compare them.

> Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:70
> +            // The 'url' fallback is for transitioning from older frontends and should be removed.

Now might be a good time to remove it!

> Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:244
> -        return newAction;
> +        return this._actions[index];

Would be easier to avoid the lookup again by storing the new action in a local. Especially given someone could modify this_actions in the event handler.

> Source/WebInspectorUI/UserInterface/Models/BreakpointAction.js:39
> +    // Static

Same here re: "Import / Export" or at least putting it by the toJSON.

Also, we don't use the "id" in deserialization, but we presumably do for the protocol? There should probably be a toProtocol() [protocol] instead of toJSON() [serialization].

[Devin Rousso]

Created attachment 366435 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 366435 [details]
Patch

Clearing flags on attachment: 366435

Committed r243727: <https://trac.webkit.org/changeset/243727>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.