Bug 34995

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.