Bug 199232 – SubFrameSOAuthorizationSession should preserve the referrer header when falling back to web path

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

SubFrameSOAuthorizationSession should preserve the referrer header when fall back to web path.

[Jiewen Tan]

<rdar://problem/51718328>

[Jiewen Tan]

Created attachment 372947 [details]
Patch

[Chris Dumez]

Comment on attachment 372947 [details]
Patch

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

> Source/WebKit/UIProcess/WebFrameProxy.cpp:82
> +void WebFrameProxy::loadURL(const URL& url, const Optional<String>& referrer)

We normally do not use Optional<String> because the String class already has a null state.

[Chris Dumez]

Comment on attachment 372947 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:71
> +                if (!navigationActionPtr->request().hasHTTPReferrer()) {

This if check would not be needed if you did not use Optional. navigationActionPtr->request().httpReferrer() would return a null String and the normal code path would do the right thing.

[Chris Dumez]

Comment on attachment 372947 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1497
> +    if (!referrer) {

Ditto, this if check is likely not needed either if you stop using Optional<String> and use a simple String instead.

[Jiewen Tan]

Comment on attachment 372947 [details]
Patch

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

> Source/WebCore/platform/network/ResourceRequestBase.h:123
> +    WEBCORE_EXPORT bool hasHTTPReferrer() const;

Removed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:71
>> +                if (!navigationActionPtr->request().hasHTTPReferrer()) {
> 
> This if check would not be needed if you did not use Optional. navigationActionPtr->request().httpReferrer() would return a null String and the normal code path would do the right thing.

It does!

>> Source/WebKit/UIProcess/WebFrameProxy.cpp:82
>> +void WebFrameProxy::loadURL(const URL& url, const Optional<String>& referrer)
> 
> We normally do not use Optional<String> because the String class already has a null state.

Fixed.

[Jiewen Tan]

Comment on attachment 372947 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1497
>> +    if (!referrer) {
> 
> Ditto, this if check is likely not needed either if you stop using Optional<String> and use a simple String instead.

Removed.

[Jiewen Tan]

Created attachment 372962 [details]
Patch

[youenn fablet]

Comment on attachment 372962 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:71
> +                frame->loadURL(navigationActionPtr->request().url(), navigationActionPtr->request().httpReferrer());

I wonder whether other parameters might be needed in the future (cache policy, user agent...).
Maybe we should pass navigationActionPtr->request() directly.

[Jiewen Tan]

Comment on attachment 372962 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:71
>> +                frame->loadURL(navigationActionPtr->request().url(), navigationActionPtr->request().httpReferrer());
> 
> I wonder whether other parameters might be needed in the future (cache policy, user agent...).
> Maybe we should pass navigationActionPtr->request() directly.

Thanks, Youenn. That’s in my mind as well. However, I’m not sure if there will be any side effects to load the same request again. We will find out later if it is needed.

[WebKit Commit Bot]

Comment on attachment 372962 [details]
Patch

Clearing flags on attachment: 372962

Committed r246872: <https://trac.webkit.org/changeset/246872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.