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
Mark Lam
2012-12-11 17:43:09 PST
Created attachment 188629 [details]
the proposed patch.
Let's give the ews bots a crack at it first.
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 on attachment 188629 [details]
the proposed patch.
Looks ready for a review.
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. (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 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. Thanks for the review. The code has been updated to address Geoff's comments. Landed in r143090: <http://trac.webkit.org/changeset/143090>. |