[webkit-reviews] review granted: [Bug 125127] Indexed Database work should be done on a non-main queue : [Attachment 218253] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 125127: Indexed Database work should be done on a non-main queue
https://bugs.webkit.org/show_bug.cgi?id=125127

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218253&action=review


> Source/WebKit2/DatabaseProcess/DatabaseProcess.h:77
> +    RefPtr<WorkQueue> m_queue;

Should use Ref for this instead of RefPtr.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:74
> +    m_processDatabaseRequestQueueCalled = true;

I don’t understand how this ever gets set back to false. It seems like once we
complete all there requests in the queue a single time we will no longer handle
any requests.

The flaw seems to be in the name too. It just says that this function was
“called”, which isn’t really what we want to know. I think it should be called
m_processingDatabaseQueueRequests or something like that.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:105
> +	       IDBDatabaseMetadata metadata;
> +	       completionCallback(false, metadata);

Could write this as a one-liner:

    completionCallback(false, IDBDatabaseMetadata());

Also, constant false is unclear here at this call site.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:74
> +    void enqueueDatabaseQueueRequest(PassRefPtr<AsyncRequest>);

I think this should be PassRef rather than PassRefPtr since it’s never null.
Been a while so I may have forgotten what the right type is.

> Source/WebKit2/Shared/AsyncRequest.h:47
> +    AsyncRequest(std::function<void ()> abortHandler);

Should mark this explicit.

> Source/WebKit2/Shared/AsyncRequest.h:83
> +	   : AsyncRequest(abortHandler)

Why no std::move here?