[webkit-reviews] review granted: [Bug 178855] Make NetworkRTCResolver port agnostic : [Attachment 335095] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 178855: Make NetworkRTCResolver port agnostic
https://bugs.webkit.org/show_bug.cgi?id=178855

Attachment 335095: Patch

https://bugs.webkit.org/attachment.cgi?id=335095&action=review




--- Comment #51 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 335095
  --> https://bugs.webkit.org/attachment.cgi?id=335095
Patch

LGTM!

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

> Source/WebCore/platform/network/DNS.h:40
> +    IPAddress(const struct sockaddr_in& address)

Should be explicit.

> Source/WebCore/platform/network/DNS.h:52
> +enum class DNSError { Unknown, CantResolve, Cancelled };

s/CantResolve/CannotResolve, not exactly sure what this means, is Timeout
better?
Unknown means there is an error? Maybe change it to General?

> Source/WebCore/platform/network/DNS.h:54
> +using DNSAddressesOrError =
Expected<std::reference_wrapper<Vector<WebCore::IPAddress>>, DNSError>;

I would go with Expected<Vector<WebCore::IPAddress>, DNSError>
We probably create a Vector anyway in our resolution and we can move it to
create DNSAddressesOrError.

> Source/WebCore/platform/network/DNS.h:59
> +WEBCORE_EXPORT void stopResolveDNS(uint64_t identifier);

As a side note, we have ObjectIdentifier which allows to type identifiers
without any runtime cost.
This allows ensuring we are not mixing identifiers at compile time.
See for instance ServiceWorkerTypes.h for some examples.
Maybe we could use this here as well, as a follow-up for instance.

> Source/WebCore/platform/network/cf/DNSResolveQueueCFNet.h:36
> +    void stopResolve(uint64_t identifier) final;

Do you need these to be public?

> Source/WebCore/platform/network/cf/DNSResolveQueueCFNet.h:39
> +    void updateIsUsingProxy() final;

Ditto here.

> Source/WebCore/platform/network/curl/DNSResolveQueueCurl.h:36
> +    void stopResolve(uint64_t identifier) final;

Ditto.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:35
> +#include <wtf/CompletionHandler.h>

Not sure this include is needed.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:101
> +    auto completionHandler = resolveQueue->getCompletionHandler(identifier);

Maybe there is a way to have takeCompletionHandler() which would allow getting
and removing it at the same time?

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:147
> +	   return nullptr;

I would add ASSERT(completionAndCancelHandlers !=
m_completionAndCancelHandlers.end()) since there is probably something wrong in
that case.
Or maybe add the assert at the call site?