[webkit-reviews] review denied: [Bug 34992] Add async bindings for Worker access to DB : [Attachment 53290] The previous patch was missing the WebCore changes.

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 34992: Add async bindings for Worker access to DB
https://bugs.webkit.org/show_bug.cgi?id=34992

Attachment 53290: The previous patch was missing the WebCore changes.
https://bugs.webkit.org/attachment.cgi?id=53290&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>

Looks great. I have only one question left:

(In reply to comment #8)
> > > Index: LayoutTests/storage/change-version-handle-reuse.html
> > >  {
> > >	   if (window.layoutTestController) {
> > > +        layoutTestController.clearAllDatabases();
> > 
> > It seems existing db tests are gaining the new call to clearAllDatabases()
> > before start. It may change slightly what they test, at first look it makes

> > them run in a more 'sterile' environment. I wonder if it is necessary. 
> 
> I added that to clean things up a bit.  Looking at the tests, there was
little
> protection against flakiness.  Many tests could leave a random amount of data

> lying around, enough to make an openDatabase call in another test go over
> quota, depending on implementation and luck.	I think we should add that call

> to all database tests unless we have a really good reason not to start clean.

> This goes double for the multi-process test harness.

I hear you, but at the same time two things worry me:
- we might end up with the tests only verifying creating a new database, never
opening an existing one. It is probably not ideal. Perhaps it should be only
added to tests that can generate big databases over time?
- in case of new-run-webkit-tests the clearAllDatabases() can come in during
another test's running. I'm not sure I know all the details of the new script,
but I don't think we have separate local store dirs for multiple DRTs
running...

r- to clarify if the clearAllDatabases does not interfere with
new-run-webkit-tests, please feel free to flip back if it does not.