[Webkit-unassigned] [Bug 27967] Decouple the code that deals with the main DB and quota management from the rest of the DB code

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27967





--- Comment #11 from Michael Nordman <michaeln at google.com>  2009-08-13 00:23:35 PDT ---
187             'storage/SQLTransactionClientImpl.cpp',
Some of the make files make reference to SQLTransactionClientImpl.cpp, did you
mean SQLTransactionClient.cpp here?

330     m_transactionClient->databaseSizeChanged(m_database.get());
So this method is invoked on the background db thread. 

392     m_shouldRetryCurrentStatement =
m_transactionClient->databaseExceedsQuota(...);
And this one on the 'apartment' thread of the Database object (the main thread)

426     m_transactionClient->databaseChanged(m_database.get());
And this one on the background thread

I would appreciate some indication of the threading usage in the
SQLTransactionListener source files. To figure out how it was used, i had to
track down the callsites. Not sure how to best indicate the threading rules...
either doc comments in the .h file, or runtime asserts in the .cpp file?

38     // A "listener" interface to the SQLTransaction class. Allows
 39     // SQLTransaction to notify interested parties when certain things are
 40     // happening in a transaction.
 41     class SQLTransactionClient {
 42     public:
 43         // Notifies everybody that the database has changed. Called at the
end
 44         // end of every successful transaction.
 45         void databaseChanged(Database*);
 46 
 47         // Notifies everybody that the size of the database file might have
changed.
 48         // Called after executing each statement in a transaction
 49         void databaseSizeChanged(Database*);
 50 
 51         // Notifies everybody that the size of the database file exceeds
its quota.
 52         // If this method returns true, the current statement in the
transaction is
 53         // retried. Otherwise, the transaction is rolled back.
 54         //
 55         // This method should return true if and only if the quota for this
database
 56         // file was increased (by the user, for example) since calling this
method.
 57         bool databaseExceedsQuota(Database*);
 58     };
 59 }

After reading your doc comments, maybe these methods could be renamed for
clarity.

void didCommitTransaction(...);  // for databaseChanged
void didExecuteStatement(...);  // for databaseSizeChanged
bool didExceedQuota(...);  // for databaseExceedsQuota

And would it make sense to have the SQLTransaction* be the parameter since it
is a transaction listener, the caller can get the Database* from the accessor
on the transaction object (i think).

Also, the new class (TransactionCoordinator) you just added is also a
transaction listener of sorts. Should these two classes be merged? The
TransactionCoordinator must be owned by the DBThread to properly serve its
purpose. In this patch, you have the new 'listener' object being owned by the
Database object, but i think it could just as easily be owned by the DBThread
and serve its purpose.

Wdyt?

60     RefPtr<SecurityOrigin> origin = database->securityOriginCopy();

Making a copy of a refcounted object looks odd? Is it mutable? Is there no
database->securityOrigin() accessor?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.