Bug 104751 – webdatabase: Split the SQLStatement between its front-end and back-end.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mark Lam]

Split the SQLStatement between its front-end and back-end.

[Mark Lam]

Created attachment 188629 [details]
the proposed patch.

Let's give the ews bots a crack at it first.

[WebKit Review Bot]

Attachment 188629 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/webdatabase/Database.cpp', u'Source/WebCore/Modules/webdatabase/Database.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackend.h', u'Source/WebCore/Modules/webdatabase/SQLStatement.cpp', u'Source/WebCore/Modules/webdatabase/SQLStatement.h', u'Source/WebCore/Modules/webdatabase/SQLStatementBackend.cpp', u'Source/WebCore/Modules/webdatabase/SQLStatementBackend.h', u'Source/WebCore/Modules/webdatabase/SQLTransaction.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.h', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj']" exit_code: 1
Source/WebCore/Modules/webdatabase/SQLStatementBackend.cpp:142:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 188629 [details]
the proposed patch.

Looks ready for a review.

[Michael Nordman]

At a glance (emphasis on glance), this lgtm (fwiw)

Although i don't understand why in the diff viewer SQLStatementBackend.cpp appears twice, once with a SQLStatement class impl and another with a SQLStatementBackend impl. Maybe that's just showing the raw copy and then again after the edits to the raw copy.

[Mark Lam]

(In reply to comment #4)
> Although i don't understand why in the diff viewer SQLStatementBackend.cpp appears twice, once with a SQLStatement class impl and another with a SQLStatementBackend impl. Maybe that's just showing the raw copy and then again after the edits to the raw copy.

Yes, "SQLStatementBackend.cpp appears twice" because once for the svn copy from SVNStatement (so that we also inherit the change history from SQLStatement), and once for the changes on top of that copy.

[Geoffrey Garen]

Comment on attachment 188629 [details]
the proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=188629&action=review

r=me

> Source/WebCore/Modules/webdatabase/SQLStatement.h:49
>  class SQLStatement : public ThreadSafeRefCounted<SQLStatement> {

Since the SQLStatementBackend is the sole owner of this object, let's not use thread safe refcounting; instead, let's use OwnPtr inside SQLStatementBackend.

> Source/WebCore/Modules/webdatabase/SQLStatement.h:64
> +    SQLStatementBackend* m_backend;

This is a good place to comment that m_backend owns us, to explain why a raw pointer is OK here.

> Source/WebCore/Modules/webdatabase/SQLStatementBackend.cpp:67
> +//     - During this time, SQLTransaction operates on a raw SQLStatement* and not a RefPtr.
> +//       It is understood that the SQLTransactionBackend will be keeping the SQLStatement
> +//       alive via its SQLStatementBackend via SQLTransactionBackend::m_currentStatement
> +//       for this entire period.

How about clarifying something like:

Inside SQLTransaction::deliverStatementCallback(), we operate on a raw SQLStatement*. This pointer is valid because it is owned by SQLTransactionBackend's SQLTransactionBackend::m_currentStatementBackend.

> Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:383
> +SQLStatement* SQLTransactionBackend::currentStatement()
> +{
> +    return m_currentStatement->frontend();
> +}

Idiomatically, m_currentStatement implies an SQLStatement, not an SQLStatementBackend. Let's call it m_currentStatementBackend.

[Mark Lam]

Thanks for the review.  The code has been updated to address Geoff's comments.

Landed in r143090: <http://trac.webkit.org/changeset/143090>.