[webkit-reviews] review denied: [Bug 73609] Grant workers experimental access to IndexedDB. : [Attachment 117553] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied David Grogan
<dgrogan at chromium.org>'s request for review:
Bug 73609: Grant workers experimental access to IndexedDB.
https://bugs.webkit.org/show_bug.cgi?id=73609

Attachment 117553: Patch
https://bugs.webkit.org/attachment.cgi?id=117553&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117553&action=review


A few questions...

> Source/WebKit/chromium/ChangeLog:3
> +	   Grant workers experimental access to IndexedDB.

What do you mean experimental access?

> Source/WebCore/storage/IDBFactory.cpp:101
> +    m_backend->openFromWorker(name, request, context->securityOrigin(),
static_cast<WorkerContext*>(context), String());

Why have two methods open/openFromWorker when they have the same impl?


Get rid of that and a lot of this code collapses.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:106
> +void IDBFactoryBackendImpl::openFromWorker(const String& name,
PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<SecurityOrigin> securityOrigin,
WorkerContext*, const String& dataDir)

please avoid abbreviations: "dataDir"

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:158
> +{

Why is "callbacks" a PassRefPtr when nothing takes ownership of it in here?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:159
> +    const String uniqueIdentifier = computeUniqueIdentifier(name,
securityOrigin.get());

securityOrigin should be put in a local RefPtr since you are doing more than
just passing it directly to another function call.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:178
> +    m_databaseBackendMap.set(uniqueIdentifier, databaseBackend.get());

How does the lifetime of databaseBackend work?

It looks like it goes out of scope and nothing has a ref count on it.

Yes, I know this is the code that existed before :).

> Source/WebCore/workers/WorkerContext.cpp:532
> +	   m_idbFactoryBackendInterface =
IDBFactoryBackendInterface::create().get();

You shouldn't need to do "get" here.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:110
> +    void signalCompleted(bool result)

Why isn't "result" used?

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:115
> +		   createCallbackTask(&didComplete,
AllowCrossThreadAccess(this), result), m_mode);

AllowCrossThreadAccess is a really really bad sign.

Since "this" is a ThreadSafeRefCounted object, you shouldn't need to use it.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:121
> +	   , m_mode(mode)

why isn't m_result initialized?

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:125
> +				  AllowCrossThreadAccess(frame),

Everytime you put in AllowCrossThreadAccess, you need to be able to answer the
question: How does this stay alive until it is used in the callback?

Please answer for commonClient and frame.  Also remove it for "this".

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:149
> +    WebCore::WorkerLoaderProxy* m_workerLoaderProxy;

WebCore:: shouldn't be needed anywhere due to the using statement above.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:150
> +    WTF::String m_mode;

WTF shouldn't be needed iirc.

Even more critical, putting a String (a RefCounted object) in a
ThreadSafeRefCounted object is a no-no. Please find another way to accomplish
your goal.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:155
> +    WorkerContext* workerContext = static_cast<WorkerContext*>(context);

Assert that the context is a worker first. (I feel like we should have a
function that does this cast and assert but I guess we don't)

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:158
> +    WorkerLoaderProxy* workerLoaderProxy =
&workerContext->thread()->workerLoaderProxy();

Why the loader proxy? (This looks odd to me.)

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:169
> +    if (runLoop.runInMode(workerContext, mode) == MessageQueueTerminated) {

There is only a single task posted to this mode?

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:187
> +    m_webIDBFactory->open(name, new WebIDBCallbacksImpl(callbacks), origin,
webFrame, dataDir);

A raw new is a bad sign. Who owns this? Why doesn't it have an adoptPtr around
it?