Summary: | Refactor DB layout tests so that they work in Web Workers as well as Pages. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric U. <ericu> | ||||||||
Component: | DOM | Assignee: | Eric U. <ericu> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Enhancement | CC: | commit-queue, dimich, dumi, ericu | ||||||||
Priority: | P4 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 34990 | ||||||||||
Attachments: |
|
Description
Eric U.
2010-02-16 13:57:25 PST
Created attachment 56933 [details]
Patch
LGTM. Are the other DB layout tests failing in workers, or are you just trying to keep the patches small? (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. 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 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 on attachment 57060 [details]
Another set of tests. This is much smaller than the diff size suggests.
Same prefix issue with openDatabase, otherwise LGTM.
(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. 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 on attachment 57291 [details]
Merged the two patches and added a function
Forgot to click "patch".
Comment on attachment 57291 [details]
Merged the two patches and added a function
r=me
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> All reviewed patches have been landed. Closing bug. |