[webkit-reviews] review denied: [Bug 28918] Add support for Database.readTransaction() : [Attachment 39219] patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 28918: Add support for Database.readTransaction()
https://bugs.webkit.org/show_bug.cgi?id=28918

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
These are all style nits. Overall, looks great!

> +	   * bindings/js/JSDatabaseCustom.cpp:
> +	   (WebCore::DatabaseTransactionHelper):
> +	   (WebCore::JSDatabase::transaction):
> +	   (WebCore::JSDatabase::readTransaction):
> +	   * bindings/v8/custom/V8CustomBinding.h:
> +	   * bindings/v8/custom/V8DatabaseCustom.cpp:
> +	   (WebCore::DatabaseTransactionHelper):
> +	   (WebCore::DatabaseReadTransaction):
> +	   * storage/Database.cpp:
> +	   (WebCore::Database::changeVersion):
> +	   (WebCore::Database::transaction):
> +	   * storage/Database.h:
> +	   * storage/Database.idl:
> +	   * storage/DatabaseAuthorizer.cpp:
> +	   (WebCore::DatabaseAuthorizer::createView):
> +	   (WebCore::DatabaseAuthorizer::createTempView):
> +	   (WebCore::DatabaseAuthorizer::dropView):
> +	   (WebCore::DatabaseAuthorizer::dropTempView):
> +	   * storage/DatabaseAuthorizer.h:
> +	   * storage/SQLTransaction.cpp:
> +	   (WebCore::SQLTransaction::create):
> +	   (WebCore::SQLTransaction::SQLTransaction):
> +	   (WebCore::SQLTransaction::executeSQL):
> +	   (WebCore::SQLTransaction::acquireLock):
> +	   * storage/SQLTransaction.h:
> +	   * storage/SQLTransactionCoordinator.cpp:
> +	   (WebCore::SQLTransactionCoordinator::acquireLock):
> +	   * storage/SQLTransactionCoordinator.h:

Would be good to have short descriptions in the ChangeLog.

> -JSValue JSDatabase::transaction(ExecState* exec, const ArgList& args)
> +static JSValue DatabaseTransactionHelper(ExecState* exec, const ArgList&
args, Database* database, bool readOnly)

camelCase, and badName :)

How about createTransaction?

>  
> -CALLBACK_FUNC_DECL(DatabaseTransaction)
> +static v8::Handle<v8::Value> DatabaseTransactionHelper(const v8::Arguments&
args, bool readOnly)

Ditto.

> +    m_transactionQueue.append(SQLTransaction::create(this, callback,
errorCallback, successCallback, ChangeVersionWrapper::create(oldVersion,
newVersion), false));

It's not obvious what "false" is for here. How about we just make readOnly a
default param and omit the "false" here?

> +    static PassRefPtr<SQLTransaction> create(Database*,
PassRefPtr<SQLTransactionCallback>, PassRefPtr<SQLTransactionErrorCallback>,
> +						PassRefPtr<VoidCallback>,
PassRefPtr<SQLTransactionWrapper>, bool readOnly);

bool readOnly = false

> -void SQLTransactionCoordinator::acquireLock(SQLTransaction* transaction)
> +void SQLTransactionCoordinator::acquireLock(SQLTransaction* transaction,
bool)

use UNUSED_PARAM here, bool by itself is lonely.

> +function runTest() {

brace on new line.