[webkit-reviews] review granted: [Bug 196230] Web Inspector: Debugger: modernize serialization of breakpoints and the maps that hold them : [Attachment 365975] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 196230: Web Inspector: Debugger: modernize serialization of breakpoints and
the maps that hold them
https://bugs.webkit.org/show_bug.cgi?id=196230

Attachment 365975: Patch

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




--- Comment #12 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 365975
  --> https://bugs.webkit.org/attachment.cgi?id=365975
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.originalSourceCod
e))
> +	       .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.originalSourceCod
e)).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].