Bug 46738 – Web Inspector: add event listener breakpoints sidebar pane

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Podivilov]

Web Inspector: add event listener breakpoints sidebar pane

[Timothy Hatcher]

We should really consider consolidating all the breakpoints under a single Breakpoints sidebar pane.

[Pavel Feldman]

Today we have following types of breakpoints:
- JS Breakpoints
- DOM Breakpoints
- XHR breakpoints
These are the ones you can manage: add / remove / set conditions and such.

In addition to that, under this bug, we will have event breakpoints that are the tree of the events you can check to stop upon. This is more of a Pause on next... thingy.

Good designs for combining managed breakpoints are welcome!

[Pavel Podivilov]

Created attachment 69078 [details]
Windows 7 screenshot

[Pavel Podivilov]

Created attachment 69642 [details]
Mac screenshot.

[Pavel Podivilov]

Created attachment 69643 [details]
Patch.

[Joseph Pecoraro]

Comment on attachment 69643 [details]
Patch.

Took a quick look. View in context: https://bugs.webkit.org/attachment.cgi?id=69643&action=review

> WebCore/inspector/front-end/BreakpointsSidebarPane.js:274
> +    _populate: function()
> +    {
> +
> +        var categories = {
> +            "Mouse": ["click", "dblclick", "mousedown", "mouseup", "mouseover", "mousemove", "mouseout", "mousewheel"],
> +            "Keyboard": ["keydown", "keypress", "keyup"],
> +            "HTML frame/object": ["load", "unload", "abort", "error", "resize", "scroll"],
> +            "HTML form": ["select", "change", "submit", "reset", "focus", "blur"],
> +            "User interface": ["DOMFocusIn", "DOMFocusOut", "DOMActivate"],
> +            "Mutation": ["DOMAttrModified", "DOMCharacterDataModified", "DOMNodeInserted", "DOMNodeInsertedIntoDocument", "DOMNodeRemoved",
> +                         "DOMNodeRemovedFromDocument", "DOMSubtreeModified"]
> +        };
> +
> +        for (var category in categories) {
> +            var categoryTreeElement = new TreeElement(category);
> +            this.categoriesTreeOutline.appendChild(categoryTreeElement);
> +            categoryTreeElement.listItemElement.addStyleClass("event-category");
> +            categoryTreeElement.selectable = true;
> +

NIT: Extra newline up top.

I think these keys should be localized, since they are the title of TreeElement's displayed to the user.
Would be easy to solve with along the lines of: (and adding the strings.

  var categoryTreeElement = new TreeElement(WebInspector.UIString(category));


> WebCore/inspector/front-end/EventListenersSidebarPane.js:186
> +    this.propertiesElement.className = "event-properties properties-tree source-code"; /* Changed from "properties" */

How does this relate to your other changes? How does this affect the Event
Listeners sidebar pane in the Element's panel?


> WebCore/inspector/front-end/PropertiesSection.js:35
> +    this.propertiesElement.className = "properties properties-tree source-code";

Same comment. How does this affect the properties pane in the Elements panel?

[Pavel Podivilov]

Created attachment 69778 [details]
Patch.

[Pavel Podivilov]

> NIT: Extra newline up top.
> 
> I think these keys should be localized, since they are the title of TreeElement's displayed to the user.
> Would be easy to solve with along the lines of: (and adding the strings.
> 
>   var categoryTreeElement = new TreeElement(WebInspector.UIString(category));
> 
done

> 
> > WebCore/inspector/front-end/EventListenersSidebarPane.js:186
> > +    this.propertiesElement.className = "event-properties properties-tree source-code"; /* Changed from "properties" */
> 
> How does this relate to your other changes? How does this affect the Event
> Listeners sidebar pane in the Element's panel?
> 
> 
> > WebCore/inspector/front-end/PropertiesSection.js:35
> > +    this.propertiesElement.className = "properties properties-tree source-code";
> 
> Same comment. How does this affect the properties pane in the Elements panel?

It doesn't affect the panels, it's just a refactoring of css.

[Joseph Pecoraro]

(In reply to comment #8)
> > Would be easy to solve with along the lines of: (and adding the strings.)
> > 
> >   var categoryTreeElement = new TreeElement(WebInspector.UIString(category));
>
> done

Were the new strings added to localizedStrings.js? The file didn't appear in the
patch. Remember `git diff --binary`would show the file. Thanks for this fix.

[Pavel Podivilov]

(In reply to comment #9)
> (In reply to comment #8)
> > > Would be easy to solve with along the lines of: (and adding the strings.)
> > > 
> > >   var categoryTreeElement = new TreeElement(WebInspector.UIString(category));
> >
> > done
> 
> Were the new strings added to localizedStrings.js? The file didn't appear in the
> patch. Remember `git diff --binary`would show the file. Thanks for this fix.

Removed cq? from patch. Do you think it's ok to update localizedStrings.js just before committing (not attach it here)?

[Yury Semikhatsky]

Comment on attachment 69778 [details]
Patch.

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

> WebCore/inspector/front-end/BreakpointManager.js:150
> +        breakpoint.compareTo = function(other)

We should introduce a dedicated class for XHR breakpoints and define the method on it.

> WebCore/inspector/front-end/BreakpointManager.js:292
> +            if (breakpointId)

it'd be harmless to do the assignment even if breakpointId is undefined.

[Pavel Podivilov]

Committed r69203: <http://trac.webkit.org/changeset/69203>

[Pavel Podivilov]

(In reply to comment #11)
> (From update of attachment 69778 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69778&action=review
> 
> > WebCore/inspector/front-end/BreakpointManager.js:150
> > +        breakpoint.compareTo = function(other)
> 
> We should introduce a dedicated class for XHR breakpoints and define the method on it.

Let me fix it in a separate patch.

> 
> > WebCore/inspector/front-end/BreakpointManager.js:292
> > +            if (breakpointId)
> 
> it'd be harmless to do the assignment even if breakpointId is undefined.

breakpointId is 0 if breakpoint was not set on backend.

[Yury Semikhatsky]

Comment on attachment 69778 [details]
Patch.

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

>>> WebCore/inspector/front-end/BreakpointManager.js:292
>>> +            if (breakpointId)
>> 
>> it'd be harmless to do the assignment even if breakpointId is undefined.
> 
> breakpointId is 0 if breakpoint was not set on backend.

Then we should mark this breakpoint as invalid somehow, having _id === 0 could be one of the possibilities.