Bug 178855 – Make NetworkRTCResolver port agnostic

• Component: WebRTC
• Status: RESOLVED FIXED

[Alejandro G. Castro]

We need these changes to use it in non cocoa ports.

[Alejandro G. Castro]

Created attachment 324982 [details]
Patch

[Alejandro G. Castro]

Created attachment 324984 [details]
Patch

[Alejandro G. Castro]

Created attachment 324986 [details]
Patch

[Alejandro G. Castro]

Created attachment 324988 [details]
Patch

[Alejandro G. Castro]

Created attachment 324989 [details]
Patch

[Sam Weinig]

The normal pattern we use for platform differences is to create a platform abstraction (in this case, maybe a DNSResolver or HostResolver class) in WebCore/platform and then use that. 

In this case, we have the beginning of an abstraction in WebCore already for DNS prefetching, DNSResolveQueue, that perhaps we could add to.

[Alejandro G. Castro]

(In reply to Sam Weinig from comment #6)
> The normal pattern we use for platform differences is to create a platform
> abstraction (in this case, maybe a DNSResolver or HostResolver class) in
> WebCore/platform and then use that. 
> 
> In this case, we have the beginning of an abstraction in WebCore already for
> DNS prefetching, DNSResolveQueue, that perhaps we could add to.

If reusing that class is an option that would be great for us, it was actually my plan to try to use the DNSSoup implementation in our port. If that is an option in your case too we can refactor this code.

[Alejandro G. Castro]

Created attachment 333372 [details]
Patch

[Alejandro G. Castro]

After talking to youenn about the best approach we have implemented a solution using the DNSResolveQueue that can be used by other ports to move the code platform dependent code to that class in the platform directory.

[Alejandro G. Castro]

Created attachment 333374 [details]
Patch

[Alejandro G. Castro]

Created attachment 333375 [details]
Patch

[Alejandro G. Castro]

Created attachment 333376 [details]
Patch

[Alejandro G. Castro]

Created attachment 333462 [details]
Patch

[Alejandro G. Castro]

Created attachment 333465 [details]
Patch

[Alejandro G. Castro]

Created attachment 333466 [details]
Patch

[Alejandro G. Castro]

Created attachment 333471 [details]
Patch

[Alejandro G. Castro]

Created attachment 333472 [details]
Patch

[Alejandro G. Castro]

Created attachment 333473 [details]
Patch

[Alejandro G. Castro]

Created attachment 333482 [details]
Patch

[Alejandro G. Castro]

Created attachment 333483 [details]
Patch

[Alejandro G. Castro]

Created attachment 333484 [details]
Patch

[Alejandro G. Castro]

Created attachment 333588 [details]
Patch

[Alejandro G. Castro]

Created attachment 333590 [details]
Patch

[Alejandro G. Castro]

Created attachment 333591 [details]
Patch

[Alejandro G. Castro]

Created attachment 333592 [details]
Patch

[Alejandro G. Castro]

Created attachment 333593 [details]
Patch

[Alejandro G. Castro]

Created attachment 333594 [details]
Patch

[Alejandro G. Castro]

Created attachment 333676 [details]
Patch

[Alejandro G. Castro]

Created attachment 333678 [details]
Patch

[Alejandro G. Castro]

Created attachment 333680 [details]
Patch

[Alejandro G. Castro]

Created attachment 333681 [details]
Patch

[Alejandro G. Castro]

Created attachment 333685 [details]
Patch

[youenn fablet]

Comment on attachment 333685 [details]
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()->sin_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.

[Alejandro G. Castro]

Thanks very much for the review!

(In reply to youenn fablet from comment #33)
> Comment on attachment 333685 [details]
> 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
> 

Right.

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

Right.

> > 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/
> 

Right.

> > Source/WebCore/platform/network/DNS.h:40
> > +    explicit IPAddress(const struct sockaddr_in* address)
> 
> This should take a ref instead of a pointer.
> 

Right.

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

Right.

> > 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>&&)>&&?
> 

That is a good point, NetworkRTCResolver is already doing that ErrorOr<Vector> with a lambda
created in the NetworkRTCProvider, the lambda is stored in the Resolver, that is why I thought to
avoid a bigger refactor in the Cocoa code using the Observer with the lambda and
the identifier is a better option. But I can try that option, that would mean we could remove
NetworkRTCResolver as soon as Cocoa class is removed, maybe I can even do that for Soup
already and add some ifdef in the Provider class with a FIXME. It is a step further but I can
try that.

Regarding the errors basically Resolver sends errors when we destroy the object and the operation
is still active it could do it in other situations I can add API to the
observer to set it (SetError), Cocoa currently has a FIXME saying there is no information and bailing
without calling complete. Now I think there is a bug in the current Cocoa upstream code, it
must call complete with empty list or libwebrtc would not know it is done. I think I'll fix
this in the patch.

Regarding the information libwebrtc just gets boolean for this so just unknown it is ok
in this case but it is true any other code using the API in the future could take advantage
of that information. Honestly I was tempted to do a bigger refactor of the whole API for the
DNS :-) but I'm not sure if there is any other situation other than RTC that will need it.
If you are hace a clearer understanding of the use of this code I can add more features.


> > Source/WebCore/platform/network/DNSResolveQueue.cpp:49
> > +#endif
> 
> Instead of all those #ifdef, DNSResolveQueueSoup.h could directly define
> DNSResolveQueuePlatform.
> Ditto for Curl and CFNet.
> 

Right.

> > Source/WebCore/platform/network/DNSResolveQueue.cpp:80
> >      , m_lastProxyEnabledStatusCheckTime(0)
> If you are hace a clearer understanding of the use of this code I can add more features.
> Maybe m_isUsingProxy and m_lastProxyEnabledStatusCheckTime could be
> initialized in DNSResolveQueue.h
> 

Right.

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

Right. I replied about this in a previous comment.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:102
> > +    addresses.reserveInitialCapacity(1);
> 
> I would create addresses after the early return below.

Right.

> 
> > 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))
> 

Right, I'll try to add that.

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

Right.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:39
> > +    void stopResolve(DNSResolveObserver*) final;
> 
> These two can be made private?
> 

I don't think so, they are used in DNS.cpp.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:42
> > +    void updateIsUsingProxy() final;
> 
> Why is it protected? Can it be private as well.
> 

It is used in the base class DNSResolveQueue and implemented in the
platform implementations. Before is was basically just defined per
platform, without inheritance.

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

If we pass the completionHandler down to the DNS.h API I would basically try
to remove the NetworkRtcResolver all together and make the provider call
directly: resolve and stopResolve. Passing the id and the completion handler.
I can try to do the dance of the Cocoa code in this step if I do that :-),
compilation is always painful but I think I did so much already I can
basically go all the way in the rabbit hole :-).

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:189
> > +            addresses.uncheckedAppend(RTCNetwork::IPAddress(rtc::IPAddress(address.get()->sin_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
> } };
> });
> 

Right.

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

Right.

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

Right.

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

Right.

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

Right.

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

Right.

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

Right. I'll try to check if I can remove it already.

Thanks again for the review.

[Alejandro G. Castro]

Created attachment 334149 [details]
Patch

[Alejandro G. Castro]

Created attachment 334150 [details]
Patch

[Alejandro G. Castro]

Created attachment 334152 [details]
Patch

[Alejandro G. Castro]

I added most of the comments, just avoided some of the changes in the Cocoa code and leave that for the moment where you can refactor that code because I do not have a working machine with that platform around to do bigger changes. At that point we can even try to get rid of the NetworkRTCResolver and call directly to the DNS.h APIs.

[Alejandro G. Castro]

Created attachment 334153 [details]
Patch

[Alejandro G. Castro]

Created attachment 334156 [details]
Patch

[Alejandro G. Castro]

Created attachment 334157 [details]
Patch

[Alejandro G. Castro]

Created attachment 334158 [details]
Patch

[youenn fablet]

Comment on attachment 334158 [details]
Patch

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

> Source/WebCore/platform/network/DNS.cpp:44
> +void resolveDNS(const String& hostname, uint64_t identifier, DNSCompletionHandler& completionHandler)

Probably a DNSCompletionHandler&&

> Source/WebCore/platform/network/DNS.cpp:50
> +    WebCore::DNSResolveQueue::singleton().resolve(hostname, identifier, completionHandler);

And WTFMove here.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:97
> +    auto* completionHandler = static_cast<DNSCompletionHandler*>(data);

It seems strange for DNSCompletionHandler to be passed directly here.
Usually when we pass a completion handler, we store it, so maybe DNSResolveQueueSoup could store it directly.
Or we would pass to DNSResolveQueueSoup the object owning the completion handler, which would be NetworkRTCResolver (that is in WebKit namespace now).

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

Can probably be made private at some point.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:47
> +    HashMap<uint64_t, GRefPtr<GCancellable>> m_cancelHandlers;

I would tend to augment this map to store pending completion handlers.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:180
> +            });

Remove 4 spaces?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:49
> +    WebCore::DNSCompletionHandler m_completionHandler;

If we let the queue store the completion handler, NetworkRTCResolver would just store the identifier.

[Alejandro G. Castro]

Thanks for the review! :-) All the changes you propose are because the completion handler is still owned by the NetworkRTCResolver, the reason I did not change this was I had to modify the cocoa code more deeply as I said in comment 38.

My proposal for the final design or the code would be to remove the NetworkRTCResolver object and let the NetworkRTCProvider call directly to the DNS API that stores the handler in the Queue but to do that I had to modify the cocoa code completely, that is why I avoided it and proposed this as a first step, and later when we decide if we want to remove the NetworkRTCResolver we store the handlers in the queue.

My proposal to go ahead is to write the final solution for soup and add an ifdef for cocoa in the RTCPRovider code, that you can remove when you implement it for cocoa. Does that sound good?

Thanks again for the review.

(In reply to youenn fablet from comment #43)
> Comment on attachment 334158 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334158&action=review
> 
> > Source/WebCore/platform/network/DNS.cpp:44
> > +void resolveDNS(const String& hostname, uint64_t identifier, DNSCompletionHandler& completionHandler)
> 
> Probably a DNSCompletionHandler&&
>

...

[Alejandro G. Castro]

Created attachment 335087 [details]
Patch

[Alejandro G. Castro]

Created attachment 335088 [details]
Patch

[Alejandro G. Castro]

Created attachment 335090 [details]
Patch

[Alejandro G. Castro]

Created attachment 335091 [details]
Patch

[Alejandro G. Castro]

Created attachment 335094 [details]
Patch

[Alejandro G. Castro]

Created attachment 335095 [details]
Patch

[youenn fablet]

Comment on attachment 335095 [details]
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?

[Alejandro G. Castro]

Thanks for the review! :-)

(In reply to youenn fablet from comment #51)
> Comment on attachment 335095 [details]
> 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.
> 

Right.

> > 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?
> 

It is the error we have in soup library SOUP_STATUS_CANT_RESOLVE, the documentation doesn't say much more as usual :-). I've checked the GIO library which is the one handling this and apparently there are 3 situations that are handled with that: the address is not found, network problem or similar, or unknown. It looks pretty general but all of them mean it can not resolve so I think it is not bad option for the name.

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

Right, I'll change the Cocoa code accordingly.

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

Sounds interesting option for a follow up refactor, the identifier comes from the WebProcess so probably we would need to modify all the IPC.

> > Source/WebCore/platform/network/cf/DNSResolveQueueCFNet.h:36
> > +    void stopResolve(uint64_t identifier) final;
> 
> Do you need these to be public?
> 

Yes, this is used to delegate the action for the stopResolveDNS API in DNS.h.

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

Right, this was was a leftover of one of the refactors.

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

This needs to be public.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:35
> > +#include <wtf/CompletionHandler.h>
> 
> Not sure this include is needed.
> 

I've checked it 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?
> 

Not sure, I'll check it.

> > 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?

Good point, I'll use an assertion.

Thanks again for the review.

[Alejandro G. Castro]

Created attachment 335187 [details]
Patch

[Alejandro G. Castro]

Created attachment 335188 [details]
Patch

[Alejandro G. Castro]

Created attachment 335189 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 335189 [details]
Patch for landing

Rejecting attachment 335189 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 335189, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
e too many unreachable loose objects; run 'git prune' to remove them.

Current branch master is up to date.
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
error: The last gc run reported the following. Please correct the root cause
and remove /Volumes/Data/EWS/WebKit/.git/gc.log.
Automatic cleanup will not be performed until the file is removed.

warning: There are too many unreachable loose objects; run 'git prune' to remove them.


Full output: http://webkit-queues.webkit.org/results/6839248

[Alejandro G. Castro]

commit-queue failed in a weird way, it landed the patch but failed to update, apparently some performance problem with git.

https://trac.webkit.org/changeset/229359/webkit

Anyway it seems everything worked so closing the bug.

Thanks everyone.

[Radar WebKit Bug Importer]

<rdar://problem/38222040>