[webkit-reviews] review denied: [Bug 27966] Race condition in the database code : [Attachment 34535] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dumitru Daniliuc <dumi at chromium.org>'s
request for review:
Bug 27966: Race condition in the database code
https://bugs.webkit.org/show_bug.cgi?id=27966

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
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()) {

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();

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

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

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

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