[webkit-reviews] review granted: [Bug 112103] Loads are never canceled in the NetworkProcess : [Attachment 192641] Patch v1

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 112103: Loads are never canceled in the NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=112103

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=192641&action=review


> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:110
> +	   callOnMainThread(NetworkResourceLoader::performCleanups, 0);

We now have a version of callOnMainThread in WebCore that is type safe, or (as
helpful here) doesn't require passing an argument.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:157
> +#ifndef NDEBUG

Should probably check ASSERTIONS_ENABLED, not NDEBUG.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:173
> +template<typename U> void NetworkResourceLoader::sendAbortingOnFailure(const
U& message)
> +{
> +    if (!send(message))
> +	   abortInProgressLoad();

Maybe it's OK, but the name made me wonder if an actual abort() will be called.


Given that sendSync doesn't get this treatment, I'd consider just doing this
inline for consistency.

> Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:45
> +    virtual void connectionToWebProcessDidClose();

OVERRIDE

> Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h:47
>      virtual bool isSynchronous() { return true; }

Yes, here too.