Bug 34995 - Refactor DB layout tests so that they work in Web Workers as well as Pages.
Summary: Refactor DB layout tests so that they work in Web Workers as well as Pages.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Enhancement
Assignee: Eric U.
URL:
Keywords:
Depends on:
Blocks: 34990
  Show dependency treegraph
 
Reported: 2010-02-16 13:57 PST by Eric U.
Modified: 2010-05-28 20:46 PDT (History)
4 users (show)

See Also:


Attachments
Patch (26.51 KB, patch)
2010-05-24 14:54 PDT, Eric U.
dimich: review-
Details | Formatted Diff | Diff
Another set of tests. This is much smaller than the diff size suggests. (48.47 KB, patch)
2010-05-25 18:29 PDT, Eric U.
dimich: review-
ericu: commit-queue-
Details | Formatted Diff | Diff
Merged the two patches and added a function (72.79 KB, patch)
2010-05-27 19:38 PDT, Eric U.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric U. 2010-02-16 13:57:25 PST
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.
Comment 1 Eric U. 2010-05-24 14:54:19 PDT
Created attachment 56933 [details]
Patch
Comment 2 Dumitru Daniliuc 2010-05-25 12:48:47 PDT
LGTM. Are the other DB layout tests failing in workers, or are you just trying to keep the patches small?
Comment 3 Eric U. 2010-05-25 13:37:18 PDT
(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.
Comment 4 Eric U. 2010-05-25 18:29:29 PDT
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.
Comment 5 Dmitry Titov 2010-05-27 14:05:43 PDT
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.
Comment 6 Dmitry Titov 2010-05-27 14:09:01 PDT
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.
Comment 7 Eric U. 2010-05-27 19:35:09 PDT
(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.
Comment 8 Eric U. 2010-05-27 19:38:27 PDT
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.
Comment 9 Eric U. 2010-05-27 19:39:10 PDT
Comment on attachment 57291 [details]
Merged the two patches and added a function

Forgot to click "patch".
Comment 10 Dmitry Titov 2010-05-28 12:47:14 PDT
Comment on attachment 57291 [details]
Merged the two patches and added a function

r=me
Comment 11 WebKit Commit Bot 2010-05-28 20:46:01 PDT
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>
Comment 12 WebKit Commit Bot 2010-05-28 20:46:07 PDT
All reviewed patches have been landed.  Closing bug.