Bug 82967

Summary: Web Inspector: break on DOM node insertion only once per operation, not once per inserted node
Product: WebKit Reporter: Adam Klein <adamk>
Component: Web Inspector (Deprecated)Assignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: loislo, ojan, pfeldman, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82631    
Attachments:
Description Flags
Patch
none
Patch with test ojan: review+

Description Adam Klein 2012-04-02 16:22:12 PDT
Web Inspector: break on DOM node insertion only once per operation, not once per inserted node
Comment 1 Adam Klein 2012-04-02 16:32:35 PDT
Created attachment 135221 [details]
Patch
Comment 2 Ojan Vafai 2012-04-02 16:53:16 PDT
Looks good to me. Leaving r? so the inspector folks have a chance to r-/r+.
Comment 3 Adam Klein 2012-04-02 16:55:31 PDT
(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.
Comment 4 Adam Klein 2012-04-04 09:28:27 PDT
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.
Comment 5 Pavel Feldman 2012-04-04 09:55:16 PDT
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.
Comment 6 Adam Klein 2012-04-04 10:36:45 PDT
(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...
Comment 7 Adam Klein 2012-04-04 11:27:42 PDT
Created attachment 135639 [details]
Patch with test
Comment 8 Ojan Vafai 2012-04-04 11:46:50 PDT
Comment on attachment 135639 [details]
Patch with test

Yay!
Comment 9 Pavel Feldman 2012-04-04 11:53:30 PDT
Comment on attachment 135639 [details]
Patch with test

Clearing cq+ while I am figuring out whether the test is actually right.
Comment 10 Pavel Feldman 2012-04-04 11:58:54 PDT
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.
Comment 11 Adam Klein 2012-04-04 12:01:28 PDT
(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.
Comment 12 Adam Klein 2012-04-04 13:10:44 PDT
Committed r113230: <http://trac.webkit.org/changeset/113230>