[webkit-reviews] review denied: [Bug 176628] [WK2] Add API to get the redirect chain of a WKDownload : [Attachment 321241] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 176628: [WK2] Add API to get the redirect chain of a WKDownload
https://bugs.webkit.org/show_bug.cgi?id=176628

Attachment 321241: Patch

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




--- Comment #23 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 321241
  --> https://bugs.webkit.org/attachment.cgi?id=321241
Patch

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

> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:148
> +    m_processPool->downloadClient().willSendRequest(m_processPool.get(),
this, proposedRequest, redirectResponse, [this, protectedThis](const
ResourceRequest& newRequest) {

protectedThis should be a Ref and we should either WTFMove it here so we don't
do unnecessary ref churn, or we should just create it in the lambda.

> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:149
> +	   m_redirectChain.append(newRequest.url());

Do we want the first URL to be in the redirect chain?

> Source/WebKit/UIProcess/WebFrameProxy.h:93
> +    const Vector<WebCore::URL>& redirectChain() const { return
m_redirectChain; }

Vector<WebCore::URL>&& takeProvisionalLoadRedirectChain()

> Source/WebKit/UIProcess/WebFrameProxy.h:146
> +    Vector<WebCore::URL> m_redirectChain;

A frame isn't redirected.
I think a better name for this variable would be
m_provisionalLoadRedirectChain.

> Source/WebKit/UIProcess/WebPageProxy.cpp:2281
> +	   download->setRedirectChain(frame.redirectChain());

If it's not a download, won't we still need to clear the redirect chain?  Could
you add a test that does more than one redirected load in the same frame?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:482
> +    if (redirectChain.count > 0)

We already made sure count was 2.