Bug 46738 - Web Inspector: add event listener breakpoints sidebar pane
Summary: Web Inspector: add event listener breakpoints sidebar pane
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-28 11:06 PDT by Pavel Podivilov
Modified: 2010-10-06 23:50 PDT (History)
9 users (show)

See Also:


Attachments
Windows 7 screenshot (89.96 KB, image/png)
2010-09-28 11:40 PDT, Pavel Podivilov
no flags Details
Mac screenshot. (192.41 KB, image/png)
2010-10-04 09:14 PDT, Pavel Podivilov
no flags Details
Patch. (16.59 KB, patch)
2010-10-04 09:21 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch. (19.64 KB, patch)
2010-10-05 06:52 PDT, Pavel Podivilov
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2010-09-28 11:06:41 PDT
Web Inspector: add event listener breakpoints sidebar pane
Comment 1 Timothy Hatcher 2010-09-28 11:13:46 PDT
We should really consider consolidating all the breakpoints under a single Breakpoints sidebar pane.
Comment 2 Pavel Feldman 2010-09-28 11:32:45 PDT
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!
Comment 3 Pavel Podivilov 2010-09-28 11:40:24 PDT
Created attachment 69078 [details]
Windows 7 screenshot
Comment 4 Pavel Podivilov 2010-10-04 09:14:50 PDT
Created attachment 69642 [details]
Mac screenshot.
Comment 5 Pavel Podivilov 2010-10-04 09:21:00 PDT
Created attachment 69643 [details]
Patch.
Comment 6 Joseph Pecoraro 2010-10-04 09:58:42 PDT
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?
Comment 7 Pavel Podivilov 2010-10-05 06:52:30 PDT
Created attachment 69778 [details]
Patch.
Comment 8 Pavel Podivilov 2010-10-05 06:55:13 PDT
> 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.
Comment 9 Joseph Pecoraro 2010-10-05 09:53:51 PDT
(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.
Comment 10 Pavel Podivilov 2010-10-05 10:10:10 PDT
(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)?
Comment 11 Yury Semikhatsky 2010-10-06 09:00:14 PDT
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.
Comment 12 Pavel Podivilov 2010-10-06 09:48:55 PDT
Committed r69203: <http://trac.webkit.org/changeset/69203>
Comment 13 Pavel Podivilov 2010-10-06 09:50:48 PDT
(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.
Comment 14 Yury Semikhatsky 2010-10-06 23:50:27 PDT
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.