Bug 124698 - Hook up WebProcess-side of getOrEstablishIDBDatabaseMetadata
Summary: Hook up WebProcess-side of getOrEstablishIDBDatabaseMetadata
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 124521
  Show dependency treegraph
 
Reported: 2013-11-20 23:21 PST by Brady Eidson
Modified: 2013-12-16 14:22 PST (History)
2 users (show)

See Also:


Attachments
Patch v1 (19.86 KB, patch)
2013-11-20 23:30 PST, Brady Eidson
andersca: review-
Details | Formatted Diff | Diff
Patch v2 (19.71 KB, patch)
2013-11-21 11:34 PST, Brady Eidson
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2013-11-20 23:21:49 PST
Hook up WebProcess-side of getOrEstablishIDBDatabaseMetadata

With this change, the indexedDB API in WK2 with the DatabaseProcess actually does something - window.indexedDB.open(...) calls the javascript error handler.
Comment 1 Brady Eidson 2013-11-20 23:30:26 PST
Created attachment 217521 [details]
Patch v1
Comment 2 WebKit Commit Bot 2013-11-20 23:31:04 PST
Attachment 217521 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.messages.in', u'Source/WebKit2/Shared/AsyncRequest.cpp', u'Source/WebKit2/Shared/AsyncRequest.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.h', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.messages.in']" exit_code: 1
Source/WebKit2/Shared/AsyncRequest.h:60:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/Shared/AsyncRequest.h:78:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/Shared/AsyncRequest.h:89:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brady Eidson 2013-11-20 23:53:37 PST
(In reply to comment #2)
> Attachment 217521 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', 
> Source/WebKit2/Shared/AsyncRequest.h:60:  Extra space before ( in function call  [whitespace/parens] [4]
> Source/WebKit2/Shared/AsyncRequest.h:78:  Extra space before ( in function call  [whitespace/parens] [4]
> Source/WebKit2/Shared/AsyncRequest.h:89:  Extra space before ( in function call  [whitespace/parens] [4]
> Total errors found: 3 in 10 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Not clear to me that these styles should be in place for std::function formats.
Comment 4 Anders Carlsson 2013-11-21 10:58:16 PST
Comment on attachment 217521 [details]
Patch v1

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

> Source/WebKit2/Shared/AsyncRequest.cpp:37
> +    DEFINE_STATIC_LOCAL(uint64_t, requestID, (0));

There’s no need to allocate a uint64_t on the heap. Just declare the ID variable static like we do for other IDs.

> Source/WebKit2/Shared/AsyncRequest.cpp:53
> +    m_abortHandler = handler;

should use std::move(handler)

> Source/WebKit2/Shared/AsyncRequest.cpp:58
> +    if (m_abortHandler != nullptr) {

We don’t check != nullptr, just if (m_abortHandler) should be enough.

> Source/WebKit2/Shared/AsyncRequest.cpp:69
> +void AsyncRequest::clearAbortHandler()
> +{
> +    m_abortHandler = nullptr;
> +}

I don’t think this function is needed.

> Source/WebKit2/Shared/AsyncRequest.h:44
> +    template<typename... Arguments> void requestCompleted(Arguments... arguments);

This should take a parameter pack of rvalue references: Arguments&&...

> Source/WebKit2/Shared/AsyncRequest.h:54
> +private:
> +    uint64_t m_requestID;
> +    std::function<void()> m_abortHandler;

Just make m_abortHandler protected, then you don’t need clearAbortHandler().

> Source/WebKit2/Shared/AsyncRequest.h:58
> +class SpecializedAsyncRequest FINAL : public AsyncRequest {

I think this should be called AsyncRequestImpl.

> Source/WebKit2/Shared/AsyncRequest.h:70
> +    void requestCompleted(Arguments... arguments)

This should take a parameter pack of rvalues, Arguments&&...

> Source/WebKit2/Shared/AsyncRequest.h:72
> +        m_completionHandler(arguments...);

You should use std::forward here to forward the arguments:

m_completionHandler(std::forward<Arguments>(arguments)…)

> Source/WebKit2/Shared/AsyncRequest.h:92
> +template<typename... Arguments> void AsyncRequest::requestCompleted(Arguments... arguments)

Again, rvalue reference pack.

> Source/WebKit2/Shared/AsyncRequest.h:94
> +    SpecializedAsyncRequest<Arguments...>* serverTask = static_cast<SpecializedAsyncRequest<Arguments...>*>(this);

You may have to use std::decay on the parameter pack here to be able to cast.

> Source/WebKit2/Shared/AsyncRequest.h:95
> +    serverTask->requestCompleted(arguments...);

Who deletes this task? This should also use std::forward.
Comment 5 Brady Eidson 2013-11-21 11:30:57 PST
(In reply to comment #4)
> > Source/WebKit2/Shared/AsyncRequest.h:95
> > +    serverTask->requestCompleted(arguments...);
> 
> Who deletes this task? 

The AsyncRequest itself is RefCounted and managed by its user.  In the case of this patch, WebIDBServerConnection.

New patch for the rest of the feedback on its way.
Comment 6 Brady Eidson 2013-11-21 11:34:21 PST
Created attachment 217587 [details]
Patch v2
Comment 7 WebKit Commit Bot 2013-11-21 11:35:26 PST
Attachment 217587 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.messages.in', u'Source/WebKit2/Shared/AsyncRequest.cpp', u'Source/WebKit2/Shared/AsyncRequest.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.h', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.messages.in']" exit_code: 1
Source/WebKit2/Shared/AsyncRequest.h:44:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebKit2/Shared/AsyncRequest.h:60:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/Shared/AsyncRequest.h:70:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebKit2/Shared/AsyncRequest.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/Shared/AsyncRequest.h:88:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/Shared/AsyncRequest.h:91:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 6 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Brady Eidson 2013-11-21 11:40:32 PST
Talked to Anders IRL re: style bot - We both agree it's wrong here.

Filed https://bugs.webkit.org/show_bug.cgi?id=124730 for that.
Comment 9 Brady Eidson 2013-11-21 11:41:01 PST
(In reply to comment #7)
> Attachment 217587 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.messages.in', u'Source/WebKit2/Shared/AsyncRequest.cpp', u'Source/WebKit2/Shared/AsyncRequest.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.h', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.messages.in']" exit_code: 1
> Source/WebKit2/Shared/AsyncRequest.h:44:  Missing spaces around &&  [whitespace/operators] [3]
> Source/WebKit2/Shared/AsyncRequest.h:70:  Missing spaces around &&  [whitespace/operators] [3]
> Source/WebKit2/Shared/AsyncRequest.h:91:  Missing spaces around &&  [whitespace/operators] [3]

Well it's wrong about this, too!  I'll file that also.
Comment 10 Brady Eidson 2013-11-21 11:46:31 PST
Filed https://bugs.webkit.org/show_bug.cgi?id=124731 for the (Arguments&&...) complaint.
Comment 11 Anders Carlsson 2013-11-21 11:48:14 PST
Comment on attachment 217587 [details]
Patch v2

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

> Source/WebKit2/Shared/AsyncRequest.h:62
> +        return adoptRef(new AsyncRequestImpl<Arguments...>(completionHandler));

std::move.

> Source/WebKit2/Shared/AsyncRequest.h:78
> +        : m_completionHandler(completionHandler)

std::move.
Comment 12 Brady Eidson 2013-11-21 11:58:04 PST
http://trac.webkit.org/changeset/159639