Bug 45114 - Web Inspector: show status message below call stack when debugger is paused
Summary: Web Inspector: show status message below call stack when debugger is paused
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: 45975
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-02 10:01 PDT by Pavel Podivilov
Modified: 2010-09-17 10:56 PDT (History)
12 users (show)

See Also:


Attachments
Screenshot. (148.91 KB, image/png)
2010-09-02 10:01 PDT, Pavel Podivilov
no flags Details
Proposed patch. (11.61 KB, patch)
2010-09-02 10:01 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Proposed patch. (27.44 KB, patch)
2010-09-06 04:40 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Fix wording. (27.43 KB, patch)
2010-09-13 03:11 PDT, Pavel Podivilov
yurys: review-
Details | Formatted Diff | Diff
All comments addressed. (29.21 KB, patch)
2010-09-14 09:51 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
What's new (12.69 KB, patch)
2010-09-14 09:52 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch (30.06 KB, patch)
2010-09-17 02:56 PDT, Pavel Podivilov
pfeldman: 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-02 10:01:34 PDT
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).
Comment 1 Pavel Podivilov 2010-09-02 10:01:57 PDT
Created attachment 66383 [details]
Proposed patch.
Comment 2 Joseph Pecoraro 2010-09-02 10:57:07 PDT
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
Comment 3 Pavel Podivilov 2010-09-02 11:48:09 PDT
(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.
Comment 4 Pavel Podivilov 2010-09-06 04:40:42 PDT
Created attachment 66626 [details]
Proposed patch.

* Updated localizedStrings.js
* check status message in DOM breakpoints test
Comment 5 Pavel Podivilov 2010-09-08 01:32:21 PDT
Hey guys,

Any comments about changing the UI? Does the screenshot looks good to you?
Comment 6 Joseph Pecoraro 2010-09-08 09:44:17 PDT
(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?
Comment 7 Pavel Podivilov 2010-09-09 09:09:08 PDT
(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?
Comment 8 Joseph Pecoraro 2010-09-09 09:45:41 PDT
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.
Comment 9 Joseph Pecoraro 2010-09-09 09:47:22 PDT
>     "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.
Comment 10 Pavel Podivilov 2010-09-13 03:11:13 PDT
Created attachment 67388 [details]
Fix wording.
Comment 11 Pavel Podivilov 2010-09-13 03:14:52 PDT
(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 12 Pavel Feldman 2010-09-13 05:18:23 PDT
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 13 Yury Semikhatsky 2010-09-13 05:18:40 PDT
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 14 Yury Semikhatsky 2010-09-13 05:19:18 PDT
Comment on attachment 67388 [details]
Fix wording.

Reverted r- by pfeldman
Comment 15 Pavel Podivilov 2010-09-14 09:51:54 PDT
Created attachment 67567 [details]
All comments addressed.
Comment 16 Pavel Podivilov 2010-09-14 09:52:46 PDT
Created attachment 67569 [details]
What's new
Comment 17 Pavel Feldman 2010-09-14 10:49:42 PDT
Comment on attachment 67567 [details]
All comments addressed.

cq- since it should contain binary changes.
Comment 18 Pavel Podivilov 2010-09-17 02:56:47 PDT
Created attachment 67895 [details]
Patch
Comment 19 Pavel Podivilov 2010-09-17 03:36:07 PDT
(In reply to comment #18)
> Created an attachment (id=67895) [details]
> Patch

Uploaded with "webkit-patch upload" in order to handle binary diff correctly.
Comment 20 Joseph Pecoraro 2010-09-17 10:00:47 PDT
(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.
Comment 21 Pavel Feldman 2010-09-17 10:16:31 PDT
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
Comment 22 WebKit Review Bot 2010-09-17 10:44:34 PDT
http://trac.webkit.org/changeset/67721 might have broken GTK Linux 32-bit Release