Bug 215747 – Web Inspector: allow event breakpoints to be configured when they're added

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

after bug 215362, it's possible to right-click event breakpoints and edit them, but this is a two step process that could me made into a single step

[Devin Rousso]

Created attachment 407115 [details]
Patch

this does change how the event/URL breakpoint popover looks when non-editable, but I personally think it looks fine, espcially since it'll only really affect legacy remote inspection

[Devin Rousso]

Created attachment 407116 [details]
[Image] after Patch is applied (event breakpoint popover)

[Devin Rousso]

Created attachment 407117 [details]
[Image] after Patch is applied (URL breakpoint popover)

[Blaze Burg]

Comment on attachment 407115 [details]
Patch

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:120
> +localizedStrings["All Animation Frames @ Event Breakpoint"] = "All Animation Frames";

The '@' reads weird to me, but this is a huge improvement.

> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:60
> +        return this._eventListener ? InspectorBackend.hasCommand("DOM.setBreakpointForEventListener", "options") : InspectorBackend.hasCommand("DOMDebugger.setEventBreakpoint", "options");

Please break this into separate lines. It's hard to read.

> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:69
> +            actions: json.actions?.map((actionJSON) => WI.BreakpointAction.fromJSON(actionJSON)) || [],

That's cool.

> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:115
> +        return WI.EventBreakpoint.editable || super.editable;

Ah, now I see the use site. Maybe this can be something more descriptive, like targetSupportsEditing.

I remember wanting to put all of the supportsFooBarFeature properties on some centralized object in the past. Sometimes I want to know "does this feature exist on <old iOS version>", and in the current world I have to go figure out where it is implemented.

> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:156
> +WI.EventBreakpoint.ReferencePage = "event-breakpoints";

Similarly, I think we should have the reference pages all defined in one enum so I can easily see which ones exist as links.

[Radar WebKit Bug Importer]

<rdar://problem/67973557>

[Blaze Burg]

Comment on attachment 407115 [details]
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.toLowerCase()))

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.

[Blaze Burg]

Created attachment 407701 [details]
[VIDEO] event name suggestions don't dismiss

[Devin Rousso]

Comment on attachment 407115 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:60
>> +        return this._eventListener ? InspectorBackend.hasCommand("DOM.setBreakpointForEventListener", "options") : InspectorBackend.hasCommand("DOMDebugger.setEventBreakpoint", "options");
> 
> Please break this into separate lines. It's hard to read.

agreed

>> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:115
>> +        return WI.EventBreakpoint.editable || super.editable;
> 
> Ah, now I see the use site. Maybe this can be something more descriptive, like targetSupportsEditing.
> 
> I remember wanting to put all of the supportsFooBarFeature properties on some centralized object in the past. Sometimes I want to know "does this feature exist on <old iOS version>", and in the current world I have to go figure out where it is implemented.

I like `supportsEditing`, will change :)

>> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:156
>> +WI.EventBreakpoint.ReferencePage = "event-breakpoints";
> 
> Similarly, I think we should have the reference pages all defined in one enum so I can easily see which ones exist as links.

I personally like having it attached to the object as it makes the relationship super clear.  Also, this lets us do the fun things like `foo.constructor.ReferencePage`, which would be harder if it was an enum.  I think the consistent name of `.ReferencePage = ` should be enough to find all the links in code :)

We could have something like `WI.ReferencePage` and then have this be `WI.EventBreakpoint.ReferencePage = WI.ReferencePage.EventBreakpoints;` so that we get the best of both worlds.  What do you think of that?

>> 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.

I'm actually on the fence about this being there at all.  I feel like a placeholder of `Example: "click"` is really weird and not something we do elsewhere.  Given that there's now a "Event" label for the input, I think I'm just going to drop this.

>> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:102
>> +                    if (eventName.toLowerCase().startsWith(this._domEventNameInputElement.value.toLowerCase()))
> 
> 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.

Wow great catch!  FWIW this already happens with the existing event breakpoint popover, but it is far more pronounced with this change as there's now other focusable things in the event breakpoint popover.  I'll fix it :)

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

My thinking here was that `options` wouldn't include the `eventName` as that's determined by this popover.  The `options` is for the other stuff like `condition` and `actions`.  I'll add a `console.assert(!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.

Really?  We have similar naming all over the place.  `_codeMirror` isn't specific enough IMO and could clash with the super class (e.g. there's already a `_conditionCodeMirror` from `WI.BreakpointPopover`).  `_CodeMirrorForURL` is odd IMO because of the leading capital.  What is it about `_urlCodeMirror` that you don't like?

>> 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.

I can't make it one-line because `addAction` doesn't return the `WI.BreakpointAction` and this test modifies the `action` later on.

We could add a `createAction` but I'd really rather not add code that only exists for tests, as we don't have a way to strip that code out.

[Blaze Burg]

(In reply to Devin Rousso from comment #8)
> Comment on attachment 407115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407115&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:60
> >> +        return this._eventListener ? InspectorBackend.hasCommand("DOM.setBreakpointForEventListener", "options") : InspectorBackend.hasCommand("DOMDebugger.setEventBreakpoint", "options");
> > 
> > Please break this into separate lines. It's hard to read.
> 
> agreed
> 
> >> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:115
> >> +        return WI.EventBreakpoint.editable || super.editable;
> > 
> > Ah, now I see the use site. Maybe this can be something more descriptive, like targetSupportsEditing.
> > 
> > I remember wanting to put all of the supportsFooBarFeature properties on some centralized object in the past. Sometimes I want to know "does this feature exist on <old iOS version>", and in the current world I have to go figure out where it is implemented.
> 
> I like `supportsEditing`, will change :)
> 
> >> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:156
> >> +WI.EventBreakpoint.ReferencePage = "event-breakpoints";
> > 
> > Similarly, I think we should have the reference pages all defined in one enum so I can easily see which ones exist as links.
> 
> I personally like having it attached to the object as it makes the
> relationship super clear.  Also, this lets us do the fun things like
> `foo.constructor.ReferencePage`, which would be harder if it was an enum.  I
> think the consistent name of `.ReferencePage = ` should be enough to find
> all the links in code :)
> 
> We could have something like `WI.ReferencePage` and then have this be
> `WI.EventBreakpoint.ReferencePage = WI.ReferencePage.EventBreakpoints;` so
> that we get the best of both worlds.  What do you think of that?

That would be great. I do understand the magic uses of putting it on the constructor, however a single file is much easier to audit.

> >> 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.
> 
> I'm actually on the fence about this being there at all.  I feel like a
> placeholder of `Example: "click"` is really weird and not something we do
> elsewhere.  Given that there's now a "Event" label for the input, I think
> I'm just going to drop this.

OK

> >> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:102
> >> +                    if (eventName.toLowerCase().startsWith(this._domEventNameInputElement.value.toLowerCase()))
> > 
> > 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.
> 
> Wow great catch!  FWIW this already happens with the existing event
> breakpoint popover, but it is far more pronounced with this change as
> there's now other focusable things in the event breakpoint popover.  I'll
> fix it :)

OK

> >> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:128
> >> +        options.eventName = this._domEventNameInputElement.value;
> > 
> > Should this be options.eventName ??=  ?
> 
> My thinking here was that `options` wouldn't include the `eventName` as
> that's determined by this popover.  The `options` is for the other stuff
> like `condition` and `actions`.  I'll add a
> `console.assert(!options.eventName);`

OK

> >> Source/WebInspectorUI/UserInterface/Views/URLBreakpointPopover.js:34
> >> +        this._urlCodeMirror = null;
> > 
> > I don't like this name. Alternatives would be this._codeMirror or this._CodeMirrorForURL.
> 
> Really?  We have similar naming all over the place.  `_codeMirror` isn't
> specific enough IMO and could clash with the super class (e.g. there's
> already a `_conditionCodeMirror` from `WI.BreakpointPopover`). 
> `_CodeMirrorForURL` is odd IMO because of the leading capital.  What is it
> about `_urlCodeMirror` that you don't like?

Okay, nevermind. Maybe I've been using Objective-C for too long. (the leading capital was a typo, sorry!)

> >> 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.
> 
> I can't make it one-line because `addAction` doesn't return the
> `WI.BreakpointAction` and this test modifies the `action` later on.
> 
> We could add a `createAction` but I'd really rather not add code that only
> exists for tests, as we don't have a way to strip that code out.

OK

[Devin Rousso]

Created attachment 407726 [details]
Patch

[Blaze Burg]

Comment on attachment 407726 [details]
Patch

r=me

[EWS]

Committed r266480: <https://trac.webkit.org/changeset/266480>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407726 [details].