[webkit-reviews] review denied: [Bug 215747] Web Inspector: allow event breakpoints to be configured when they're added : [Attachment 407115] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 215747: Web Inspector: allow event breakpoints to be configured when
they're added
https://bugs.webkit.org/show_bug.cgi?id=215747

Attachment 407115: Patch

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




--- Comment #6 from Brian Burg <bburg at apple.com> ---
Comment on attachment 407115
  --> https://bugs.webkit.org/attachment.cgi?id=407115
Patch

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

r- due to noted bug. Overall this patch is in great shape.

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:80
> +	   this._domEventNameInputElement.placeholder = WI.UIString("Example:
\u201C%s\u201D").format(WI.unlocalizedString("click"));

This UIString needs more context.

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:102
> +		       if
(eventName.toLowerCase().startsWith(this._domEventNameInputElement.value.toLowe
rCase()))

The only functional bug I ran across was this: if you type the entire event
name, there is still a suggestion for the event name that exactly matches. This
suggestion does not go away when the full string is typed out, as happens for
script autocompletion.

I think this could be fixed in the case of only one suggestion by not appending
if the eventName === the input value.

For the case where the input is both equal and a prefix to another event name,
I think there needs some more code to ensure the suggestions dismiss when the
field loses focus.

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:120
> +	   // CodeMirror needs a refresh after the popover displays, to layout,
otherwise it doesn't appear.

Nit: awkward grammar

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:128
> +	   options.eventName = this._domEventNameInputElement.value;

Should this be options.eventName ??=  ?

> Source/WebInspectorUI/UserInterface/Views/URLBreakpointPopover.js:34
> +	   this._urlCodeMirror = null;

I don't like this name. Alternatives would be this._codeMirror or
this._CodeMirrorForURL.

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:269
> +	       let action = new
WI.BreakpointAction(WI.BreakpointAction.Type.Log, {data: "BREAKPOINT ACTION LOG
1"});

This seems like an annoying change. Is there an argument for deleting the
createAction() factory method? Code can always do the two-step process if
needed, but it seems like an ergonomic regression for tests.

EDIT: or, make it one-line as in some other test changes above.