Bug 34992 – Add async bindings for Worker access to DB

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

Umbrella bug 34990 depends on this.  This bug is just for adding the bindings and the last bits of code to support them.  This should include at least one proof-of-concept test; I'll log a separate bug for the rest of the tests.

[Eric U.]

Created attachment 51583 [details]
Patch

[Dumitru Daniliuc]

Comment on attachment 51583 [details]
Patch

> Index: LayoutTests/storage/change-version-handle-reuse.js
> ===================================================================
> +        db = openDatabase("ChangeVersion", "", "Test that changing a database version doesn\'t kill our handle to it", 1);

minor: probably don't need the \ in "doesn\'t".

> +        setTimeout(runTest2, 1000);

why do you want to wait 1s here? if you want to make sure the other transaction is done before calling runTest2(), you can call runTest2() in the transaction's success callback.

> Index: LayoutTests/storage/execute-sql-args.js
> ===================================================================
> +function runTest()
> +{
> +
> +    var db = openDatabase("ExecuteSQLArgsTest", "1.0", "Test of handling of the arguments to SQLTransaction.executeSql", 1);
> +    db.transaction(runTransactionTests);
> +}

minor: unnecessary empty line.

> Index: LayoutTests/storage/resources/database-worker-controller.js
> ===================================================================
> Index: LayoutTests/storage/resources/database-worker.js
> ===================================================================

4-space indentation in these files.
also, not sure if the "no {} around 1-line if-statements" rule applies to for-loops too, might be worth asking somebody.

[David Levin]

(In reply to comment #2)
> > Index: LayoutTests/storage/resources/database-worker.js

> also, not sure if the "no {} around 1-line if-statements" rule applies to
> for-loops too, might be worth asking somebody.

It applies to for loops but it isn't enforced in js files.

[Eric U.]

(In reply to comment #2)
> (From update of attachment 51583 [details])
> > Index: LayoutTests/storage/change-version-handle-reuse.js
> > ===================================================================
> > +        db = openDatabase("ChangeVersion", "", "Test that changing a database version doesn\'t kill our handle to it", 1);
> 
> minor: probably don't need the \ in "doesn\'t".

Fixed.

> > +        setTimeout(runTest2, 1000);
> 
> why do you want to wait 1s here? if you want to make sure the other transaction
> is done before calling runTest2(), you can call runTest2() in the transaction's
> success callback.

Done.  I don't know why that was there; it predates this refactoring.  I see that the HTML also refers to a reload, which isn't in this test, so I suspect that's kruft left over from copy+paste.  I'm also removing that mention of the reload.

> > Index: LayoutTests/storage/execute-sql-args.js
> > ===================================================================
> > +function runTest()
> > +{
> > +
> > +    var db = openDatabase("ExecuteSQLArgsTest", "1.0", "Test of handling of the arguments to SQLTransaction.executeSql", 1);
> > +    db.transaction(runTransactionTests);
> > +}
> 
> minor: unnecessary empty line.

Removed.

> > Index: LayoutTests/storage/resources/database-worker-controller.js
> > ===================================================================
> > Index: LayoutTests/storage/resources/database-worker.js
> > ===================================================================
> 
> 4-space indentation in these files.

Fixed.

> also, not sure if the "no {} around 1-line if-statements" rule applies to
> for-loops too, might be worth asking somebody.

Fixed.

[Eric U.]

Created attachment 51662 [details]
Patch

[Eric U.]

Ping: Any JSC folks have time to take a look at this?

[Dmitry Titov]

Comment on attachment 51662 [details]
Patch

I'm not exactly JCS expert but since it collects dust I'm going to try...

> Index: WebCore/ChangeLog
> +        Add async bindings for Worker access to DB

I'd say "Add bindings for async DB API in Workers". The bindings themselves are not async.

> Index: WebCore/bindings/js/JSWorkerContextCustom.cpp
> +JSValue JSWorkerContext::openDatabase(ExecState* exec, const ArgList& args) 
> +{ 
> +    ExceptionCode ec = 0; 

I understand this is basically a copy from JSDOMWindowCustom, but I'd move ExceptionCode declaration close to the line that actually uses it.

> Index: LayoutTests/ChangeLog
> +        * storage/execute-sql-args.js: This is the extracted shared core of the test.
> +        (throwOnToStringObject.toString):
> +        (var):
> +        ():

It might be better to just remove the lines describing actual changes inside test files, unless you want to add some additional info.

> 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. 

> Index: LayoutTests/storage/change-version-handle-reuse.js
> +function runTest()
> +{
> +    try {
> +        db = openDatabase("ChangeVersion", "", "Test that changing a database version doesn't kill our handle to it", 1);

What is going to happen when multi-process test harness runs those tests? One process could run regular test, while another will run worker test - both of which will access the same database. Should we use different names for databases in workers?

A general note on the tests to consider: As I understand the new worker tests can be located either in LayoutTests/Storage or in LayoutTests/fast/workers/storage (and refer back for .js files). It might be slightly better to move the worker test files to the fast/workers/storage simply because workers tests are skipped already on many platforms that do not enable workers, and at least one port (chromium) has completely different handling for workers tests for now so it'd be more convenient to add new tests in the directories that already are treated in a special way.

[Eric U.]

Thanks for the review.

I'll respond to [or just fix] all the other comments, but first I wanted to make a comment and ask for a clarification.

> > 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.
  
> A general note on the tests to consider: As I understand the new worker tests
> can be located either in LayoutTests/Storage or in
> LayoutTests/fast/workers/storage (and refer back for .js files). It might be
> slightly better to move the worker test files to the fast/workers/storage
> simply because workers tests are skipped already on many platforms that do not
> enable workers, and at least one port (chromium) has completely different
> handling for workers tests for now so it'd be more convenient to add new tests
> in the directories that already are treated in a special way.

I put the new tests in storage to let them share code easily with the database tests.  I could of course copy the tests over to fast/workers/storage, but then we end up with two copies, which may drift apart.  LayoutTests/fast/workers/storage could load resources out of "../../../storage"; would that be acceptable?

[Eric U.]

Created attachment 53213 [details]
Fixed changelogs, moved worker tests, made database names unique.

I believe I've addressed all your concerns; I left in the database cleanup at the beginning of each test, as I believe we should have that in every database test.  I'll get to the others shortly.

[Eric U.]

Created attachment 53290 [details]
The previous patch was missing the WebCore changes.

The previous patch had only the LayoutTests changes, without the WebCore changes to make use of them.

[Dmitry Titov]

Comment on attachment 53290 [details]
The previous patch was missing the WebCore changes.


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.

[Eric U.]

(In reply to comment #11)
> (From update of attachment 53290 [details])
> 
> 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?

We can always add tests that are specifically designed to re-open databases.  At the moment each test uses a unique database name, so clearing at the beginning of each test doesn't change that.

> - 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...

Hmm...you're right about that.  There's no guarantee that it's not going to delete a DB another test is using; it'd probably be a rare race that would lead to hard-to-find flakiness.  I put that in back before new-run-webkit-tests existed.  I'll take the call out for now, but we really need a solution to this, given that the tests as currently written will occasionally fail for the opposite reason: using too much quota between them where one at a time was just fine.  A call to clear out each test's databases at the end of the test, along with making sure nobody goes over quota, would let us handle everything except testing over-quota behavior.  If a test fails or crashes before cleanup, that will leave a mess, but at least we'll know what caused the rest of the failures.

I'll leave that for another CL, though.

[Eric U.]

Created attachment 53304 [details]
Removed calls to clearAllDatabases

[Dmitry Titov]

Comment on attachment 53304 [details]
Removed calls to clearAllDatabases

r=me

[WebKit Commit Bot]

Comment on attachment 53304 [details]
Removed calls to clearAllDatabases

Clearing flags on attachment: 53304

Committed r57688: <http://trac.webkit.org/changeset/57688>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dmitry Titov]

Reverted r57688 for reason:

Makes fast/workers/dedicated-worker-lifecycle.html crashing on all GTK bots

Committed r57704: <http://trac.webkit.org/changeset/57704>

[Eric U.]

(In reply to comment #17)
> Reverted r57688 for reason:
> 
> Makes fast/workers/dedicated-worker-lifecycle.html crashing on all GTK bots
> 
> Committed r57704: <http://trac.webkit.org/changeset/57704>

I didn't touch that test or anything that should have affected it directly.  My assumption is that I've done something that reveals another bug, and that that test ran after one of my new tests, which got things into an unusual state.  That underlying bug could easily have come out of the refactorings I did to make this CL possible, though.

I've just build+run both debug and release builds of webkit-gtk, and I can't reproduce the problem at all, so either:

1) The underlying problem's been fixed in the past 2 weeks.
2) The underlying problem's gotten harder to repro in the past 2 weeks.
3) I just can't reproduce the conditions on the bots.

Suggestions?  Is there a way to run this patch past the bots again without checking it in?

[Eric U.]

Created attachment 54736 [details]
Updated patch to take into account the big string refactoring.  No functional change, only a one-liner in JSWorkerContextCustom.cpp.

[Eric U.]

Comment on attachment 54736 [details]
Updated patch to take into account the big string refactoring.  No functional change, only a one-liner in JSWorkerContextCustom.cpp.

Dmitry and I discussed this, and we decided to re-land it and see if anything breaks this time.  I can't repro what happened on the gtk bots on Ubuntu 9.1 in either debug or release, but if this patch is revealing an existing bug, it's probably going to affect all platforms eventually.

[Adam Barth]

Comment on attachment 54736 [details]
Updated patch to take into account the big string refactoring.  No functional change, only a one-liner in JSWorkerContextCustom.cpp.

WebCore/bindings/js/JSWorkerContextCustom.cpp:151
 +      const UString& name = args.at(0).toString(exec); 
What about exceptions?

WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:144
 +      // Implementation coming soon.
Please throw a NOT_IMPLEMENTED exception.

[Eric U.]

Created attachment 54745 [details]
Added exception for V8 bindings.

[Eric U.]

Comment on attachment 54745 [details]
Added exception for V8 bindings.

As per discussion with Adam offline, since these bindings were copied from the DOM bindings, they're ALL wrong in the same way, and there's a bug open already, we're going to leave them as-is for the big cleanup.

[Dmitry Titov]

Comment on attachment 54745 [details]
Added exception for V8 bindings.

r=me
Lets give it another try.

[WebKit Commit Bot]

Comment on attachment 54745 [details]
Added exception for V8 bindings.

Clearing flags on attachment: 54745

Committed r58569: <http://trac.webkit.org/changeset/58569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58569 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/58569
http://trac.webkit.org/changeset/58571

[Eric U.]

We rolled this back again, but managed to get a good stack trace from the breakage.  That led to https://bugs.webkit.org/show_bug.cgi?id=38623, which is now fixed, so let's try this again--it should now work without breaking anybody.

[Eric U.]

Created attachment 55639 [details]
Patch

[Dumitru Daniliuc]

Comment on attachment 55639 [details]
Patch

unofficial review.

> Index: WebCore/ChangeLog
> ===================================================================
>          * WebCore.vcproj/WebCore.vcproj:
>  
> ->>>>>>> .r59084
>  2010-05-10  Dirk Schulze  <[e-mail]>
>  
>          Reviewed by Nikolas Zimmermann.
> @@ -2804,6 +2834,7 @@
>          (WebCore::FileStream::read):
>          * html/FileStream.h:
>  
> +>>>>>>> .r59110
>  2010-05-05  Steve Falkenburg  <[e-mail]>
>  
>          Reviewed by Maciej Stachowiak.
> @@ -5008,6 +5039,7 @@
>          (WebCore::jsTestObjPrototypeFunctionMethodWithNonOptionalArgAndOptionalArg):
>          (WebCore::jsTestObjPrototypeFunctionMethodWithNonOptionalArgAndTwoOptionalArgs):
>  
> +>>>>>>> .r58796
>  2010-04-29  Gustavo Noronha Silva  <gustavo.noronhaollabora.co.uk>

looks like you need to resolve WebCore/ChangeLog.

> Index: WebCore/bindings/js/JSWorkerContextCustom.cpp
> ===================================================================
> +#include "Database.h"
> +#include "JSDatabase.h"
> +#include "JSDatabaseCallback.h"

i think these includes should be inside #if ENABLE(DATABASE) too (the includes for the sync DB classes are).

>  #if ENABLE(DATABASE)
> +JSValue JSWorkerContext::openDatabase(ExecState* exec, const ArgList& args) 
> +{ 
> +    const UString& name = args.at(0).toString(exec); 
> +    const UString& version = args.at(1).toString(exec); 
> +    const UString& displayName = args.at(2).toString(exec); 
> +    unsigned long estimatedSize = args.at(3).toInt32(exec); 
> +    RefPtr<DatabaseCallback> creationCallback; 
> +    if ((args.size() >= 5) && args.at(4).isObject()) 
> +        creationCallback = JSDatabaseCallback::create(asObject(args.at(4)), globalObject()); 
> + 
> +    ExceptionCode ec = 0; 
> +    JSValue result = toJS(exec, globalObject(), WTF::getPtr(impl()->openDatabase(ustringToString(name), ustringToString(version), ustringToString(displayName), estimatedSize, creationCallback.release(), ec))); 
> +    setDOMException(exec, ec); 
> +    return result; 
> +} 
> +#endif

can you please update this file and follow the same pattern that openDatabaseSync() does? it's pretty much the same code, only with more error checking (abarth insisted on it).

> Index: WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp
> ===================================================================
> --- WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp	(revision 59110)
> +++ WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp	(working copy)
> @@ -42,6 +42,7 @@
>  #include "ScheduledAction.h"
>  #include "V8Binding.h"
>  #include "V8BindingMacros.h"
> +#include "V8Database.h"

should probably be inside #if ENABLE(DATABASE) too.

> Index: WebCore/workers/WorkerContext.cpp
> ===================================================================
> @@ -275,13 +276,21 @@ PassRefPtr<Database> WorkerContext::open
>          return 0;
>      }
>  
> -    ASSERT(Database::isAvailable());
>      if (!Database::isAvailable())
>          return 0;

we should set 'ec = SECURITY_ERR' too. you can probably combine this 'if' with the previous one.

> Index: WebCore/workers/WorkerContext.idl
> ===================================================================
>  #if defined(ENABLE_DATABASE) && ENABLE_DATABASE
> -        // Database openDatabase(in DOMString name, in DOMString version, in DOMString displayName, in unsigned long estimatedSize);
> +        [EnabledAtRuntime, Custom] Database openDatabase(in DOMString name, in DOMString version, in DOMString displayName, in unsigned long estimatedSize, in DatabaseCallback creationCallback)
> +            raises(DOMException);
>          [EnableAtRuntime, Custom] DatabaseSync openDatabaseSync(in DOMString name, in DOMString version, in DOMString displayName, in unsigned long estimatedSize, in DatabaseCallback creationCallback);

oops, i have a bug here: s/Enable/Enabled/. can you please fix it since you're editing this code?

also, i'm not sure we need [EnabledAtRuntime] here at all, since in the implemention of openDatabase{Sync}() we manually check if Database{Sync}.isAvailable(), but we can figure that out in a separate patch.

> Index: LayoutTests/fast/workers/storage/resources/database-worker-controller.js
> ===================================================================
> --- LayoutTests/fast/workers/storage/resources/database-worker-controller.js	(revision 0)
> +++ LayoutTests/fast/workers/storage/resources/database-worker-controller.js	(revision 0)
> @@ -0,0 +1,24 @@
> +var databaseWorker = new Worker('resources/database-worker.js');
> +
> +databaseWorker.onerror = function(event) {
> +    log("Caught an error from the worker!");
> +    log(event);
> +    for (var i in event) {
> +        log("event[" + i + "]: " + event[i]);
> +    }

no need for {}.

> +databaseWorker.onmessage = function(event) {
> +    if (event.data.indexOf('log:') == 0)
> +        log(event.data.substring(4));
> +    else if (event.data == 'notifyDone') {
> +        if (window.layoutTestController)
> +                layoutTestController.notifyDone();

wrong indentation, should be 4 spaces.

this is definitely beyond the scope of this patch, but i think it would be nice if we could figure out a way to share the *-expected.txt files between the DOM and worker storage tests too.

[Eric U.]

(In reply to comment #30)
> (From update of attachment 55639 [details])
> unofficial review.
> 
> > Index: WebCore/ChangeLog
> > ===================================================================
> >          * WebCore.vcproj/WebCore.vcproj:
> >  
> > ->>>>>>> .r59084
> >  2010-05-10  Dirk Schulze  <[e-mail]>
> >  
> >          Reviewed by Nikolas Zimmermann.
> > @@ -2804,6 +2834,7 @@
> >          (WebCore::FileStream::read):
> >          * html/FileStream.h:
> >  
> > +>>>>>>> .r59110
> >  2010-05-05  Steve Falkenburg  <[e-mail]>
> >  
> >          Reviewed by Maciej Stachowiak.
> > @@ -5008,6 +5039,7 @@
> >          (WebCore::jsTestObjPrototypeFunctionMethodWithNonOptionalArgAndOptionalArg):
> >          (WebCore::jsTestObjPrototypeFunctionMethodWithNonOptionalArgAndTwoOptionalArgs):
> >  
> > +>>>>>>> .r58796
> >  2010-04-29  Gustavo Noronha Silva  <gustavo.noronhaollabora.co.uk>
> 
> looks like you need to resolve WebCore/ChangeLog.

Fixed.  Looks like I wasn't the only one, either ;'>.

> > Index: WebCore/bindings/js/JSWorkerContextCustom.cpp
> > ===================================================================
> > +#include "Database.h"
> > +#include "JSDatabase.h"
> > +#include "JSDatabaseCallback.h"
> 
> i think these includes should be inside #if ENABLE(DATABASE) too (the includes for the sync DB classes are).

Fixed.

> >  #if ENABLE(DATABASE)
> > +JSValue JSWorkerContext::openDatabase(ExecState* exec, const ArgList& args) 
> > +{ 
> > +    const UString& name = args.at(0).toString(exec); 
> > +    const UString& version = args.at(1).toString(exec); 
> > +    const UString& displayName = args.at(2).toString(exec); 
> > +    unsigned long estimatedSize = args.at(3).toInt32(exec); 
> > +    RefPtr<DatabaseCallback> creationCallback; 
> > +    if ((args.size() >= 5) && args.at(4).isObject()) 
> > +        creationCallback = JSDatabaseCallback::create(asObject(args.at(4)), globalObject()); 
> > + 
> > +    ExceptionCode ec = 0; 
> > +    JSValue result = toJS(exec, globalObject(), WTF::getPtr(impl()->openDatabase(ustringToString(name), ustringToString(version), ustringToString(displayName), estimatedSize, creationCallback.release(), ec))); 
> > +    setDOMException(exec, ec); 
> > +    return result; 
> > +} 
> > +#endif
> 
> can you please update this file and follow the same pattern that openDatabaseSync() does? it's pretty much the same code, only with more error checking (abarth insisted on it).

Fixed.  Thanks for the example code; there wasn't a correct example in the whole project when he and I discussed it.

> > Index: WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp
> > ===================================================================
> > --- WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp	(revision 59110)
> > +++ WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp	(working copy)
> > @@ -42,6 +42,7 @@
> >  #include "ScheduledAction.h"
> >  #include "V8Binding.h"
> >  #include "V8BindingMacros.h"
> > +#include "V8Database.h"
> 
> should probably be inside #if ENABLE(DATABASE) too.

Fixed.

> > Index: WebCore/workers/WorkerContext.cpp
> > ===================================================================
> > @@ -275,13 +276,21 @@ PassRefPtr<Database> WorkerContext::open
> >          return 0;
> >      }
> >  
> > -    ASSERT(Database::isAvailable());
> >      if (!Database::isAvailable())
> >          return 0;
> 
> we should set 'ec = SECURITY_ERR' too. you can probably combine this 'if' with the previous one.

Yup; done.

> > Index: WebCore/workers/WorkerContext.idl
> > ===================================================================
> >  #if defined(ENABLE_DATABASE) && ENABLE_DATABASE
> > -        // Database openDatabase(in DOMString name, in DOMString version, in DOMString displayName, in unsigned long estimatedSize);
> > +        [EnabledAtRuntime, Custom] Database openDatabase(in DOMString name, in DOMString version, in DOMString displayName, in unsigned long estimatedSize, in DatabaseCallback creationCallback)
> > +            raises(DOMException);
> >          [EnableAtRuntime, Custom] DatabaseSync openDatabaseSync(in DOMString name, in DOMString version, in DOMString displayName, in unsigned long estimatedSize, in DatabaseCallback creationCallback);
> 
> oops, i have a bug here: s/Enable/Enabled/. can you please fix it since you're editing this code?

Fixed.

> also, i'm not sure we need [EnabledAtRuntime] here at all, since in the implemention of openDatabase{Sync}() we manually check if Database{Sync}.isAvailable(), but we can figure that out in a separate patch.

With the runtime enabler, V8 simply won't present the interface.  Without it, you can call the function, and you'll get an exception.  It's a small difference, but it's there.  I don't know how consistent we are about it, or if it makes any real difference to anyone.

> > Index: LayoutTests/fast/workers/storage/resources/database-worker-controller.js
> > ===================================================================
> > --- LayoutTests/fast/workers/storage/resources/database-worker-controller.js	(revision 0)
> > +++ LayoutTests/fast/workers/storage/resources/database-worker-controller.js	(revision 0)
> > @@ -0,0 +1,24 @@
> > +var databaseWorker = new Worker('resources/database-worker.js');
> > +
> > +databaseWorker.onerror = function(event) {
> > +    log("Caught an error from the worker!");
> > +    log(event);
> > +    for (var i in event) {
> > +        log("event[" + i + "]: " + event[i]);
> > +    }
> 
> no need for {}.

Removed.

> > +databaseWorker.onmessage = function(event) {
> > +    if (event.data.indexOf('log:') == 0)
> > +        log(event.data.substring(4));
> > +    else if (event.data == 'notifyDone') {
> > +        if (window.layoutTestController)
> > +                layoutTestController.notifyDone();
> 
> wrong indentation, should be 4 spaces.

Fixed.

> this is definitely beyond the scope of this patch, but i think it would be nice if we could figure out a way to share the *-expected.txt files between the DOM and worker storage tests too.

It would indeed.

I'll re-upload as soon as my build+test completes.

[Eric U.]

Created attachment 55737 [details]
Patch

[WebKit Review Bot]

Attachment 55737 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2190154

[Dmitry Titov]

Comment on attachment 55737 [details]
Patch

It needs a new method in RuntimeEnabledFeatures now...

[Eric U.]

Created attachment 55798 [details]
Patch

[Eric U.]

Comment on attachment 55798 [details]
Patch

Pretty much the same as the last patch, just based on newer code.  I made the fix, but since I took a while to do it [needed a sync+clean rebuild] Dumi snuck the fix in already.

[Dmitry Titov]

Comment on attachment 55798 [details]
Patch

r=me

[Dumitru Daniliuc]

Comment on attachment 55798 [details]
Patch

> Index: WebCore/workers/WorkerContext.cpp
> ===================================================================
>  PassRefPtr<Database> WorkerContext::openDatabase(const String& name, const String& version, const String& displayName, unsigned long estimatedSize, PassRefPtr<DatabaseCallback> creationCallback, ExceptionCode& ec)
>  {
> -    if (!securityOrigin()->canAccessDatabase()) {
> +    if (!securityOrigin()->canAccessDatabase() || !Database::isAvailable()) {
>          ec = SECURITY_ERR;
>          return 0;
>      }
>  
> -    ASSERT(Database::isAvailable());
> -    if (!Database::isAvailable())
> -        return 0;
> -
>      return Database::openDatabase(this, name, version, displayName, estimatedSize, creationCallback, ec);
>  }

if you haven't submitted this patch yet, please apply the same refactoring to openDatabaseSync (i missed it in my patches) -- no need to re-upload/re-review.

[Dmitry Titov]

(In reply to comment #38)
> if you haven't submitted this patch yet, please apply the same refactoring to openDatabaseSync (i missed it in my patches) -- no need to re-upload/re-review.

I don't think Eric is a committer yet. So we use commit queue... This can be a separate patch, right?

[Dumitru Daniliuc]

(In reply to comment #39)
> I don't think Eric is a committer yet. So we use commit queue... This can be a separate patch, right?

ah, forgot that. yes, this can definitely be a separate patch.

[WebKit Commit Bot]

Comment on attachment 55798 [details]
Patch

Clearing flags on attachment: 55798

Committed r59540: <http://trac.webkit.org/changeset/59540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.