Bug 104751

Summary: webdatabase: Split the SQLStatement between its front-end and back-end.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: WebCore Misc.Assignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, beidson, ggaren, gyuyoung.kim, levin+threading, michaeln, rakuco, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 103668    
Attachments:
Description Flags
the proposed patch. ggaren: review+

Description Mark Lam 2012-12-11 17:43:09 PST
Split the SQLStatement between its front-end and back-end.
Comment 1 Mark Lam 2013-02-15 13:10:50 PST
Created attachment 188629 [details]
the proposed patch.

Let's give the ews bots a crack at it first.
Comment 2 WebKit Review Bot 2013-02-15 13:48:16 PST
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.
Comment 3 Mark Lam 2013-02-15 14:29:59 PST
Comment on attachment 188629 [details]
the proposed patch.

Looks ready for a review.
Comment 4 Michael Nordman 2013-02-15 14:56:22 PST
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.
Comment 5 Mark Lam 2013-02-15 14:58:37 PST
(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.
Comment 6 Geoffrey Garen 2013-02-15 18:16:47 PST
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.
Comment 7 Mark Lam 2013-02-15 19:06:59 PST
Thanks for the review.  The code has been updated to address Geoff's comments.

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