Refactor DatabaseTask in preparation for removing threadsafe refcounting from it

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Dmitry Titov]

DatabaseTask contains lock inside itself so the main thread that posts the task for the database thread can wait of the task for the completion. Some operations on Database are synchronous and they are implemented this way.

This change moves the lock out from the task so it can be created separately and used on the main thread. This removes the need to keep the pointer to the task beyond posting it into the MessageQueue. This is needed to make MessageQueue OwnPtr-like and removing the threadsafe refcounting from the DatabaseTask. Also, the queue of SQL transactions can be just Deque rather then MessageQueue - it doesn't use the waitForMessage functionality anyways.

[Dmitry Titov]

Created attachment 42169 [details]
Proposed patch.

[Michael Nordman]

generally, looks reasonable to me

* Maybe ASSERT(synchronizer) in the ctors for the tasks that take references to variables on the callers stack in which the results will be stored.

38 DatabaseTaskSynchronizer::DatabaseTaskSynchronizer()
39     : m_taskCompleted(false)
40 {
41     m_synchronousMutex.set(new Mutex);
42     m_synchronousCondition.set(new ThreadCondition);
43 }

I think these data members should be defined inline w/o heap allocating them, as there's no reason to construct this class unless its going to be used for sync'ing. As data members of the Task class, they were rarely used, so heap allocating independently in that rare case made sense.

[Michael Nordman]

Also...

class Database {
  bool m_isProcessingTasks;
};

Maybe rename the new data member to m_isTransactionQueueEnabled.

Yong Li has a patch out for review that uses a very similarly named data member (in the DatabaseThread class i think) for a very different purpose, m_isProcessingTask (singular and actually applies to any DatabaseTask). Renaming this one in the Database class would could avoid cognitive collisions with that.

Also, I think m_isTransactionQueueEnabled is a more accurate name for this new data member irrespective of what's going on in DatabaseThread.

[Dmitry Titov]

Created attachment 42218 [details]
Updated patch.

Updated. All Michael's comments make perfect sense. Thanks!

[David Levin]

Comment on attachment 42218 [details]
Updated patch.

Just a few nits to address on check in.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        (WebCore::DatabaseTask::DatabaseTask): Ctor takes DatabaseTaskSynchronizer which can be NULL.

Use 0 instead of NULL (since NULL isn't used in the code).

> diff --git a/WebCore/storage/Database.h b/WebCore/storage/Database.h


> @@ -56,43 +78,19 @@ void DatabaseTask::performTask()
> +    if(m_synchronizer)

add space after if.


> diff --git a/WebCore/storage/DatabaseTask.h b/WebCore/storage/DatabaseTask.h
> +    static PassRefPtr<DatabaseOpenTask> create(Database* db, DatabaseTaskSynchronizer* synchronizer, ExceptionCode& code, bool& success) {

brace position.

> +    static PassRefPtr<DatabaseTransactionTask> create(PassRefPtr<SQLTransaction> transaction) {

brace position.

> +    static PassRefPtr<DatabaseTableNamesTask> create(Database* db, DatabaseTaskSynchronizer* synchronizer, Vector<String>& names) {

brace position.

There are several more functions with bad brace placement as well.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/50360