Bug 124698 – Hook up WebProcess-side of getOrEstablishIDBDatabaseMetadata

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 217521 [details]
Patch v1

[WebKit Commit Bot]

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.

[Brady Eidson]

(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.

[Anders Carlsson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 217587 [details]
Patch v2

[WebKit Commit Bot]

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.

[Brady Eidson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Filed https://bugs.webkit.org/show_bug.cgi?id=124731 for the (Arguments&&...) complaint.

[Anders Carlsson]

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.

[Brady Eidson]

http://trac.webkit.org/changeset/159639