Bug 76683 – IndexedDB: refactor basics layout test so that it can be run on workers.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

IndexedDB: refactor basics layout test so that it can be run on workers.

[David Grogan]

Created attachment 123247 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123247 [details]
Patch

Attachment 123247 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11298324

New failing tests:
storage/indexeddb/basics-workers.html

[David Grogan]

Created attachment 123374 [details]
Patch

[David Grogan]

Created attachment 123380 [details]
Patch

[David Grogan]

Created attachment 123383 [details]
Patch

[David Grogan]

Josh, could you take a look at this?

https://chromiumcodereview.appspot.com/9269021 runs this as a ui test.

I'm going to convert more (all of the?) layout tests to run on workers after taking a look at integrating idb-worker-common.js and fs-worker-test-util.js into js-test-pre.js.

[Joshua Bell]

Comment on attachment 123383 [details]
Patch

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

overall lgtm, with a few questions

> LayoutTests/storage/indexeddb/basics-workers.html:11
> +<script src="../../fast/js/resources/js-test-post.js"></script>

We were removing js-test-post.js from other IDB layout tests - is it still necessary here?

> LayoutTests/storage/indexeddb/basics.html:10
> +<script src="../../fast/js/resources/js-test-post.js"></script>

We were removing js-test-post.js from other IDB layout tests - is it still necessary here?

> LayoutTests/storage/indexeddb/resources/basics.js:2
> +    window = self;

I assume this is necessary because some of the shared code (from fast/js?) refers to window? If so, perhaps add a FIXME (here) to change those references to "self" if they're truly agnostic between running in a window and in a worker.

If it's only code in this test (e.g. "'webkitIndexedDB' in window") then I'd just change them to self (which should be safe either way...)

> LayoutTests/storage/indexeddb/resources/basics.js:12
> +    shouldBeTrue("'webkitIndexedDB' in window");

s/window/self/ per above?

> LayoutTests/storage/indexeddb/resources/basics.js:15
> +    shouldBeTrue("'webkitIDBCursor' in window");

s/window/self/ per above?

[David Grogan]

Comment on attachment 123383 [details]
Patch

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

>> LayoutTests/storage/indexeddb/basics-workers.html:11
>> +<script src="../../fast/js/resources/js-test-post.js"></script>
> 
> We were removing js-test-post.js from other IDB layout tests - is it still necessary here?

Yeah, it calls layoutTestController.waitUntilDone() when jsTestIsAsync == true.

>> LayoutTests/storage/indexeddb/resources/basics.js:2
>> +    window = self;
> 
> I assume this is necessary because some of the shared code (from fast/js?) refers to window? If so, perhaps add a FIXME (here) to change those references to "self" if they're truly agnostic between running in a window and in a worker.
> 
> If it's only code in this test (e.g. "'webkitIndexedDB' in window") then I'd just change them to self (which should be safe either way...)

Good idea.  FIXME added.

>> LayoutTests/storage/indexeddb/resources/basics.js:12
>> +    shouldBeTrue("'webkitIndexedDB' in window");
> 
> s/window/self/ per above?

Done.

>> LayoutTests/storage/indexeddb/resources/basics.js:15
>> +    shouldBeTrue("'webkitIDBCursor' in window");
> 
> s/window/self/ per above?

Done.

[David Grogan]

Created attachment 124414 [details]
Patch

[David Grogan]

Created attachment 124422 [details]
Patch

[David Grogan]

Tony, could you review this?

[Tony Chang]

Comment on attachment 124422 [details]
Patch

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

> LayoutTests/storage/indexeddb/resources/basics.js:2
> +if (this.importScripts) {
> +    // FIXME: Change js-test-pre.js to use self in place of window where

Should this file go in resources or script-tests?  If this file is only used by basics.html, then it probably should go in script-tests.

> LayoutTests/storage/indexeddb/resources/idb-worker-common.js:1
> +function debug(message)

Is this file used or is it just used when run in Chrome?

[David Grogan]

Comment on attachment 124422 [details]
Patch

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

>> LayoutTests/storage/indexeddb/resources/basics.js:2
>> +    // FIXME: Change js-test-pre.js to use self in place of window where
> 
> Should this file go in resources or script-tests?  If this file is only used by basics.html, then it probably should go in script-tests.

Also in basics-workers.html.  Should I still put it in script-tests?  I thought script-tests/make-script-test-wrappers was obsolete for tests outside of fast/js.

>> LayoutTests/storage/indexeddb/resources/idb-worker-common.js:1
>> +function debug(message)
> 
> Is this file used or is it just used when run in Chrome?

It's used in DRT as well.  DRT started running dedicated workers sometime in December.

[Tony Chang]

Comment on attachment 124422 [details]
Patch

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

LGTM

>>> LayoutTests/storage/indexeddb/resources/basics.js:2
>>> +    // FIXME: Change js-test-pre.js to use self in place of window where
>> 
>> Should this file go in resources or script-tests?  If this file is only used by basics.html, then it probably should go in script-tests.
> 
> Also in basics-workers.html.  Should I still put it in script-tests?  I thought script-tests/make-script-test-wrappers was obsolete for tests outside of fast/js.

Ah, you can keep it here since it's used by 2 files.

[WebKit Review Bot]

Comment on attachment 124422 [details]
Patch

Clearing flags on attachment: 124422

Committed r106392: <http://trac.webkit.org/changeset/106392>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.