[Webkit-unassigned] [Bug 27966] Race condition in the database code

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 12 15:40:30 PDT 2009


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





--- Comment #22 from Dumitru Daniliuc <dumi at chromium.org>  2009-08-12 15:40:27 PDT ---
(In reply to comment #21)
> (From update of attachment 34535 [details])
> For the future (this one being in pre-existing code), WebKit code tends to
> favor early returns instead of long if-blocks:
> 544545     if (m_document->databaseThread()) {

fixed.

> Yes, our style is strange, but comments count as "lines" for multi-line if
> blocks and shoudl have  { } (I used to get this confused myself):
>  87     if (pendingTransactions.isEmpty())
>  88         // No more pending transactions; delete dbIdentifier's queue
>  89         m_pendingTransactions.remove(it);
>  90     else
>  91         // We have more pending transactions; start the next one
>  92         pendingTransactions.first()->lockAcquired();

fixed.

> No argument names when they don't add clarity:
>  46         void acquireLock(SQLTransaction* transaction);
>  47         void releaseLock(SQLTransaction* transaction);

removed.

> Why not add named functions for these:
>  34                      // statement success callback
>  35                      function(result) { log(dbName + " read statement
> succeeded"); },
>  36 
>  37                      // statement failure callback
>  38                      function(tx, error) { log(dbName + " read statement
> failed: " + error.message); });
> 
> Then you dont' have to declare them twice, nor do you need the extra lines of
> comments or spaces because the method names make it obvious what you're doing.
> 
> function successCallback(testName) {
> return function(result) { log(dbName + " " + testName + " statement
> succeeded"); }
> 
> successCallback("write")

done.

> If you wanted, you coudl even make them more general and cover the other places
> you use success/error callbacks too.

we could do this, but since the messages are quite different (read vs. write,
statement vs. transaction, and read/write doesn't even apply in case of a
transaction callback), i feel like this would only lead to more complicated and
unintuitive implementations for successCallback() and errorCallback(), as well
as some pretty cryptic calls to these functions.

> I'm ready to r+ this, but I think we should go one more round (since iirc you
> are not yet a committer).

i'm working my way there. :)

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



More information about the webkit-unassigned mailing list