Bug 90310 – IndexedDB: Enable IDBFactory.deleteDatabase() and webkitGetDatabaseNames() in Workers

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Enable IDBFactory.deleteDatabase() and webkitGetDatabaseNames() in Workers

[Joshua Bell]

Created attachment 150257 [details]
Patch

[Joshua Bell]

Seemed pretty straightforward, but dgrogan@ should take a look.

Note that this will collide with https://bugs.webkit.org/show_bug.cgi?id=88338 but the resolution should be pretty straightforward.

[David Levin]

Comment on attachment 150257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150257&action=review

I'm fine with others completely the review (especially since I will be out for a while but I wanted to push this code change to reduce duplicate code).

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:134
> +#endif

I think a lot of this code would be simpler if the difference between the worker and the document were handled down in the backend as opposed to being exposed here.


Your call would look like this:
  RefPtr<IDBVersionChangeRequest> request = IDBVersionChangeRequest::create(context, IDBAny::createNull(), "");
  m_backend->deleteDatabaseFromWorker(name, request, context, String());

with no ifdef workers.

Ditto for getDatabaseNamesFromWorker

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:184
> +    WebSecurityOrigin origin(prpOrigin);

Continuing the idea of only having getDatabaseNames (and not getDatabaseNamesFromWorker)


void IDBFactoryBackendProxy::getDatabaseNames(PassRefPtr<IDBCallbacks> callbacks, ScriptExecutionContext* context, const String& dataDir)
{
    WebSecurityOrigin origin(context->securityOrigin());
    if (!allowIndexedDB(context, "Database Listing", origin, callbacks))
        return;
  
    WebFrameImpl* webFrame = getWebFrame(context);
    m_webIDBFactory->getDatabaseNames(new WebIDBCallbacksImpl(callbacks), origin, webFrame, dataDir);
}


Then make allowIndexedDB and getWebFrame of the right thing for WorkerContext vs Document. All your ifdef's move to that one piece of code and a lot of your duplicate code collapses and gets removed.

[Joshua Bell]

Created attachment 150659 [details]
Patch

[Joshua Bell]

Thanks for the push to simplify this, David!

(In reply to comment #3)
> I think a lot of this code would be simpler if the difference between the worker and the document were handled down in the backend as opposed to being exposed here.

Done!

> with no ifdef workers.

One #ifdef remains in IDBFactory.cpp to handle the early exit case if workers are disabled, but factored out into a single static helper function.
 
> void IDBFactoryBackendProxy::getDatabaseNames(PassRefPtr<IDBCallbacks> callbacks, ScriptExecutionContext* context, const String& dataDir)

Since both the proxy impl. and real backend impl. need to implement the interface, the methods do end up needing a SecurityOrigin parameter. This is redundant for the proxy since it can be gleaned from the context...

>     WebSecurityOrigin origin(context->securityOrigin());

...but the context is null on the real backend and it needs the origin passed in. Otherwise, basically copy/pasta what you suggested.

[Joshua Bell]

dgrogan@ - can you do an informal review? Sounds like levin@ may not be around for a bit.

[David Grogan]

Comment on attachment 150659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150659&action=review

LGTM

The re-org is nice.  Looks like you'll have to rebase since https://bugs.webkit.org/show_bug.cgi?id=88338 landed.

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:83
> +    ASSERT(context->isDocument() || context->isWorkerContext());

Is this line removable as it's in isContextValid?

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:111
> +    ASSERT(context->isDocument() || context->isWorkerContext());

Here too.

[Joshua Bell]

(In reply to comment #7)
> (From update of attachment 150659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150659&action=review
> 
> LGTM
> 
> The re-org is nice.  Looks like you'll have to rebase since https://bugs.webkit.org/show_bug.cgi?id=88338 landed.

Ugh - 88338 actually broke this. The method that is used to fetch the IDB path is not safe to call from a worker outside of the initial script evaluation - the data it references is cleared before the event loop is run.

> 
> > Source/WebCore/Modules/indexeddb/IDBFactory.cpp:83
> > +    ASSERT(context->isDocument() || context->isWorkerContext());
> 
> Is this line removable as it's in isContextValid?

Correct, I'll remove it.

[Joshua Bell]

Created attachment 151344 [details]
Patch

[Joshua Bell]

(In reply to comment #9)
> Created an attachment (id=151344) [details]
> Patch

Latest patch is rebased, but has issues (see http://webkit.org/b/90832). The bit that is broken by that bug is avoided in this patch by an #if 0, but we can wait and let a fix for that one be created first.

[Joshua Bell]

Created attachment 151997 [details]
Patch

[Joshua Bell]

Rebased now that http://webkit.org/b/90832 has landed as http://trac.webkit.org/changeset/122463

tony@ - r?

[WebKit Review Bot]

Comment on attachment 151997 [details]
Patch

Clearing flags on attachment: 151997

Committed r122515: <http://trac.webkit.org/changeset/122515>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.