[webkit-reviews] review granted: [Bug 28714] Need to write tests for worker lifecycle mechanisms : [Attachment 38557] patch containing new layout tests

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Andrew Wilson
<atwilson at chromium.org>'s request for review:
Bug 28714: Need to write tests for worker lifecycle mechanisms
https://bugs.webkit.org/show_bug.cgi?id=28714

Attachment 38557: patch containing new layout tests
https://bugs.webkit.org/attachment.cgi?id=38557&action=review

------- Additional Comments from David Levin <levin at chromium.org>


> diff --git
a/LayoutTests/fast/workers/resources/create-shared-worker-frame.html
b/LayoutTests/fast/workers/resources/create-shared-worker-frame.html
> \ No newline at end of file

Please fix.

> diff --git a/LayoutTests/fast/workers/resources/worker-util.js
b/LayoutTests/fast/workers/resources/worker-util.js

> +function gc()
> +{
> +    for (var i = 0; i < 10000; i++) { // force garbage collection (FF
requires about 9K allocations before a collect)
> +	   var s = new String("abc");
> +    }

I think there is a layoutTestController.gc() function that you may want to use
if it is there.

 
> +function waitUntilThreadCountMatches(callback, count)
...
> +	   setTimeout(function() { waitUntilThreadCountMatches(callback,
count); }, 100);

A shorter poll interval would be good to ensure that the test runs quickly.


> +function ensureThreadCountMatches(callback, count)
> +{
> +    // Just wait until the thread count matches, then wait another 100ms to
see if it changes, then fire the callback.

100ms is a bit long for a unit test.  Is there any way to make this smaller?

> +    waitUntilThreadCountMatches(function() {
> +	       setTimeout(function() { waitUntilThreadCountMatches(callback,
count); },  100);

You have an extra space before the 100.


> +    if (window.layoutTestController &&
window.layoutTestController.notifyDone)

Seems odd to check "window.layoutTestController.notifyDone"?