Bug 104750 - webdatabase: Split the SQLTransaction between its front-end and back-end.
Summary: webdatabase: Split the SQLTransaction between its front-end and back-end.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on: 109109 109243
Blocks: 103668 104749
  Show dependency treegraph
 
Reported: 2012-12-11 17:41 PST by Mark Lam
Modified: 2013-02-14 18:04 PST (History)
14 users (show)

See Also:


Attachments
Preliminary patch to test the bots. (133.40 KB, patch)
2013-02-14 09:54 PST, Mark Lam
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
The patch (with build fixes, ChangeLog, and more comprehensive comments). (149.70 KB, patch)
2013-02-14 12:26 PST, Mark Lam
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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