Summary: | SubFrameSOAuthorizationSession should preserve the referrer header when falling back to web path | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||
Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, bfulgham, cdumez, commit-queue, jiewen_tan, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Jiewen Tan
2019-06-26 13:28:36 PDT
Created attachment 372947 [details]
Patch
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. 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. 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. 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. 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. Created attachment 372962 [details]
Patch
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. 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. Comment on attachment 372962 [details] Patch Clearing flags on attachment: 372962 Committed r246872: <https://trac.webkit.org/changeset/246872> All reviewed patches have been landed. Closing bug. |