Bug 34995 – Refactor DB layout tests so that they work in Web Workers as well as Pages.

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

All the current layout tests should be able to run in the Worker implementation of the async DB as well, with a small refactoring.

Umbrella bug 34990 depends on this.

[Eric U.]

Created attachment 56933 [details]
Patch

[Dumitru Daniliuc]

LGTM. Are the other DB layout tests failing in workers, or are you just trying to keep the patches small?

[Eric U.]

(In reply to comment #2)
> LGTM. Are the other DB layout tests failing in workers, or are you just trying to keep the patches small?

The latter.  I figured a bunch of small patches would be easier to review, so I'm just porting a few at a time.

Some of them will be less trivial to port [those involving reloading, in particular], but there are still a bunch more trivial ones to go.

[Eric U.]

Created attachment 57060 [details]
Another set of tests.  This is much smaller than the diff size suggests.

This is almost entirely trivial changes [moving code from .html to .js files].  It's *much* smaller than the patch size suggests.

It adds changes to a file added in the first patch, so I'll have to do a manual merge in between the two of them being committed.  I'm setting CQ? on #1 and CQ- on #2 to enable that.

[Dmitry Titov]

Comment on attachment 56933 [details]
Patch

Sorry it takes too long.

LayoutTests/storage/multiple-databases-garbage-collection.js:27
 +      persistentDB = openDatabase("MultipleDatabasesTest1", "1.0", "Test one out of a set of databases being destroyed (1)", 32768);
+ DB_TEST_SUFFIX here?

LayoutTests/storage/multiple-databases-garbage-collection.js:28
 +      forgottenDB = openDatabase("MultipleDatabasesTest2", "1.0", "Test one out of a set of databases being destroyed (2)", 32768);
same

LayoutTests/storage/multiple-transactions-on-different-handles.js:13
 +      return openDatabase("MultipleTransactionsOnDifferentHandlesTest",
+ DB_TEST_SUFFIX
Maybe there should be a single function somewhere in database-common.js to make sure the suffix is not forgotten in each test, and perhaps simply a variable for each test containing the desired name of the database? Perhaps openTestDatabase can be repurposed to be this common function...

r- to add the suffix and please consider a common function that does this.

[Dmitry Titov]

Comment on attachment 57060 [details]
Another set of tests.  This is much smaller than the diff size suggests.

Same prefix issue with openDatabase, otherwise LGTM.

[Eric U.]

(In reply to comment #5)
> (From update of attachment 56933 [details])
> Sorry it takes too long.
> 
> LayoutTests/storage/multiple-databases-garbage-collection.js:27
>  +      persistentDB = openDatabase("MultipleDatabasesTest1", "1.0", "Test one out of a set of databases being destroyed (1)", 32768);
> + DB_TEST_SUFFIX here?
> 
> LayoutTests/storage/multiple-databases-garbage-collection.js:28
>  +      forgottenDB = openDatabase("MultipleDatabasesTest2", "1.0", "Test one out of a set of databases being destroyed (2)", 32768);
> same
> 
> LayoutTests/storage/multiple-transactions-on-different-handles.js:13
>  +      return openDatabase("MultipleTransactionsOnDifferentHandlesTest",
> + DB_TEST_SUFFIX
> Maybe there should be a single function somewhere in database-common.js to make sure the suffix is not forgotten in each test, and perhaps simply a variable for each test containing the desired name of the database? Perhaps openTestDatabase can be repurposed to be this common function...
> 
> r- to add the suffix and please consider a common function that does this.

I made a common function, put it into database-common.js and database-worker.js, and removed the explicit mentions of DB_TEST_SUFFIX in each of the test files.  Other than overriding openDatabase directly, which I think would be confusing, I think that's about all we can do.

I've merged the two changelists together, since I'm not sure what the commit queue does if you give it two partial changelists; the code is [other than that addition and the calls to it] unchanged.  However, I've removed one test: fast/workers/storage/hash-change-with-xhr.html is causing asserts, so I want to chase that down.  But I don't see any reason to delay the rest of the CL based on that.

[Eric U.]

Created attachment 57291 [details]
Merged the two patches and added a function

Please don't take my word that this is the same code--give it a once-over to see if I've missed anything.  There was a bit of slicing and dicing to get all the changes integrated and in the same client.  I *think* it's all right, but it's possible I missed a file somewhere.

[Eric U.]

Comment on attachment 57291 [details]
Merged the two patches and added a function

Forgot to click "patch".

[Dmitry Titov]

Comment on attachment 57291 [details]
Merged the two patches and added a function

r=me

[WebKit Commit Bot]

Comment on attachment 57291 [details]
Merged the two patches and added a function

Clearing flags on attachment: 57291

Committed r60387: <http://trac.webkit.org/changeset/60387>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.