Bug 90310 - IndexedDB: Enable IDBFactory.deleteDatabase() and webkitGetDatabaseNames() in Workers
: IndexedDB: Enable IDBFactory.deleteDatabase() and webkitGetDatabaseNames() in...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Joshua Bell
:
Depends on: 90832
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-29 14:21 PDT by Joshua Bell
Modified: 2012-07-12 15:19 PDT (History)
5 users (show)

See Also:


Attachments
Patch (24.60 KB, patch)
2012-06-29 14:24 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (32.15 KB, patch)
2012-07-03 12:31 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (33.00 KB, patch)
2012-07-09 16:14 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (33.08 KB, patch)
2012-07-12 10:46 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-06-29 14:21:55 PDT
IndexedDB: Enable IDBFactory.deleteDatabase() and webkitGetDatabaseNames() in Workers
Comment 1 Joshua Bell 2012-06-29 14:24:06 PDT
Created attachment 150257 [details]
Patch
Comment 2 Joshua Bell 2012-06-29 14:25:50 PDT
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.
Comment 3 David Levin 2012-07-02 23:37:23 PDT
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.
Comment 4 Joshua Bell 2012-07-03 12:31:59 PDT
Created attachment 150659 [details]
Patch
Comment 5 Joshua Bell 2012-07-03 12:36:55 PDT
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.
Comment 6 Joshua Bell 2012-07-09 10:16:56 PDT
dgrogan@ - can you do an informal review? Sounds like levin@ may not be around for a bit.
Comment 7 David Grogan 2012-07-09 13:53:40 PDT
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.
Comment 8 Joshua Bell 2012-07-09 15:53:58 PDT
(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.
Comment 9 Joshua Bell 2012-07-09 16:14:51 PDT
Created attachment 151344 [details]
Patch
Comment 10 Joshua Bell 2012-07-09 16:16:14 PDT
(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.
Comment 11 Joshua Bell 2012-07-12 10:46:27 PDT
Created attachment 151997 [details]
Patch
Comment 12 Joshua Bell 2012-07-12 10:48:27 PDT
Rebased now that http://webkit.org/b/90832 has landed as http://trac.webkit.org/changeset/122463

tony@ - r?
Comment 13 WebKit Review Bot 2012-07-12 15:19:28 PDT
Comment on attachment 151997 [details]
Patch

Clearing flags on attachment: 151997

Committed r122515: <http://trac.webkit.org/changeset/122515>
Comment 14 WebKit Review Bot 2012-07-12 15:19:34 PDT
All reviewed patches have been landed.  Closing bug.