Bug 124385

Summary: Move execution of IDBTransactionBackendOperations to the IDBServerConnection
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, andersca, buildbot, commit-queue, eflews.bot, gtk-ews, gyuyoung.kim, jsbell, rego+ews, rniwa, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1
buildbot: commit-queue-
Patch v2 - Less macros, more variadic templates...?
eflews.bot: commit-queue-
Patch v3 - Macros, and hopefully a successful build
buildbot: commit-queue-
Patch v4 - All the WebCore.xcodeproj changes needed... thorton: review+

Description Brady Eidson 2013-11-14 16:01:04 PST
Move execution of IDBTransactionBackendOperations to the IDBServerConnection

This almost removes all knowledge of the backing stores from the front end.
Comment 1 Brady Eidson 2013-11-14 21:07:04 PST
Created attachment 217010 [details]
Patch v1
Comment 2 Build Bot 2013-11-14 21:40:26 PST
Comment on attachment 217010 [details]
Patch v1 

Attachment 217010 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/23778008
Comment 3 Build Bot 2013-11-14 21:56:42 PST
Comment on attachment 217010 [details]
Patch v1 

Attachment 217010 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/23718007
Comment 4 Brady Eidson 2013-11-14 22:01:13 PST
(In reply to comment #3)
> (From update of attachment 217010 [details])
> Attachment 217010 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/23718007

Whoops, forgot to include .xcodeproj changes to export 2 more headers.

I'm reworking part of the patch right now anyways, will fix in the next version.
Comment 5 Brady Eidson 2013-11-14 22:57:09 PST
Created attachment 217016 [details]
Patch v2 - Less macros, more variadic templates...?

check-webkit-style will complain:
indexeddb/leveldb/IDBServerConnectionLevelDB.cpp:60:  Place brace on its own line for function definitions.  [whitespace/braces] [4]

But I think it needs updates for C++ lambdas.
Comment 6 WebKit Commit Bot 2013-11-14 22:59:20 PST
Attachment 217016 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/indexeddb/IDBDatabaseBackend.cpp', u'Source/WebCore/Modules/indexeddb/IDBDatabaseBackend.h', u'Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp', u'Source/WebCore/Modules/indexeddb/IDBServerConnection.h', u'Source/WebCore/Modules/indexeddb/IDBTransactionBackend.cpp', u'Source/WebCore/Modules/indexeddb/IDBTransactionBackend.h', u'Source/WebCore/Modules/indexeddb/IDBTransactionBackendOperations.cpp', u'Source/WebCore/Modules/indexeddb/IDBTransactionBackendOperations.h', u'Source/WebCore/Modules/indexeddb/leveldb/IDBBackingStoreLevelDB.cpp', u'Source/WebCore/Modules/indexeddb/leveldb/IDBBackingStoreLevelDB.h', u'Source/WebCore/Modules/indexeddb/leveldb/IDBServerConnectionLevelDB.cpp', u'Source/WebCore/Modules/indexeddb/leveldb/IDBServerConnectionLevelDB.h', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebProcessIDBDatabaseBackend.h']" exit_code: 1
Source/WebCore/Modules/indexeddb/leveldb/IDBServerConnectionLevelDB.cpp:60:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 EFL EWS Bot 2013-11-14 23:01:09 PST
Comment on attachment 217016 [details]
Patch v2 - Less macros, more variadic templates...?

Attachment 217016 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/23778026
Comment 8 EFL EWS Bot 2013-11-14 23:02:29 PST
Comment on attachment 217016 [details]
Patch v2 - Less macros, more variadic templates...?

Attachment 217016 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/23738028
Comment 9 kov's GTK+ EWS bot 2013-11-14 23:03:13 PST
Comment on attachment 217016 [details]
Patch v2 - Less macros, more variadic templates...?

Attachment 217016 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/23708023
Comment 10 Brady Eidson 2013-11-14 23:10:47 PST
Hmmm  the EFL/GTK errors seem to be deficiencies in their respective compilers parameter packs and lamdbas.  That's too bad...
Comment 11 Anders Carlsson 2013-11-15 06:45:58 PST
Comment on attachment 217016 [details]
Patch v2 - Less macros, more variadic templates...?

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

> Source/WebCore/Modules/indexeddb/leveldb/IDBServerConnectionLevelDB.cpp:68
> +template <typename FunctionType, typename... Arguments>
> +class CallOnDestruct {
> +public:
> +    CallOnDestruct(FunctionType callback, Arguments... arguments)
> +        : m_originalCallback(callback)
> +    {
> +        setArguments(arguments...);
> +    }
> +
> +    ~CallOnDestruct()
> +    {
> +        auto callback = m_callbackToPerform;
> +        callOnMainThread([callback]() {
> +            callback();
> +        });
> +    }
> +
> +    void setArguments(Arguments... arguments)
> +    {
> +        auto callback = m_originalCallback;
> +        m_callbackToPerform = [callback, arguments...]() {
> +            callback(arguments...);
> +        };
> +    }
> +
> +private:
> +    FunctionType m_originalCallback;
> +    std::function<void()> m_callbackToPerform;
> +};

I think this is more confusing than what you had earlier.
Comment 12 Brady Eidson 2013-11-15 07:41:55 PST
(In reply to comment #11)
> (From update of attachment 217016 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217016&action=review
> 
> > Source/WebCore/Modules/indexeddb/leveldb/IDBServerConnectionLevelDB.cpp:68
> > +template <typename FunctionType, typename... Arguments>
> > +class CallOnDestruct {
> > +public:
> > +    CallOnDestruct(FunctionType callback, Arguments... arguments)
> > +        : m_originalCallback(callback)
> > +    {
> > +        setArguments(arguments...);
> > +    }
> > +
> > +    ~CallOnDestruct()
> > +    {
> > +        auto callback = m_callbackToPerform;
> > +        callOnMainThread([callback]() {
> > +            callback();
> > +        });
> > +    }
> > +
> > +    void setArguments(Arguments... arguments)
> > +    {
> > +        auto callback = m_originalCallback;
> > +        m_callbackToPerform = [callback, arguments...]() {
> > +            callback(arguments...);
> > +        };
> > +    }
> > +
> > +private:
> > +    FunctionType m_originalCallback;
> > +    std::function<void()> m_callbackToPerform;
> > +};
> 
> I think this is more confusing than what you had earlier.

I know we're still looking for the right idiom here, and I definitely don't think the macros are it.

But since the EFL/GTK compilers can't handle the variadic class anyways, and this code is only for them for now, I'll go back to the macros + fixing the mac build.
Comment 13 Brady Eidson 2013-11-15 09:40:05 PST
Created attachment 217058 [details]
Patch v3 - Macros, and hopefully a successful build
Comment 14 Build Bot 2013-11-15 10:12:31 PST
Comment on attachment 217058 [details]
Patch v3 - Macros, and hopefully a successful build

Attachment 217058 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/23658197
Comment 15 Brady Eidson 2013-11-15 10:19:26 PST
Created attachment 217061 [details]
Patch v4 - All the WebCore.xcodeproj changes needed...
Comment 16 Brady Eidson 2013-11-15 12:55:56 PST
http://trac.webkit.org/changeset/159353