Bug 138991

Summary: Web Inspector: Pause Reason Improvements (Breakpoint, Debugger Statement, Pause on Next Statement)
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: burg, graouts, joepeck, jonowells, mark.lam, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[IMAGE] Images
none
[PATCH] PROGRESS: Needs UI Polish
none
[IMAGE] Consistent "Pause Reason" title
none
[IMAGE] "Pause Reason – %s" title
none
[IMAGE] Variable Title based on Pause Reason
none
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix
timothy: review+
[IMAGE] Pause Reason - Breakpoint none

Description Joseph Pecoraro 2014-11-21 17:15:32 PST
* 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
Comment 1 Radar WebKit Bug Importer 2014-11-21 17:15:43 PST
<rdar://problem/19065627>
Comment 2 Joseph Pecoraro 2014-11-21 21:58:33 PST
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
Comment 3 Joseph Pecoraro 2014-11-21 22:00:26 PST
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.
Comment 4 Joseph Pecoraro 2014-12-02 14:31:22 PST
Created attachment 242445 [details]
[IMAGE] Consistent "Pause Reason" title
Comment 5 Joseph Pecoraro 2014-12-02 14:31:48 PST
Created attachment 242446 [details]
[IMAGE] "Pause Reason – %s" title
Comment 6 Joseph Pecoraro 2014-12-02 14:32:16 PST
Created attachment 242448 [details]
[IMAGE] Variable Title based on Pause Reason
Comment 7 Joseph Pecoraro 2014-12-02 14:32:55 PST
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.
Comment 8 Joseph Pecoraro 2014-12-02 18:54:48 PST
Created attachment 242476 [details]
[PATCH] Proposed Fix

Improved UI for pausing on a breakpoint.
Comment 9 Joseph Pecoraro 2015-01-06 18:44:13 PST
Created attachment 244120 [details]
[PATCH] Proposed Fix

Rebased. Tests still pass. Image of pausing on a Breakpoint to come next.
Comment 10 Joseph Pecoraro 2015-01-06 18:44:37 PST
Created attachment 244121 [details]
[IMAGE] Pause Reason - Breakpoint
Comment 11 Timothy Hatcher 2015-01-06 22:19:20 PST
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.
Comment 12 Brian Burg 2015-01-07 07:28:00 PST
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
Comment 13 Joseph Pecoraro 2015-01-08 15:17:25 PST
http://trac.webkit.org/changeset/178137