[webkit-reviews] review denied: [Bug 124698] Hook up WebProcess-side of getOrEstablishIDBDatabaseMetadata : [Attachment 217521] Patch v1

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 124698: Hook up WebProcess-side of getOrEstablishIDBDatabaseMetadata
https://bugs.webkit.org/show_bug.cgi?id=124698

Attachment 217521: Patch v1
https://bugs.webkit.org/attachment.cgi?id=217521&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
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.