[webkit-reviews] review denied: [Bug 40607] Implement the sync DB API in workers : [Attachment 60445] patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 40607: Implement the sync DB API in workers
https://bugs.webkit.org/show_bug.cgi?id=40607

Attachment 60445: patch #3: implementing DatabaseSync::transaction() and
changeVersion() + tests
https://bugs.webkit.org/attachment.cgi?id=60445&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
This patch is huge.  I only reviewed the first half of it and there's already
some concerning stuff.	:(

WebCore/storage/DatabaseSync.cpp:95
 +	    return new ChangeVersionPreflightStep(expectedVersion);
AdoptRef ???  This looks like a leak.

WebCore/storage/DatabaseSync.cpp:115
 +	ChangeVersionPreflightStep(const String& expectedVersion) :
m_expectedVersion(expectedVersion) { }
explicit?  Please make this more than one line.

WebCore/storage/DatabaseSync.cpp:123
 +	    return new ChangeVersionPostflightStep(newVersion);
adoptRef.  If you haven't already, please read
http://webkit.org/coding/RefPtr.html

WebCore/storage/DatabaseSync.cpp:138
 +	ChangeVersionPostflightStep(const String& newVersion) :
m_newVersion(newVersion) { }
explicit, multiline.

WebCore/storage/DatabaseSync.cpp:162
 +	if (!transaction->begin(ec) || !transaction->execute(ec) ||
!transaction->commit(ec))
Can these return ec and also true?  If not, that seems redundant.

WebCore/storage/SQLStatementSync.cpp:27
 +   */
missing space

WebCore/storage/SQLStatementSync.cpp:59
 +  bool SQLStatement::execute(Database* db)
This function is too complicated for me to understand at 4am.

WebCore/storage/SQLStatementSync.cpp:2
 +   * Copyright (C) 2010 Google Inc. All rights reserved.
Woah there.  You can't just change the copyright on a file from Apple to
Google.  What's going on here with the license block?

WebCore/storage/SQLStatementSync.cpp:75
 +	for (unsigned int i = 0; i < m_arguments.size(); ++i) {
We usually just say "unsigned"

WebCore/storage/SQLStatementSync.h:54
 +  
Extra blank line.

WebCore/storage/SQLTransaction.h: 
 +  typedef int ExceptionCode;
Who review this line of code?  It's nonsense.  (Hope it wasn't me!)

WebCore/storage/SQLTransactionClient.h:44
 +  class SQLTransactionClient : public Noncopyable {
If this is a client, why aren't the methods virtual?

WebCore/storage/SQLTransactionSync.cpp:80
 +  PassRefPtr<SQLResultSet> SQLTransactionSync::executeSQL(const String&
sqlStatement, const Vector<SQLValue>& arguments, ExceptionCode& ec)
again, too big for my little brain.