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

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has asked	for review:
Bug 40607: Implement the sync DB API in workers
https://bugs.webkit.org/show_bug.cgi?id=40607

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

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
> This patch is huge.

the layout tests are probably more than half of it. another good chunk of it is
changes to build files.

> I only reviewed the first half of it and there's already some concerning
stuff.	:(

happy to fix them. :)

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

fixed.

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

done.

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

done. i thought PassRefPtr::PassRefPtr(T*) and adoptRef() did the same thing.

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

done.

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

you're right, they are redundant. fixed, made these functions return an
ExceptionCode.

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

not sure what you mean, didn't change anything.

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

SQLStatement{Sync}::execute() do the following:
1. changes the authorizer mode to read-only, if it's a read-only transaction
(to make sure write-statements are disallowed).
2. prepares the statement.
3. makes sure the number of arguments matches the number of '?'s in the
statements.
4. bind all arguments.
5. step through the statement, and add data to the result set, one row at a
time.

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

oops, reverted, and added a 1-line google copyright. also fixed the copyright
notice in all other sync classes (they should all have the google copyright
header).

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

fixed.

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

removed.

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

it was around since... forever. michael had the same comment, so i removed the
'typedef int ExceptionCode;' line from all DB header files and added '#include
"ExceptionCode.h"'.

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

made them virtual, even though this is the only implementation we have (well, 1
implementation for chromium, and 1 for all other ports), and i think it's quite
unlikely we'll ever have another one.

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

same code as SQLTransaction::executeSQL(), only instead of storing a SQLError,
we set a SQLException.