[webkit-reviews] review granted: [Bug 178855] Make NetworkRTCResolver port agnostic : [Attachment 333685] 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 333685: Patch

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




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

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

> Source/WebCore/ChangeLog:13
> +	   No new tests because this is a refactor.

It would be nice if we could add some Unit tests to the functionalities exposed
in DNS.h

> Source/WebCore/ChangeLog:18
> +	   * SourcesCocoa.txt: Mov the DNSCFNet class to DNSResolveQueueCFNet.

s/Mov/Move.

> Source/WebKit/ChangeLog:8
> +	   Create an specific Cocoa class to isolate the generic code in the
base class, make the base implementation port

s/an/a/

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

This should take a ref instead of a pointer.

> Source/WebCore/platform/network/DNS.h:46
> +    const struct sockaddr_in* get() { return &m_address; };

Return a ref as well.

> Source/WebCore/platform/network/DNS.h:55
> +    virtual void completed(const Vector<IPAddress>&) { };

I guess sometimes we could get an error, which would call for something like
ErrorOr<Vector>.
Probably completed could take a Vector<IPAddress>&&.
Instead of DNSResolveObserver, could we take a lambda, something like a
Function<void(Vector<IPAddress>&&)>&&?

> Source/WebCore/platform/network/DNSResolveQueue.cpp:49
> +#endif

Instead of all those #ifdef, DNSResolveQueueSoup.h could directly define
DNSResolveQueuePlatform.
Ditto for Curl and CFNet.

> Source/WebCore/platform/network/DNSResolveQueue.cpp:80
>      , m_lastProxyEnabledStatusCheckTime(0)

Maybe m_isUsingProxy and m_lastProxyEnabledStatusCheckTime could be initialized
in DNSResolveQueue.h

> Source/WebCore/platform/network/DNSResolveQueue.h:49
> +    virtual void stopResolve(DNSResolveObserver*) = 0;

If we would pass a lambda here, resolve could return a uint64_t and stopResolve
would take this uint64_t.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:102
> +    addresses.reserveInitialCapacity(1);

I would create addresses after the early return below.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:109
> +    addresses.uncheckedAppend(WebCore::IPAddress(ipaddress));

Might be able to write something like
observer->completed(Vector<WebCore::IPAddress>::from(ipaddress))

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:35
> +class DNSResolveQueueSoup : public DNSResolveQueue {

Should be made final.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:39
> +    void stopResolve(DNSResolveObserver*) final;

These two can be made private?

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:42
> +    void updateIsUsingProxy() final;

Why is it protected? Can it be private as well.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:53
> +#endif

Or we could make NetworkRTCResolver.h declare a std::unique_ptr
createNetworkRTCResolver(...) which would be defined in COCOA/Soup specific CPP
files

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:189
> +	      
addresses.uncheckedAppend(RTCNetwork::IPAddress(rtc::IPAddress(address.get()->s
in_addr)));

You can probably write something like: 
auto addresses = WTF::map(result.value().get(), [] (auto& address) {
    return RTCNetwork::IPAddress { rtc::IPAddress { address.get()->sin_addr }
};
});

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:31
> +#include "WebCore/DNS.h"

Should be <WebCore/DNS.h> probably.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:44
> +    explicit NetworkRTCResolver(uint64_t identifier, CompletionHandler&&);

No need for explicit anymore.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:48
> +    virtual void stop();

Should use override.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.cpp:35
> +static void resolvedName(CFHostRef hostRef, CFHostInfoType typeInfo, const
CFStreamError *error, void *info)

CFStreamError*, void*

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.cpp:60
> +    resolver->completed(addresses);

Ditto for WTF::map.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.h:36
> +class NetworkRTCResolverCocoa : public NetworkRTCResolver {

Make class final.
Maybe add a FIXME to remove it once DNSResolver is implemented as well.