Bug 82967 – Web Inspector: break on DOM node insertion only once per operation, not once per inserted node

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

[Adam Klein]

Web Inspector: break on DOM node insertion only once per operation, not once per inserted node

[Adam Klein]

Created attachment 135221 [details]
Patch

[Ojan Vafai]

Looks good to me. Leaving r? so the inspector folks have a chance to r-/r+.

[Adam Klein]

(In reply to comment #2)
> Looks good to me. Leaving r? so the inspector folks have a chance to r-/r+.

Thanks. To inspector folks: I'd especially appreciate suggestions on a test.

[Adam Klein]

Can someone from the inspector side take a look at this? It's blocking me for moving forward with the DOM refactoring in bug 82631.

[Pavel Feldman]

Comment on attachment 135221 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +        I tried to write a test for this in inspector/debugger/dom-breakpoints.html,

Your change looks good. The way I would test it is I would use a snippet like:

testNode.innerHTML = "<br><br>";
debugger;

And would test that I stop on the innerHTML = line first, then I would click continue and check that I stopped on the next line.

[Adam Klein]

(In reply to comment #5)
> (From update of attachment 135221 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135221&action=review
> 
> > Source/WebCore/ChangeLog:23
> > +        I tried to write a test for this in inspector/debugger/dom-breakpoints.html,
> 
> Your change looks good. The way I would test it is I would use a snippet like:
> 
> testNode.innerHTML = "<br><br>";
> debugger;
> 
> And would test that I stop on the innerHTML = line first, then I would click continue and check that I stopped on the next line.

That test manually works fine, but coming up with the right set of incantations around evaluateInPageWithoutTimeout, resumeExecution, and waitUntilPaused is leaving me mystified with test timeouts. Will try in a standalone test...

[Adam Klein]

Created attachment 135639 [details]
Patch with test

[Ojan Vafai]

Comment on attachment 135639 [details]
Patch with test

Yay!

[Pavel Feldman]

Comment on attachment 135639 [details]
Patch with test

Clearing cq+ while I am figuring out whether the test is actually right.

[Pavel Feldman]

Comment on attachment 135639 [details]
Patch with test

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

False alarm, I think it is Ok. Thanks for fixing it, sorry for moving it back to the end of the cq!

> LayoutTests/inspector/debugger/dom-breakpoints.html:55
> +    debugger;

Oh, ok, you need it because of the runAfterPendingDispatches used in waitUntilPausedAndDumpStack.

[Adam Klein]

(In reply to comment #10)
> (From update of attachment 135639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135639&action=review
> 
> False alarm, I think it is Ok. Thanks for fixing it, sorry for moving it back to the end of the cq!
> 
> > LayoutTests/inspector/debugger/dom-breakpoints.html:55
> > +    debugger;
> 
> Oh, ok, you need it because of the runAfterPendingDispatches used in waitUntilPausedAndDumpStack.

Thanks for double-checking, I wasn't sure which thing in that helper was messing me up, but I could tell something was.

[Adam Klein]

Committed r113230: <http://trac.webkit.org/changeset/113230>