Bug 169479 - Web Inspector: Show additional pause reason details for DOM "subtree modified" breakpoint
Summary: Web Inspector: Show additional pause reason details for DOM "subtree modified...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-10 12:01 PST by Matt Baker
Modified: 2017-04-03 20:19 PDT (History)
4 users (show)

See Also:


Attachments
[Image] pause reason UI (344.09 KB, image/png)
2017-03-10 12:01 PST, Matt Baker
no flags Details
[Image] pause reason v1 (638.84 KB, image/png)
2017-03-17 19:56 PDT, Matt Baker
no flags Details
[Image] pause reason v2 (104.60 KB, image/png)
2017-03-17 19:57 PDT, Matt Baker
no flags Details
Patch (6.21 KB, patch)
2017-03-18 02:17 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2017-03-10 12:01:07 PST
Created attachment 304061 [details]
[Image] pause reason UI

Summary:
Show additional pause reason details for DOM "subtree modified" breakpoint. Make use of the `insertion` and `targetNode` info sent from the backend.

insertion (boolean)
 - disambiguates a subtree modification (node inserted or removed)

taretNode (object)
 - if insertion = true, remote object payload for the DOM node owning the breakpoint (how is this helpful?)
 - if insertion = false, remote object payload for the DOM node being removed

Notes:
Attached is a WIP UI for the insertion = false case.
Comment 1 Joseph Pecoraro 2017-03-10 12:54:41 PST
So when we pause for exceptions we expose a `$exception` value in the console. Something similar might be neat here as well, but I don't think any name will be obvious. We could show it in the sidebar kind of like your image.

I agree the targetNode should probably always be the node being inserted/removed. We should already know the node owning the breakpoint.
Comment 2 Matt Baker 2017-03-12 12:33:06 PDT
(In reply to comment #0)
> Created attachment 304061 [details]
> [Image] pause reason UI
> 
> Summary:
> Show additional pause reason details for DOM "subtree modified" breakpoint.
> Make use of the `insertion` and `targetNode` info sent from the backend.
> 
> insertion (boolean)
>  - disambiguates a subtree modification (node inserted or removed)
> 
> taretNode (object)
>  - if insertion = true, remote object payload for the DOM node owning the
> breakpoint (how is this helpful?)

I spoke too soon. The comment on InspectorDOMDebuggerAgent.cpp:307 explains: "For inheritable breakpoint types, target node isn't always the same as the node that owns a breakpoint."

The following scenario would be an example:

<body> <-- set a 'Subtree Modified' breakpoint here
    <div>
        <p>...</p> <--- Insert a new <p> here
    </div>
</body>

target node: "<div>"
owner node: "<body>"
Comment 3 Matt Baker 2017-03-12 12:41:52 PDT
For the following DOM tree with a 'Subtree Modified' breakpoint set on body:

> <body>
      <div>
          <p>...</p>
      </div>
  </body>

Removing the paragraph should break and show:

[P] Triggered DOM Breakpoint
Subtree Modified
Remove descendant <p>

Inserting a new paragraph at the same spot should break and show:

[P] Triggered DOM Breakpoint
Subtree Modified
Append child to <div>
Comment 4 Matt Baker 2017-03-17 19:56:57 PDT
Created attachment 304849 [details]
[Image] pause reason v1
Comment 5 Matt Baker 2017-03-17 19:57:33 PDT
Created attachment 304850 [details]
[Image] pause reason v2
Comment 6 Matt Baker 2017-03-18 02:17:43 PDT
Created attachment 304866 [details]
Patch
Comment 7 WebKit Commit Bot 2017-04-03 20:19:12 PDT
Comment on attachment 304866 [details]
Patch

Clearing flags on attachment: 304866

Committed r214861: <http://trac.webkit.org/changeset/214861>
Comment 8 WebKit Commit Bot 2017-04-03 20:19:14 PDT
All reviewed patches have been landed.  Closing bug.