Bug 45114 – Web Inspector: show status message below call stack when debugger is paused

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

[Pavel Podivilov]

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

[Pavel Podivilov]

Created attachment 66383 [details]
Proposed patch.

[Joseph Pecoraro]

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

[Pavel Podivilov]

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

[Pavel Podivilov]

Created attachment 66626 [details]
Proposed patch.

* Updated localizedStrings.js
* check status message in DOM breakpoints test

[Pavel Podivilov]

Hey guys,

Any comments about changing the UI? Does the screenshot looks good to you?

[Joseph Pecoraro]

(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?

[Pavel Podivilov]

(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?

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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

[Pavel Podivilov]

Created attachment 67388 [details]
Fix wording.

[Pavel Podivilov]

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

[Pavel Feldman]

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.

[Yury Semikhatsky]

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?

[Yury Semikhatsky]

Comment on attachment 67388 [details]
Fix wording.

Reverted r- by pfeldman

[Pavel Podivilov]

Created attachment 67567 [details]
All comments addressed.

[Pavel Podivilov]

Created attachment 67569 [details]
What's new

[Pavel Feldman]

Comment on attachment 67567 [details]
All comments addressed.

cq- since it should contain binary changes.

[Pavel Podivilov]

Created attachment 67895 [details]
Patch

[Pavel Podivilov]

(In reply to comment #18)
> Created an attachment (id=67895) [details]
> Patch

Uploaded with "webkit-patch upload" in order to handle binary diff correctly.

[Joseph Pecoraro]

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

[Pavel Feldman]

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

[WebKit Review Bot]

http://trac.webkit.org/changeset/67721 might have broken GTK Linux 32-bit Release