[webkit-reviews] review denied: [Bug 55919] Yet another multi-threading bug in WebSQLDatabase. : [Attachment 85012] fix

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Michael Nordman
<michaeln at google.com>'s request for review:
Bug 55919: Yet another multi-threading bug in WebSQLDatabase.
https://bugs.webkit.org/show_bug.cgi?id=55919

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85012&action=review

Thanks for doing this! 
Some notes:

> Source/WebCore/storage/SQLStatement.cpp:170
> +    RefPtr<SQLStatementErrorCallback> errorCallback =
m_statementErrorCallback.release();

This looks a bit confusing. The naming is such as if both 'callback' objects
are of the same type, and the only thing that goes on is refcount
manipulations.
Perhaps m_statementErrorCallback -> m_statementErrorCallbackBox or some other
name that would make the need for .release() (or unbox()?) obvious. Otherwise
code looks like strange assignment of RefPtrs.

> Source/WebCore/storage/SQLStatement.h:52
> +template<typename T> class SQLCallbackReference {

Indeed could be nice to have a file for it.

> Source/WebCore/storage/SQLStatement.h:90
> +    bool has() const

bool operator() could be clearer here...

> Source/WebCore/storage/SQLStatement.h:104
> +    mutable Mutex m_mutex;

Hmm, why do we need a mutex in this class? We didn't have one before, and it
seems posting the task to the context thread for destruction is all we need.
Or, is there more then one issue this patch is fixing?

> Source/WebCore/storage/SQLStatement.h:106
> +    RefPtr<ScriptExecutionContext> m_scriptExecutionContent;

m_scriptExecutionContent -> m_scriptExecutionContext