Bug 104750 – webdatabase: Split the SQLTransaction between its front-end and back-end.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mark Lam]

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

[Mark Lam]

Created attachment 188373 [details]
Preliminary patch to test the bots.

This is effectively near the final patch.  Uploading this to get a read on the ews bots first.  The style checker will not be happy, but I think the violations are justified (as a special case here) as fixing them actually makes the code less readable, not more.

Will up load a final patch after I add the ChangeLog and some additional comments.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/webdatabase/AbstractDatabaseServer.h', u'Source/WebCore/Modules/webdatabase/ChangeVersionData.h', u'Source/WebCore/Modules/webdatabase/ChangeVersionWrapper.cpp', u'Source/WebCore/Modules/webdatabase/ChangeVersionWrapper.h', u'Source/WebCore/Modules/webdatabase/Database.cpp', u'Source/WebCore/Modules/webdatabase/Database.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseBackend.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackendAsync.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseBackendAsync.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackendContext.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseBackendContext.h', u'Source/WebCore/Modules/webdatabase/DatabaseManager.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseManager.h', u'Source/WebCore/Modules/webdatabase/DatabaseServer.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseServer.h', u'Source/WebCore/Modules/webdatabase/DatabaseTask.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransaction.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransaction.h', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.h', u'Source/WebCore/Modules/webdatabase/SQLTransactionClient.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionCoordinator.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionState.h', u'Source/WebCore/Modules/webdatabase/SQLTransactionStateMachine.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionStateMachine.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', u'Source/WebCore/inspector/InspectorDatabaseAgent.cpp']" exit_code: 1
Source/WebCore/Modules/webdatabase/SQLTransactionState.h:35:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:80:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:81:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:82:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:83:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:84:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:85:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:86:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:87:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:88:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:90:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:91:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:92:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:59:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:309:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:310:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:311:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:313:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:314:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:315:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:317:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:318:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:319:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:320:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:321:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 25 in 29 files


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

[Early Warning System Bot]

Comment on attachment 188373 [details]
Preliminary patch to test the bots.

Attachment 188373 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16581402

[Early Warning System Bot]

Comment on attachment 188373 [details]
Preliminary patch to test the bots.

Attachment 188373 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16550732

[Mark Lam]

Created attachment 188400 [details]
The patch (with build fixes, ChangeLog, and more comprehensive comments).

[WebKit Review Bot]

Attachment 188400 [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/AbstractDatabaseServer.h', u'Source/WebCore/Modules/webdatabase/ChangeVersionData.h', u'Source/WebCore/Modules/webdatabase/ChangeVersionWrapper.cpp', u'Source/WebCore/Modules/webdatabase/ChangeVersionWrapper.h', u'Source/WebCore/Modules/webdatabase/Database.cpp', u'Source/WebCore/Modules/webdatabase/Database.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseBackend.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackendAsync.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseBackendAsync.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackendContext.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseBackendContext.h', u'Source/WebCore/Modules/webdatabase/DatabaseManager.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseManager.h', u'Source/WebCore/Modules/webdatabase/DatabaseServer.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseServer.h', u'Source/WebCore/Modules/webdatabase/DatabaseTask.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransaction.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransaction.h', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.h', u'Source/WebCore/Modules/webdatabase/SQLTransactionClient.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionCoordinator.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionState.h', u'Source/WebCore/Modules/webdatabase/SQLTransactionStateMachine.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionStateMachine.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', u'Source/WebCore/inspector/InspectorDatabaseAgent.cpp']" exit_code: 1
Source/WebCore/Modules/webdatabase/SQLTransactionState.h:35:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:80:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:81:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:82:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:83:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:84:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:85:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:86:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:87:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:88:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:90:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:91:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:92:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:62:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:391:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:392:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:393:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:395:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:396:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:397:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:399:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:400:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:401:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:402:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:403:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 25 in 30 files


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

[Sam Weinig]

Comment on attachment 188400 [details]
The patch (with build fixes, ChangeLog, and more comprehensive comments).

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

> Source/WebCore/Modules/webdatabase/ChangeVersionData.h:45
> +    const String& m_oldVersion;
> +    const String& m_newVersion;

These should be String.

> Source/WebCore/Modules/webdatabase/DatabaseBackendContext.cpp:37
> +    return static_cast<DatabaseContext*>(this);

This could use a comment.

> Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:95
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof(x[0]))

We already have WTF_ARRAY_LENGTH, which is a safer version of this.

[Adam Barth]

I tried to look at this patch, but I gave up because it was 149.70 KB.

[Mark Lam]

Sam, thanks for the review.  Adam, thanks for your attempt.  Sorry for the large size of the patch.

Landed in r142921: <http://trac.webkit.org/changeset/142921>.

[Michael Nordman]

@abarth: That's OK because sam has reviewed it all in short order, not even a single question asked :)

@mlam: I haven't looked too closely (there's quite a lot of it and its in bugzilla style instead of rietveld style which doesn't help me), the ChangeLog description is very nice and makes good sense. Hopefully the ews bots will be an accurate reflection of how this affects the chromium port. Certainly somewhat reflective, but they don't run under ASAN or TSAN and such (and its not really chrome that running there either). There's a reasonable chance we'll spot some fallout downstream, hopefully not, but its possible.

[Mark Lam]

(In reply to comment #10)
@michaeln, I sat next to Sam and answered questions as he reviewed it.

Here's a few things that might help put your mind at ease:
1. When I do the refactoring, I compare it to preexisting code, and use equivalence as the standard.  Where I can't achieve equivalence for the chromium port side, I reach out to you for clarification like I did last time.
2. I also do my own chromium build on mac and ran the relevant layout tests for it to make sure there was no breakage before I even submitted this patch for review. 

Regarding understanding the patch, I'd be happy to walk you thru it offline if that helps.

[Michael Nordman]

> @michaeln, I sat next to Sam and answered questions as he reviewed it.

I figured there was some out-of-band comm going on. Did any questions around using both templates<> and polymorphism for the state machine class come up? Seems like you could accomplish what you're doing with one or the other w/o having to invoke both fancy language features. It's a small thing.

> Here's a few things that might help put your mind at ease:
> 1. When I do the refactoring, I compare it to preexisting code, and use equivalence as the standard.  Where I can't achieve equivalence for the chromium port side, I reach out to you for clarification like I did last time.
> 2. I also do my own chromium build on mac and ran the relevant layout tests for it to make sure there was no breakage before I even submitted this patch for review. 

Thank you for doing that!

> Regarding understanding the patch, I'd be happy to walk you thru it offline if that helps.

Thnx, I may ping you at some point depending on wha' happens downstream.

[Mark Lam]

(In reply to comment #12)
> > @michaeln, I sat next to Sam and answered questions as he reviewed it.
> 
> I figured there was some out-of-band comm going on. Did any questions around using both templates<> and polymorphism for the state machine class come up? Seems like you could accomplish what you're doing with one or the other w/o having to invoke both fancy language features. It's a small thing.

We did not talk about that.  I did not choose to go with a template to start with, but it was necessary because I was forced to specialize the state function type (C++ restrictions).  Otherwise, I can't use the subclass methods with the base class state dispatch mechanism (which would defeat my purpose of having shared code).  The alternative is to use static methods for the state functions with lots of manual casting (and this medicine is worse than the ailment it aims to cure).

As for the use of virtual methods, that part is still open for debate.  I went with it for now because it is the most straightforward approach to me, and it is only a very small part of the patch (not worth dwelling on too long).  We can always revisit this and change it again later if needed.

[Michael Nordman]

Comment on attachment 188400 [details]
The patch (with build fixes, ChangeLog, and more comprehensive comments).

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

> Source/WebCore/Modules/webdatabase/SQLTransactionStateMachine.h:104
> +        m_nextState = (static_cast<T*>(this)->*stateFunction)();

I see...
but instead of retrieving a stateFunctionFor() to get a method ptr and then calling it a few lines down, you could just invoke a virtual changeToNextState(m_nextState) method...
or alternatively, not define stateFunctionFor() as virtual at all, T just must have such a method (need not be virtual) or the template won't compile

very much not a big deal