Break on window close when the inspector is open

Break on window close when the inspector is open

BUG=171447
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179121

[pfeldman, 2014-07-22 08:57:40]

Overall this looks good.

https://codereview.chromium.org/401523004/diff/20001/LayoutTests/inspector-en...
File
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html
(right):

https://codereview.chromium.org/401523004/diff/20001/LayoutTests/inspector-en...
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:46:
log("Paused in breakpoint");
You should dump the stack here to test that it is available.

https://codereview.chromium.org/401523004/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sources/BreakpointsSidebarPane.js (right):

https://codereview.chromium.org/401523004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sources/BreakpointsSidebarPane.js:609: if
(eventName.indexOf(':') === -1) {
This does not look particularly nice. We should either prepend everything with
instrumentation: above and get rid of the isInstrumentation flag or place it
into a new categoty.

[venuatu, 2014-07-24 13:42:03]

On 2014/07/22 08:57:40, pfeldman wrote:
> Overall this looks good.
> 
>
https://codereview.chromium.org/401523004/diff/20001/LayoutTests/inspector-en...
> File
>
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html
> (right):
> 
>
https://codereview.chromium.org/401523004/diff/20001/LayoutTests/inspector-en...
>
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:46:
> log("Paused in breakpoint");
> You should dump the stack here to test that it is available.
> 
>
https://codereview.chromium.org/401523004/diff/20001/Source/devtools/front_en...
> File Source/devtools/front_end/sources/BreakpointsSidebarPane.js (right):
> 
>
https://codereview.chromium.org/401523004/diff/20001/Source/devtools/front_en...
> Source/devtools/front_end/sources/BreakpointsSidebarPane.js:609: if
> (eventName.indexOf(':') === -1) {
> This does not look particularly nice. We should either prepend everything with
> instrumentation: above and get rid of the isInstrumentation flag or place it
> into a new categoty.

I added a new changelist with a stacktrace in the test and a new window category
in the inspector listener breakpoint pane

[pfeldman, 2014-07-28 11:58:17]

lgtm

[aandrey, 2014-07-28 14:08:35]

https://codereview.chromium.org/401523004/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right):

https://codereview.chromium.org/401523004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMDebuggerAgent.cpp:487:
pauseOnNativeEventIfNeeded(preparePauseOnNativeEventData(windowCloseEventName,
nullptr), true);
nullptr -> 0

[aandrey, 2014-07-28 14:14:43]

https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
File
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint-expected.txt
(right):

https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint-expected.txt:8:
FAIL: main window was destroyed
FAIL ?!

https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
File
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html
(right):

https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:25:
function log(message) {
use InspectorTest.addResult() instead

https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:44:
var stacktrace = InspectorTest.captureStackTraceIntoString(callFrames,
asyncStackTrace);
remove asyncStackTrace argument

https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:47:
log(stacktrace.slice(0, stacktrace.indexOf('2)')).trim());
you can drop everything except the top call frame

[venuatu, 2014-07-28 14:28:55]

On 2014/07/28 14:14:43, aandrey wrote:
>
https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
>
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint-expected.txt:8:
> FAIL: main window was destroyed
> FAIL ?!

This is the expected behaviour (normal execution should continue after when
resuming) and the message is hard coded in
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/brow...

>
https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
>
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:25:
> function log(message) {

I needed this because InspectorTest.addResult() uses output() which places
elements in elements the body which gets disposed with the window

>
https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
>
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:44:
> var stacktrace = InspectorTest.captureStackTraceIntoString(callFrames,
> asyncStackTrace);
> remove asyncStackTrace argument
> 
>
https://codereview.chromium.org/401523004/diff/40001/LayoutTests/inspector-en...
>
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:47:
> log(stacktrace.slice(0, stacktrace.indexOf('2)')).trim());
> you can drop everything except the top call frame

Will do.

[venuatu, 2014-07-28 14:58:42]

I have uploaded changelists for those issues

[aandrey, 2014-07-28 15:19:40]

https://codereview.chromium.org/401523004/diff/80001/LayoutTests/inspector-en...
File
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html
(right):

https://codereview.chromium.org/401523004/diff/80001/LayoutTests/inspector-en...
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:52:
// After this point execution will resume and the window will close
I think you should not use InspectorTest.runDebuggerTestSuite() since there will
be no test suite at all. And at this point you should just call completeTest(),
and this will allow usage of InspectorTest.addResult()

[venuatu, 2014-07-29 00:49:59]

On 2014/07/28 15:19:40, aandrey wrote:
>
https://codereview.chromium.org/401523004/diff/80001/LayoutTests/inspector-en...
> File
>
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html
> (right):
> 
>
https://codereview.chromium.org/401523004/diff/80001/LayoutTests/inspector-en...
>
LayoutTests/inspector-enabled/sources/debugger/script-window-close-breakpoint.html:52:
> // After this point execution will resume and the window will close
> I think you should not use InspectorTest.runDebuggerTestSuite() since there
will
> be no test suite at all. And at this point you should just call
completeTest(),
> and this will allow usage of InspectorTest.addResult()

I just tried to do things that way and the only output is:
------
FAIL: main window was destroyed
------

And I uploaded a changelist for using startDebuggerTest instead of the suite

[aandrey, 2014-07-29 09:32:35]

lgtm

[commit-bot: I haz the power, 2014-07-29 09:33:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/venuatu@gmail.com/401523004/100001

[commit-bot: I haz the power, 2014-07-29 10:43:46]

Change committed as 179121