[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


Dumitru Daniliuc <dumi at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34617|review?                     |
               Flag|                            |
  Attachment #34617|0                           |1
        is obsolete|                            |
  Attachment #34873|                            |review?
               Flag|                            |




--- Comment #12 from Dumitru Daniliuc <dumi at chromium.org>  2009-08-14 15:02:27 PDT ---
Created an attachment (id=34873)
 --> (https://bugs.webkit.org/attachment.cgi?id=34873)
patch

(In reply to comment #11)
> 187             'storage/SQLTransactionClientImpl.cpp',
> Some of the make files make reference to SQLTransactionClientImpl.cpp, did you
> mean SQLTransactionClient.cpp here?

yes, fixed.

> 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?

added ASSERTs in the .cpp file. i don't think we should make assumptions for
all ports.

> 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

changed.

> 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).

done. you're right: SQLTransactionCoordinator should work with SQLTransactions.

> Also, the new class (TransactionCoordinator) you just added is also a
> transaction listener of sorts. Should these two classes be merged?

i think we will eventually merge them. for now though, i think it's better to
introduce SQLTransactionClient as a separate piece of code (to make it easier
to review and understand).

> 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?

done.

> 60     RefPtr<SecurityOrigin> origin = database->securityOriginCopy();
> 
> Making a copy of a refcounted object looks odd? Is it mutable? Is there no
> database->securityOrigin() accessor?

this seems to be the only accessor for getting the origin from a Database
object. we could probably introduce a new accessor (or replace the existing
one), but i wouldn't want to do this before talking to the authors of this code
and understanding why they had a copying accessor here in the first place.

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