Created attachment 66382 [details] Screenshot. Web Inspector: show status message below call stack when debugger is paused. When pausing on DOM mutation event such as subtree modification, it's necessary to notify user about why we've paused. For JavaScript events it's usually obvious, but we may want to add status messages for JavaScript events for consistency. The list of status messages for discussion: JavaScript events: * "Paused." * "Paused on debugger statement." * "Paused on javascript breakpoint." * "Stepped over." * "Stepped into." * "Stepped out." * "Paused on exception." * "Paused on uncaught exception." DOM events: * "Paused on "Subtree Modified" breakpoint set on <node>, because new child was added to it's descendant <target>." * "Paused on "Subtree Modified" breakpoint set on <node>, because new child was added to that node." * "Paused on "Subtree Modified" breakpoint set on <node>, because because it's descendant <target> was removed." * "Paused on "Node Removed" breakpoint set on <node>." * "Paused on "Attribute Modified" breakpoint set on <node>." <node> and <target> are clickable node references (see screenshot).
Created attachment 66383 [details] Proposed patch.
Comment on attachment 66383 [details] Proposed patch. Great idea! Some quick observations on the patch. > General Comments Missing a ChangeLog! > diff --git WebCore/inspector/InspectorDebuggerAgent.cpp > - details->setValue("reason", m_breakProgramReason); > + details->setValue("status", m_breakProgramReason); Did anything use "reason" before? I did a search and didn't see anything. > diff WebCore/inspector/front-end/CallStackSidebarPane.js > + format = WebInspector.UIString("Paused on \"%s\" breakpoint set on %s, because new child was added to it's descendant %s."); > + format = WebInspector.UIString("Paused on \"%s\" breakpoint set on %s, because new child was added to that node."); > + format = WebInspector.UIString("Paused on \"%s\" breakpoint set on %s, because it's descendant %s was removed."); > + format = WebInspector.UIString("Paused on \"%s\" breakpoint set on %s."); These look like new UIStrings. They should be added to WebCore/English.lproj/localizedStrings.js. My guess is that they were added, but your git diff didn't add the binary delta. When you create a diff to put up for review make sure you pass git diff the --binary switch to include binary file deltas. > + var formatters = { > + s: function(substitution) > + { > + return substitution; > + } > + }; > + function append(a, b) > + { > + if (typeof b === "string") > + b = document.createTextNode(b); > + statusElement.appendChild(b); > + } > + String.format(format, substitutions, formatters, "", append); I'm sorry for not investigating this on my own. What is the reason for this using a custom formatter? Can you use the standard formatter found in utilities.js? String.standardFormatters
(In reply to comment #2) > (From update of attachment 66383 [details]) > Great idea! Some quick observations on the patch. > > > General Comments > > Missing a ChangeLog! > > > > diff --git WebCore/inspector/InspectorDebuggerAgent.cpp > > - details->setValue("reason", m_breakProgramReason); > > + details->setValue("status", m_breakProgramReason); > > Did anything use "reason" before? I did a search and didn't see anything. No, it actually was not used before. It was added at backend side, but will be used by fronted only after this change. BTW, this patch is based on another one, which is not landed yet (see https://bugs.webkit.org/show_bug.cgi?id=44679), sorry for confusion. > > > diff WebCore/inspector/front-end/CallStackSidebarPane.js > > + format = WebInspector.UIString("Paused on \"%s\" breakpoint set on %s, because new child was added to it's descendant %s."); > > + format = WebInspector.UIString("Paused on \"%s\" breakpoint set on %s, because new child was added to that node."); > > + format = WebInspector.UIString("Paused on \"%s\" breakpoint set on %s, because it's descendant %s was removed."); > > + format = WebInspector.UIString("Paused on \"%s\" breakpoint set on %s."); > > These look like new UIStrings. They should be added to WebCore/English.lproj/localizedStrings.js. > My guess is that they were added, but your git diff didn't add the binary delta. When you > create a diff to put up for review make sure you pass git diff the --binary switch to > include binary file deltas. Let me update localizedStrings.js when status messages are approved (corrections from native speakers are welcome). > > > > + var formatters = { > > + s: function(substitution) > > + { > > + return substitution; > > + } > > + }; > > + function append(a, b) > > + { > > + if (typeof b === "string") > > + b = document.createTextNode(b); > > + statusElement.appendChild(b); > > + } > > + String.format(format, substitutions, formatters, "", append); > > I'm sorry for not investigating this on my own. What is the reason > for this using a custom formatter? Can you use the standard formatter > found in utilities.js? > > String.standardFormatters Standard "s" formatter checks substitution type, and if it's an object, returns substitution.description, which is inappropriate here.
Created attachment 66626 [details] Proposed patch. * Updated localizedStrings.js * check status message in DOM breakpoints test
Hey guys, Any comments about changing the UI? Does the screenshot looks good to you?
(In reply to comment #5) > Any comments about changing the UI? Does the screenshot looks good to you? I thought it was very cool. My initial thought was to put it at the top of the callstack, because (1) that is where it is paused and (2) it will make the message always visible; and the message it very useful. Did you do any experimenting with the positioning?
(In reply to comment #6) > (In reply to comment #5) > > Any comments about changing the UI? Does the screenshot looks good to you? > > I thought it was very cool. My initial thought was to put it at the top of the callstack, > because (1) that is where it is paused and (2) it will make the message always visible; > and the message it very useful. Did you do any experimenting with the positioning? In the initial discussion between Pavel and Timothy it was decided to put status below call stack. Personally, I think that way it looks better, though not so convenient (won't be visible if call stack is large). BTW, are you ok with status messages wording?
Here is how I would have written them. Note I have added "a" or "an" to most of the statements. I guess this could be personal taste though. > * "Paused on debugger statement." "Paused on a `debugger` statement." Give 'debugger' a special format? > * "Paused on javascript breakpoint." "Paused on breakpoint #." or "Paused on a breakpoint." JavaScript in text displayed to the user should always be "JavaScript". However in this case I don't think the word is needed, since we know we are in JavaScript. > * "Paused on exception." "Paused on an exception." or "Paused on a <exception or error type>." An example of the latter being: "Paused on a TypeError exception." > * "Paused on uncaught exception." "Paused on an uncaught exception." or "Paused on an uncaught <exception or error type>." > DOM events: > * "Paused on "Subtree Modified" breakpoint set on <node>, because new child was added to it's descendant <target>." > * "Paused on "Subtree Modified" breakpoint set on <node>, because new child was added to that node." "Paused on a ..., because the new child was added to its ..." ^ ^^^ ^^^ "It's" becomes "it is". "Its" means ownership. This is a weird case in English, because normally the apostrophe means ownership. > * "Paused on "Subtree Modified" breakpoint set on <node>, because because it's descendant <target> was removed." No double "because". "It's" again.
> "Paused on a ..., because the new child was added to its ..." > ^ ^^^ ^^^ Arg. This should have been, "because a new child was added to its ...". Not "the". Ugh, sorry for the confusion.
Created attachment 67388 [details] Fix wording.
(In reply to comment #9) > > "Paused on a ..., because the new child was added to its ..." > > ^ ^^^ ^^^ > > Arg. This should have been, "because a new child was added to its ...". > Not "the". Ugh, sorry for the confusion. Thanks for corrections! JavaScript events: * "Paused." * "Paused on a <debugger> statement." * "Paused on a breakpoint." * "Stepped over." * "Stepped into." * "Stepped out." * "Paused on an exception." * "Paused on an uncaught exception." DOM events: * "Paused on a "Subtree Modified" breakpoint set on <node>, because a new child was added to its descendant <target>." * "Paused on a "Subtree Modified" breakpoint set on <node>, because a new child was added to that node." * "Paused on a "Subtree Modified" breakpoint set on <node>, because because its descendant <target> was removed." * "Paused on a "Node Removed" breakpoint set on <node>." * "Paused on a "Attribute Modified" breakpoint set on <node>."
Comment on attachment 67388 [details] Fix wording. View in context: https://bugs.webkit.org/attachment.cgi?id=67388&action=prettypatch Overall looks good. > WebCore/inspector/front-end/CallStackSidebarPane.js:89 > + var breakpointOwnerLink = WebInspector.panels.elements.linkifyNodeReference(breakpointOwnerNode); breakpointOwnerNode can be undefined here. > WebCore/inspector/front-end/CallStackSidebarPane.js:96 > + var targetNodeLink = WebInspector.panels.elements.linkifyNodeReference(targetNode); Ditto. > WebCore/inspector/front-end/CallStackSidebarPane.js:99 > + format = WebInspector.UIString("Paused on a \"%s\" breakpoint set on %s, because a new child was added to its descendant %s."); Nit: I'd introduce WebInspector.formatLocalized beside UIString instead.
Comment on attachment 67388 [details] Fix wording. View in context: https://bugs.webkit.org/attachment.cgi?id=67388&action=prettypatch > LayoutTests/inspector/dom-breakpoints.html:28 > + test_InsertChild = {}; style nit: we don't use underscore in the middle of variable name > LayoutTests/inspector/dom-breakpoints.html:161 > + InspectorTest.addResult("line: " + callFrames[0].line + ", function: " + callFrames[0].functionName); Wy not use captureCallStack instead?
Comment on attachment 67388 [details] Fix wording. Reverted r- by pfeldman
Created attachment 67567 [details] All comments addressed.
Created attachment 67569 [details] What's new
Comment on attachment 67567 [details] All comments addressed. cq- since it should contain binary changes.
Created attachment 67895 [details] Patch
(In reply to comment #18) > Created an attachment (id=67895) [details] > Patch Uploaded with "webkit-patch upload" in order to handle binary diff correctly.
(In reply to comment #19) > (In reply to comment #18) > > Created an attachment (id=67895) [details] [details] > > Patch > > Uploaded with "webkit-patch upload" in order to handle binary diff correctly. It is still a (delta) git binary diff, which I'm pretty sure the commit-queue (running svn-apply and svn) doesn't like: <http://webkit.org/b/38864> svn-apply: should support git binary delta diffs Meaning this would need to be landed manually.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog M LayoutTests/http/tests/inspector/inspector-test2.js M LayoutTests/inspector/dom-breakpoints-expected.txt M LayoutTests/inspector/dom-breakpoints.html M WebCore/ChangeLog M WebCore/English.lproj/localizedStrings.js M WebCore/inspector/InspectorDOMAgent.cpp M WebCore/inspector/InspectorDOMAgent.h M WebCore/inspector/InspectorDebuggerAgent.cpp M WebCore/inspector/front-end/CallStackSidebarPane.js M WebCore/inspector/front-end/ElementsPanel.js M WebCore/inspector/front-end/ScriptsPanel.js M WebCore/inspector/front-end/inspector.css M WebCore/inspector/front-end/inspector.js Committed r67721
http://trac.webkit.org/changeset/67721 might have broken GTK Linux 32-bit Release