[webkit-reviews] review granted: [Bug 120576] Web Inspector: Breakpoint Actions : [Attachment 210235] [PATCH] 2 - Frontend

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 120576: Web Inspector: Breakpoint Actions
https://bugs.webkit.org/show_bug.cgi?id=120576

Attachment 210235: [PATCH] 2 - Frontend
https://bugs.webkit.org/attachment.cgi?id=210235&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210235&action=review


Nice an clean!

> Source/WebInspectorUI/UserInterface/Breakpoint.js:43
> +	       actions[i] = WebInspector.BreakpointAction.fromProtocol(this,
actions[i]);

I wouldn't call this "fromProtocol". It is really about save and restore from
localStorage. So it is really "fromInfo" given the rename I also suggest for
the protocolObject getter. I would prefer to special case the
WebInspector.BreakpointAction constructor to check the type of the second
argument and do either normal or info object construction. I'm not a fan of
separate "from" constructor helpers if they can be avoided. (Call me jaded by
the old way "fromPayload" helpers leaked protocol data all over the Inspector
code.)

> Source/WebInspectorUI/UserInterface/BreakpointAction.js:41
> +WebInspector.BreakpointAction.fromProtocol = function(breakpoint,
breakpointActionObject)
> +{
> +    return new WebInspector.BreakpointAction(breakpoint,
breakpointActionObject.type, breakpointActionObject.data);
> +}

See previous comment.

> Source/WebInspectorUI/UserInterface/BreakpointAction.js:73
> +    get protocolObject()

I'd call this info() to match Breakpoint since this is used for both the
protocol and saving to localStorage and add a comment here that it is used for
those.

> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:49
> +    background-image: url(Images/BreakpointActionAdd.svg),
-webkit-linear-gradient(top, rgb(250, 250, 250), rgb(200, 200, 200));

Why does the gradient need to be here? Can it be moved to the SVG? I'm not sure
how this clips the gradient to the SVG circle!

> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:72
> +    -webkit-appearance: textfield;

Clever! But this adds some padding at the top and bottom that push the line
gutter down when it is showing. Not an issue in this patch since it is
disabled, but something to think about. The CodeMirror instance might be able
to set negative top and bottom margins to compensate.

> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:78
> +    width: 336px; /* NOTE: Fixed value, manually tuned to
.edit-breakpoint-popover-content.wide width */

calc()?

> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:83
> +    width: 336px; /* NOTE: Fixed value, manually tuned to
.edit-breakpoint-popover-content.wide width */

calc() here too?

> Source/WebInspectorUI/UserInterface/BreakpointActionView.js:128
> +	   case DebuggerAgent.BreakpointActionType.Log:

Yay enums!

> Source/WebInspectorUI/UserInterface/BreakpointActionView.js:152
> +		   indentWithTabs: true,
> +		   indentUnit: 4,
> +		   matchBrackets: true,

I wonder if there is a way to make these default so we don't need to set them
everywhere?

> Source/WebInspectorUI/UserInterface/Images/BreakpointActionAdd.svg:5
> +    <path stroke="#F1F2F3" stroke-width="2" d="M 4 9 L 12 9 M 8 5 L 8 13"/>
> +    <path stroke="#818181" stroke-width="2" d="M 4 8 L 12 8 M 8 4 L 8 12"/>

I like rgb() in the SVGs.