Web Inspector: Pause Reason Improvements (Breakpoint, Debugger Statement, Pause on Next Statement)

[Joseph Pecoraro]

* SUMMARY
There are many different ways to pause in the debugger that we don't give the user information about. Such as Pausing because of a breakpoint, a debugger statement, or immediately pausing via the pause button in the UI. Web Inspector should give information about these pause events.

* TEST
1. <button id="x">Debugger Statement</button>
2. <button id="y">Breakpoint</button>
3. <script>
4. window.x.onclick = function() { debugger; }
5. window.y.onclick = function() {
6.     var x = 10;
7. }
8. window.onclick = function() { console.log("click"); }
9. </script>

* STEPS TO REPRODUCE
1. Inspect test page
2. Set a breakpoint on line (6)
3. Click the Debugger Statement button
  => pause on debugger statement, expected pause reason
4. Continue.
5. Click the Breakpoint button
  => pause on breakpoint, expected pause reason
6. Continue.
7. Click the Pause button in Web Inspector
8. Click on the page, no button
  => paused immediately, expected pause reason

[Radar WebKit Bug Importer]

<rdar://problem/19065627>

[Joseph Pecoraro]

Created attachment 242110 [details]
[IMAGE] Images

These images are what I have in this patch. This is certainly NOT final UI, it was mostly to make sure functionality was working as expected.

See also the screenshots on bug 63096.

I discussed the UI a bit with others, here are some thoughts being thrown around.

  1. Keep the section title name static, "Pause Reason".
    - easier to explain the UI to someone.
    - existing Assertion/Exception/CSP Violation titles could move into the section
    - for exceptions, that could also explain that we are showing the thrown object description
  2. Goto arrow could get confused with Continue button.
  3. We could show the location in more places.
    - the goto arrow tooltip shows it, but it maybe be nice to know at a glance

[Joseph Pecoraro]

Created attachment 242111 [details]
[PATCH] PROGRESS: Needs UI Polish

This patch gets things working in the backend and includes tests. We just need to polish the UI (and ChangeLogs) before review.

[Joseph Pecoraro]

Created attachment 242445 [details]
[IMAGE] Consistent "Pause Reason" title

[Joseph Pecoraro]

Created attachment 242446 [details]
[IMAGE] "Pause Reason – %s" title

[Joseph Pecoraro]

Created attachment 242448 [details]
[IMAGE] Variable Title based on Pause Reason

[Joseph Pecoraro]

Given these few variations, I do actually think that keeping the title always "Pause Reason" makes the most sense. I'll move in that direction.

[Joseph Pecoraro]

Created attachment 242476 [details]
[PATCH] Proposed Fix

Improved UI for pausing on a breakpoint.

[Joseph Pecoraro]

Created attachment 244120 [details]
[PATCH] Proposed Fix

Rebased. Tests still pass. Image of pausing on a Breakpoint to come next.

[Joseph Pecoraro]

Created attachment 244121 [details]
[IMAGE] Pause Reason - Breakpoint

[Timothy Hatcher]

Comment on attachment 244120 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=244120&action=review

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:143
> +    RefPtr<Inspector::Protocol::Debugger::AssertPauseReason> reason = Inspector::Protocol::Debugger::AssertPauseReason::create();

auto might make this read better.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:152
> +    RefPtr<Inspector::Protocol::Debugger::CSPViolationPauseReason> reason = Inspector::Protocol::Debugger::CSPViolationPauseReason::create()
> +        .setDirective(directiveText);

Ditto.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:164
> +    RefPtr<Inspector::Protocol::Debugger::BreakpointPauseReason> reason = Inspector::Protocol::Debugger::BreakpointPauseReason::create()
> +        .setBreakpointId(it->value);

Ditto.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:431
>      RefPtr<Inspector::Protocol::Debugger::Location> location = Inspector::Protocol::Debugger::Location::create()

Ditto.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:650
> +            // FIXME: We should include the assertion condition string.

Yes!

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:656
> +            }            
> +            this._pauseReasonTextRow.text = WebInspector.UIString("Assertion Failed");

Nit: Newline in the middle would look nicer.

[Brian Burg]

Comment on attachment 244120 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=244120&action=review

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:143
>> +    RefPtr<Inspector::Protocol::Debugger::AssertPauseReason> reason = Inspector::Protocol::Debugger::AssertPauseReason::create();
> 
> auto might make this read better.

Please make the return type be 'RefPtr'. Using a local with type 'auto' will also make it possible to move RefPtr->Ref without changing the callsite.

(I wish we didn't store aux data as an untyped InspectorObject. But, there's no easy way to implement a disjoint union with inspector protocol types.)

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:144
> +    if (!message.isNull())

Could message still be empty? I always get confused about .isNull() .isEmpty()

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:589
> +    ASSERT(!injectedScript.hasNoValue());

ASSERT_ARG

[Joseph Pecoraro]

http://trac.webkit.org/changeset/178137