Bug 40774 – Web Inspector: implement layout tests for debugger

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

[Yury Semikhatsky]

Implement layout tests for JS debugger in Web Inspector.

[Yury Semikhatsky]

Created attachment 59258 [details]
Patch

[Pavel Feldman]

Comment on attachment 59258 [details]
Patch

LayoutTests/http/tests/inspector/inspector-test.js:35
 +          if (window.layoutTestController) {
You don't need this change, right? Same here. We should rely on DRT. Is there a reason we can't trust it?

LayoutTests/http/tests/inspector/inspector-test.js:107
 +      var toolbar = document.getElementById("toolbar");
What is wrong with assigning to the current panel? (that is what IC is doing by means of WebInspector.show*Panel calls.

WebCore/platform/mac/EventLoopMac.mm:35
 +      NSTimeInterval interval = [[NSDate date] timeIntervalSinceReferenceDate];
I am hesitant getting into this code / reviewing it in connection with inspector tests. Why did you change this? Does it work on all other platforms? (You are only changing the mac's platform). What non-inspector flows will it affect? r- until this is cleared properly.

[Yury Semikhatsky]

(In reply to comment #2)
> (From update of attachment 59258 [details])
> LayoutTests/http/tests/inspector/inspector-test.js:35
>  +          if (window.layoutTestController) {
> You don't need this change, right? Same here. We should rely on DRT. Is there a reason we can't trust it?
> 
This is needed to ensure that debugger is resumed after test is completed.

> LayoutTests/http/tests/inspector/inspector-test.js:107
>  +      var toolbar = document.getElementById("toolbar");
> What is wrong with assigning to the current panel? (that is what IC is doing by means of WebInspector.show*Panel calls.
> 
Will change this.

> WebCore/platform/mac/EventLoopMac.mm:35
>  +      NSTimeInterval interval = [[NSDate date] timeIntervalSinceReferenceDate];
> I am hesitant getting into this code / reviewing it in connection with inspector tests. Why did you change this? Does it work on all other platforms? (You are only changing the mac's platform). What non-inspector flows will it affect? r- until this is cleared properly.
As I wrote in ChangeLog there may be no events to be processed in the EventLoop if script execution is paused in DRT. If there were no timeout the test would hang since ScriptDebugServer would never leave the event loop and wouldn't have a chance to check if the execution was resumed.

[Yury Semikhatsky]

Created attachment 59372 [details]
Patch

[Yury Semikhatsky]

> WebCore/platform/mac/EventLoopMac.mm:35
>  +      NSTimeInterval interval = [[NSDate date] timeIntervalSinceReferenceDate];
> I am hesitant getting into this code / reviewing it in connection with inspector tests. Why did you change this? Does it work on all other platforms? (You are only changing the mac's platform). What non-inspector flows will it affect? r- until this is cleared properly.
Would be nice if the author of the code look at this change. Tim, could you review this stuff?

[Pavel Feldman]

Comment on attachment 59372 [details]
Patch

WebCore/platform/mac/EventLoopMac.mm:36
 +      interval += 0.05;
From the IRC: Timothy is suggesting longer timeout (1s). Other than that, he is fine.

[Pavel Feldman]

> From the IRC: Timothy is suggesting longer timeout (1s). Other than that, he is fine.
... with the change :)

[Yury Semikhatsky]

(In reply to comment #6)
> (From update of attachment 59372 [details])
> WebCore/platform/mac/EventLoopMac.mm:36
>  +      interval += 0.05;
> From the IRC: Timothy is suggesting longer timeout (1s). Other than that, he is fine.

1s timeout would slow down each pause/continue pair for 1s. So I'd rather continue with a shorter timeout.

[Yury Semikhatsky]

Comment on attachment 59372 [details]
Patch

Clearing flags on attachment: 59372

Committed r61749: <http://trac.webkit.org/changeset/61749>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Appears to cause Qt failure:
http://build.webkit.org/results/Qt%20Linux%20Release/r61749%20(13882)/inspector/debugger-pause-on-debugger-statement-pretty-diff.html

[WebKit Review Bot]

http://trac.webkit.org/changeset/61749 might have broken Qt Linux Release