[webkit-reviews] review granted: [Bug 107702] Recursion handling cancelled authentication challenges in NetworkProcess : [Attachment 184257] 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 107702: Recursion handling cancelled authentication challenges in
NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=107702

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

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


r=me, because this fixes a bug and is generally an improvement. I suggest
addressing the below comments though.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:141
> +	   m_handle->cancel();

I don't know if we want to call this from resourceHandleStopped(). I'm pretty
sure that this is super confusing, but I don't know if it's necessary given how
the code is structured now.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:294
> +    send(Messages::WebResourceLoader::Cancel());

Can't we just send a didFail with a cancellation error? Having a cancel message
from NetworkProcess to WebProcess makes one wonder who's in charge (it's
normally WebKit client code that's in charge of cancellation, not
NetworkProcess).

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:120
> +    bool m_inProgress;

Can this be replaced with checks for m_handle being non-null?

> Source/WebKit2/WebProcess/Network/WebResourceLoader.messages.in:25
> +    Cancel()

As alluded to above, I'd have no idea what this message means from looking at
the IDL.