[webkit-reviews] review denied: [Bug 124698] Hook up WebProcess-side of getOrEstablishIDBDatabaseMetadata : [Attachment 217521] Patch v1
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 21 10:59:43 PST 2013
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.
More information about the webkit-reviews
mailing list