[webkit-reviews] review canceled: [Bug 27967] Decouple the code that deals with the main DB and quota management from the rest of the DB code : [Attachment 34617] patch

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has canceled Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 27967: Decouple the code that deals with the main DB and quota management
from the rest of the DB code
https://bugs.webkit.org/show_bug.cgi?id=27967

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

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
(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.