[webkit-reviews] review denied: [Bug 107784] webdatabase: Refactor DatabaseContext lookup to be thread-safe : [Attachment 185182] svn up'ed to fix patch apply problem.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 107784: webdatabase: Refactor DatabaseContext lookup to be thread-safe
https://bugs.webkit.org/show_bug.cgi?id=107784

Attachment 185182: svn up'ed to fix patch apply problem.
https://bugs.webkit.org/attachment.cgi?id=185182&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=185182&action=review


r- because I think DatabaseContext::contextDestroyed() is a bug.

The rest of my comments here would be good to resolve before landing, but don't
stand in the way of an r+.

> Source/WebCore/Modules/webdatabase/DatabaseContext.cpp:55
> +    DatabaseManager::manager().notifyDatabaseContextConstructed(); 

I prefer for notifications to specify whether the event in question happened
already, or is about to happen.

In this case, I think we want to notify when construction has happened already,
and when destruction is about to happen. Other options risk our client
dereferencing an object before or after its valid state.

I'd suggest moving this call to below the initializing call to
suspendIfNeeded(), since this object is not fully constructed until after that
call. Then, I would rename the notifications to "didConstructDatabaseContext"
and "willDestructDatabaseContext".

> Source/WebCore/Modules/webdatabase/DatabaseContext.cpp:65
> +    // Make sure that the DatabaseThread is stopped before we destruct.
> +    stopDatabases();

The comment here duplicates what the code says almost verbatim, increasing
total reading time for these two lines by about 3X. I think you should remove
it.

> Source/WebCore/Modules/webdatabase/DatabaseContext.cpp:70
> +    // This is because the Databases that are managed by DatabaseThread
still
> +    // relies on this ref between the context and the thread to execute the

Grammar: "relies" should be "rely".

> Source/WebCore/Modules/webdatabase/DatabaseContext.cpp:77
>      if (m_databaseThread) {
>	   ASSERT(m_databaseThread->terminationRequested());
>	   m_databaseThread = 0;
>      }

This code is a no-op. Clearing is the default behavior of the RefPtr
destructor. I would suggest this instead:

stopDatabases();
ASSERT(!m_databaseThread || m_databaseThread->terminationRequested());

Your comment about why stopDatabases() can't clear our m_databaseThread
reference should move into stopDatabases().

> Source/WebCore/Modules/webdatabase/DatabaseContext.cpp:95
> +    // However, we're here because the destructor hasn't been called, and
the
> +    // ScriptExecutionContext we're associated with is about to be
destructed.

I don't understand this comment or code. m_scriptExecutionContext is a RefPtr,
so it should be impossible for this function to be called while
m_scriptExecutionContext is non-NULL. What am I missing?

> Source/WebCore/Modules/webdatabase/DatabaseManager.h:65
> +    PassRefPtr<DatabaseContext> getDatabaseContext(ScriptExecutionContext*);


We use the "get" prefix to indicate an out parameter. I'd rename this to
"databaseContext" or "databaseContextFor".

> Source/WebCore/Modules/webdatabase/DatabaseManager.h:127
> +    PassRefPtr<DatabaseContext>
getExistingDatabaseContext(ScriptExecutionContext*);

We use the "get" prefix to indicate an out parameter. I'd rename this to
"existingDatabaseContext" or "existingDatabaseContextFor".

> Source/WebCore/Modules/webdatabase/DatabaseManager.h:129
> +    typedef HashMap<ScriptExecutionContext*, DatabaseContext*,
PtrHash<ScriptExecutionContext*> > ContextMap;

PtrHash is the default hash for pointers, and doesn't need to be specified.

> Source/WebCore/Modules/webdatabase/DatabaseManager.h:139
> +#if !ASSERT_DISABLED
> +    int m_databaseContextRegisteredCount;
> +    int m_databaseContextInstanceCount;
> +#endif

You should comment that these data members require locking m_contextMapLock.

> Source/WebCore/Modules/webdatabase/DatabaseThread.cpp:58
> +    // care of ensuring that a termination request has been issue. The

Grammar: "issue" should be "issued".

> Source/WebCore/Modules/webdatabase/DatabaseThread.cpp:60
> +    // termination request will trigger an orderly shutdown of the thread,
and
> +    // then deref itself.

And then what will deref itself? The request, or the thread?

> Source/WebCore/dom/ActiveDOMObject.cpp:60
> +    // m_scriptExecutionContext would/should have been nullify by

Grammar: "nullify" should be "nullified".