Bug 104750

Summary: webdatabase: Split the SQLTransaction 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: abarth, andersca, ap, beidson, ggaren, gyuyoung.kim, levin+threading, michaeln, rakuco, rego+ews, sam, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109109, 109243    
Bug Blocks: 103668, 104749    
Attachments:
Description Flags
Preliminary patch to test the bots.
webkit-ews: commit-queue-
The patch (with build fixes, ChangeLog, and more comprehensive comments). sam: review+

Description Mark Lam 2012-12-11 17:41:32 PST
Split the SQLTransaction between its front-end and back-end.
Comment 1 Mark Lam 2013-02-14 09:54:56 PST
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.
Comment 2 WebKit Review Bot 2013-02-14 09:59:05 PST
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.
Comment 3 Early Warning System Bot 2013-02-14 09:59:45 PST
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
Comment 4 Early Warning System Bot 2013-02-14 09:59:58 PST
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
Comment 5 Mark Lam 2013-02-14 12:26:21 PST
Created attachment 188400 [details]
The patch (with build fixes, ChangeLog, and more comprehensive comments).
Comment 6 WebKit Review Bot 2013-02-14 12:30:27 PST
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.
Comment 7 Sam Weinig 2013-02-14 14:06:14 PST
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.
Comment 8 Adam Barth 2013-02-14 14:23:51 PST
I tried to look at this patch, but I gave up because it was 149.70 KB.
Comment 9 Mark Lam 2013-02-14 14:36:28 PST
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>.
Comment 10 Michael Nordman 2013-02-14 14:45:03 PST
@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.
Comment 11 Mark Lam 2013-02-14 15:00:00 PST
(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.
Comment 12 Michael Nordman 2013-02-14 15:08:20 PST
> @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.
Comment 13 Mark Lam 2013-02-14 15:21:27 PST
(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.
Comment 14 Michael Nordman 2013-02-14 18:04:24 PST
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