[webkit-reviews] review canceled: [Bug 34280] sqlite can silently roll back a transaction if executing a statement results in an error : [Attachment 47987] patch

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has canceled Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 34280: sqlite can silently roll back a transaction if executing a statement
results in an error
https://bugs.webkit.org/show_bug.cgi?id=34280

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

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
(In reply to comment #8)
> 397	  if (m_currentStatement->hasStatementErrorCallback() &&
> 398	      !m_sqliteTransaction->transactionWasRolledBackBySqlite()) {
> 
> Can this be on one line in the "no line is too long" webcore style?

done.

> 403	      // http://www.sqlite.org/lang_transaction.html recommends issuing
a
> manual ROLLBACK command,
> 404	      // even if the transaction was already rolled back by sqlite.
> That's why we don't call
> 405	      // m_sqliteTransaction->stop() here and allow
> cleanupAfterTransactionErrorCallback()
> 406	      // to eventually call m_sqliteTransaction->rollback().
> 
> Not sure you really need this comment here.

removed.

> 47	 bool transactionWasRolledBackBySqlite() const;
> 
> I wonder if this method name could be improved? Certainly 'transaction' in
the
> name seems redundant, wasRolledBackBySqlite() is a littler shorter.

done.

> 98	 return m_inProgress && m_db.isAutoCommitOn();
> 
> A comment a ptr to http://www.sqlite.org/lang_transaction.html may be better
in
> here.

added a reference to http://www.sqlite.org/c3ref/get_autocommit.html. i think
it states more clearly that this flag is off during a transaction, so if it's
on, then it means the transaction was rolled back.

> 9	 Test failed - there was not enough remaining storage space, or the
> storage quota was reached and the user declined to allow more space.
> 
> Can the test be arranged to differently to verify that the quota was bumped
up?

done.