Web Inspector: add event listener breakpoints sidebar pane
We should really consider consolidating all the breakpoints under a single Breakpoints sidebar pane.
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!
Created attachment 69078 [details] Windows 7 screenshot
Created attachment 69642 [details] Mac screenshot.
Created attachment 69643 [details] Patch.
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?
Created attachment 69778 [details] Patch.
> 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.
(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.
(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 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.
Committed r69203: <http://trac.webkit.org/changeset/69203>
(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 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.