WebKit Bugzilla
IndexedDB: Enable IDBFactory.deleteDatabase() and webkitGetDatabaseNames() in Workers
Created attachment 150257 [details] Patch
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 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.
Created attachment 150659 [details] Patch
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.
dgrogan@ - can you do an informal review? Sounds like levin@ may not be around for a bit.
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.
(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.
Created attachment 151344 [details] Patch
(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.
Created attachment 151997 [details] Patch
Rebased now that http://webkit.org/b/90832 has landed as http://trac.webkit.org/changeset/122463 tony@ - r?
Comment on attachment 151997 [details] Patch Clearing flags on attachment: 151997 Committed r122515: <http://trac.webkit.org/changeset/122515>
All reviewed patches have been landed. Closing bug.