We should add support for specifying non app-bound requests, mainly populating sub resource loads with this value.
<rdar://problem/73512988>
Created attachment 420337 [details] Patch
Comment on attachment 420337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420337&action=review > Source/WebCore/loader/DocumentLoader.cpp:2334 > + m_requestIsNonAppBound = requestIsNonAppBound; Can be moved inline in the header. > Source/WebCore/platform/network/ResourceRequestBase.cpp:773 > +bool ResourceRequestBase::isNonAppBound() const Can be inline in header. > Source/WebCore/platform/network/ResourceRequestBase.cpp:778 > +void ResourceRequestBase::setIsNonAppBound(bool isNonAppBound) ditto. > Source/WebCore/platform/network/ResourceRequestBase.h:264 > + bool m_isNonAppBound : 1; Should probably be moved before the Optional data member and with the other bits for better packing. > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:90 > +- (void)_isNonAppBoundRequest:(void(^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); Internally this is IOS_FAMILY only, so why is the SPI both macOS too?
Comment on attachment 420337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420337&action=review >> Source/WebCore/platform/network/ResourceRequestBase.cpp:773 >> +bool ResourceRequestBase::isNonAppBound() const > > Can be inline in header. Will fix both of these. >> Source/WebCore/platform/network/ResourceRequestBase.h:264 >> + bool m_isNonAppBound : 1; > > Should probably be moved before the Optional data member and with the other bits for better packing. Ok, I will move this. >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:90 >> +- (void)_isNonAppBoundRequest:(void(^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); > > Internally this is IOS_FAMILY only, so why is the SPI both macOS too? Mistake, I'll remove it.
(In reply to katherine_cheney from comment #4) > Comment on attachment 420337 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420337&action=review > > >> Source/WebCore/platform/network/ResourceRequestBase.cpp:773 > >> +bool ResourceRequestBase::isNonAppBound() const > > > > Can be inline in header. > > Will fix both of these. > I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason.
Comment on attachment 420337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420337&action=review >>>> Source/WebCore/platform/network/ResourceRequestBase.cpp:773 >>>> +bool ResourceRequestBase::isNonAppBound() const >>> >>> Can be inline in header. >> >> Will fix both of these. > > I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason. Ignore stylebot in this case.
Created attachment 420354 [details] Patch
(In reply to Alex Christensen from comment #6) > Comment on attachment 420337 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420337&action=review > > >>>> Source/WebCore/platform/network/ResourceRequestBase.cpp:773 > >>>> +bool ResourceRequestBase::isNonAppBound() const > >>> > >>> Can be inline in header. > >> > >> Will fix both of these. > > > > I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason. > > Ignore stylebot in this case. I think it may be causing failures on the debug bot now: ERROR: WebCore has a weak external symbol in it (/Volumes/Data/worker/macOS-Catalina-Debug-Build-EWS/build/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore) ERROR: A weak external symbol is generated when a symbol is defined in multiple compilation units and is also marked as being exported from the library. ERROR: A common cause of weak external symbols is when an inline function is listed in the linker export file. ERROR: symbol __ZN7WebCore19ResourceRequestBase16setIsNonAppBoundEb ERROR: symbol __ZNK7WebCore19ResourceRequestBase13isNonAppBoundEv
Comment on attachment 420354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420354&action=review > Source/WebCore/loader/DocumentLoader.cpp:2332 > +void DocumentLoader::setRequestIsNonAppBound(bool requestIsNonAppBound) Please move to header. > Source/WebCore/loader/DocumentLoader.h:425 > + WEBCORE_EXPORT void setRequestIsNonAppBound(bool); Drop WEBCORE_EXPORT once you've inlined in the header.
(In reply to Alex Christensen from comment #6) > Comment on attachment 420337 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420337&action=review > > >>>> Source/WebCore/platform/network/ResourceRequestBase.cpp:773 > >>>> +bool ResourceRequestBase::isNonAppBound() const > >>> > >>> Can be inline in header. > >> > >> Will fix both of these. > > > > I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason. > > Ignore stylebot in this case. I believe the correct fix is to stop the WEBCORE_EXPORT on the inline function in the header, as the style bot suggested.
(In reply to Chris Dumez from comment #10) > (In reply to Alex Christensen from comment #6) > > Comment on attachment 420337 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=420337&action=review > > > > >>>> Source/WebCore/platform/network/ResourceRequestBase.cpp:773 > > >>>> +bool ResourceRequestBase::isNonAppBound() const > > >>> > > >>> Can be inline in header. > > >> > > >> Will fix both of these. > > > > > > I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason. > > > > Ignore stylebot in this case. > > I believe the correct fix is to stop the WEBCORE_EXPORT on the inline > function in the header, as the style bot suggested. I see. I didn't realize that WEBCORE_EXPORT is not needed if the function is inlined in the header, but that makes sense.
Created attachment 420375 [details] Patch
Comment on attachment 420375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420375&action=review > Source/WebCore/loader/DocumentLoader.h:424 > + bool requestIsNonAppBound() const { return m_requestIsNonAppBound; } I am no familiar with the 'app-bound' feature at all but I am wondering why we have the negative here in the API. Why aren't we using "isAppBound" instead of "isNonAppBound". Seems to be (at least) that it would be more readable. > Source/WebKit/Shared/NavigatingToAppBoundDomain.h:30 > enum class NavigatingToAppBoundDomain : bool { Yes, No }; Ugh. Please reverse the No and Yes here so that 0 means No and 1 means Yes. > Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:312 > +- (void)_isNonAppBoundRequest:(void(^)(BOOL))completionHandler Again, I wish this was called isAppBoundRequest > Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:314 > +#if PLATFORM(IOS_FAMILY) Seems like this should cover those whole _isNonAppBoundRequest method implementation now that this method is iOS only? > Source/WebKit/UIProcess/WebPageProxy.h:2901 > + bool m_requestIsNonAppBound { false }; It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page. > Source/WebKit/WebProcess/WebPage/WebPage.h:2237 > + bool m_requestIsNonAppBound { false }; Again, I don't see how a request being app bound is related to a WebPage since you can make MANY requests in a WebPage.
Comment on attachment 420375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420375&action=review >> Source/WebCore/loader/DocumentLoader.h:424 >> + bool requestIsNonAppBound() const { return m_requestIsNonAppBound; } > > I am no familiar with the 'app-bound' feature at all but I am wondering why we have the negative here in the API. Why aren't we using "isAppBound" instead of "isNonAppBound". Seems to be (at least) that it would be more readable. Hmm, yes I left it in the negative to match the call in CFNetwork but I am thinking now it would be better to reverse it at those call sites only and then store it as isAppBound in WebKit. >> Source/WebKit/Shared/NavigatingToAppBoundDomain.h:30 >> enum class NavigatingToAppBoundDomain : bool { Yes, No }; > > Ugh. Please reverse the No and Yes here so that 0 means No and 1 means Yes. I am sorry for my past self. >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:314 >> +#if PLATFORM(IOS_FAMILY) > > Seems like this should cover those whole _isNonAppBoundRequest method implementation now that this method is iOS only? Right, will fix. >> Source/WebKit/UIProcess/WebPageProxy.h:2901 >> + bool m_requestIsNonAppBound { false }; > > It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page. I use it as a holding place after WebPageProxy::loadRequest is called until it gets sent to the WebPage and stored in the main frame's document loader. Maybe "currentRequestIsNonAppBound"? >> Source/WebKit/WebProcess/WebPage/WebPage.h:2237 >> + bool m_requestIsNonAppBound { false }; > > Again, I don't see how a request being app bound is related to a WebPage since you can make MANY requests in a WebPage. Again, maybe "currentRequestIsNonAppBound" instead?
Comment on attachment 420375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420375&action=review >>> Source/WebKit/UIProcess/WebPageProxy.h:2901 >>> + bool m_requestIsNonAppBound { false }; >> >> It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page. > > I use it as a holding place after WebPageProxy::loadRequest is called until it gets sent to the WebPage and stored in the main frame's document loader. Maybe "currentRequestIsNonAppBound"? Or, currentRequestIsAppBound if I reverse it.
(In reply to katherine_cheney from comment #15) > Comment on attachment 420375 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420375&action=review > > >>> Source/WebKit/UIProcess/WebPageProxy.h:2901 > >>> + bool m_requestIsNonAppBound { false }; > >> > >> It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page. > > > > I use it as a holding place after WebPageProxy::loadRequest is called until it gets sent to the WebPage and stored in the main frame's document loader. Maybe "currentRequestIsNonAppBound"? > > Or, currentRequestIsAppBound if I reverse it. Does it reset after we're done navigating? If not, it sounds like it should be lastRequestWasAppBound, no? or maybe even better: lastNavigationWasAppBound. I personally find "request" at page level to be confusing (although I understand it matches WKWebView.loadRequest API). Internally, the loadRequest results in a main frame navigation.
Created attachment 420501 [details] Patch
Created attachment 420505 [details] Patch
(In reply to Chris Dumez from comment #16) > (In reply to katherine_cheney from comment #15) > > Comment on attachment 420375 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=420375&action=review > > > > >>> Source/WebKit/UIProcess/WebPageProxy.h:2901 > > >>> + bool m_requestIsNonAppBound { false }; > > >> > > >> It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page. > > > > > > I use it as a holding place after WebPageProxy::loadRequest is called until it gets sent to the WebPage and stored in the main frame's document loader. Maybe "currentRequestIsNonAppBound"? > > > > Or, currentRequestIsAppBound if I reverse it. > > Does it reset after we're done navigating? If not, it sounds like it should > be lastRequestWasAppBound, no? or maybe even better: > lastNavigationWasAppBound. I personally find "request" at page level to be > confusing (although I understand it matches WKWebView.loadRequest API). > Internally, the loadRequest results in a main frame navigation. I agree this makes more sense.
Comment on attachment 420505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420505&action=review > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:112 > + request.setIsAppBound(frame.mainFrame().document()->loader()->lastNavigationWasAppBound()); These need null checks > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:769 > + request.setIsAppBound(webPage.mainFrame()->document()->loader()->lastNavigationWasAppBound()); ditto > Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:51 > +#else > +#define APP_BOUND_REQUEST_ADDITIONS This isn't needed.
(In reply to Alex Christensen from comment #20) > Comment on attachment 420505 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420505&action=review > > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:112 > > + request.setIsAppBound(frame.mainFrame().document()->loader()->lastNavigationWasAppBound()); > > These need null checks Will fix. > > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:769 > > + request.setIsAppBound(webPage.mainFrame()->document()->loader()->lastNavigationWasAppBound()); > > ditto Will fix. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:51 > > +#else > > +#define APP_BOUND_REQUEST_ADDITIONS > > This isn't needed. It will always use the Internal SDK?
Comment on attachment 420505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420505&action=review I think this looks great, but I don't think we want to limit this concept to iOS-only. r- to fix that, but otherwise this is ready to go. > Source/WebCore/loader/DocumentLoader.h:423 > +#if PLATFORM(IOS_FAMILY) I don't think this concept is limited to IOS_FAMILY only, is it? > Source/WebCore/loader/DocumentLoader.h:666 > +#if PLATFORM(IOS_FAMILY) Ditto. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1363 > + parameters.request.setIsAppBound(lastNavigationWasAppBound == LastNavigationWasAppBound::Yes ? true : false); nit: I think you just need "lastNavigationWasAppBound == LastNavigationWasAppBound::Yes" (the additional ternary operation is redundant) > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:880 > + redirectRequest.setIsAppBound(request.isAppBound()); I wonder if the test is really necessary before setting. Maybe just always set the app-bound state of the redirect to match the original request, since we always want the redirect to have the state of the originating request. > Source/WebKit/Shared/WebPageCreationParameters.cpp:174 > +#if PLATFORM(IOS_FAMILY) I don't think this is limited to IOS. > Source/WebKit/Shared/WebPageCreationParameters.h:245 > +#if PLATFORM(IOS_FAMILY) Ditto. > Source/WebKit/UIProcess/WebPageProxy.cpp:1322 > +#if PLATFORM(IOS_FAMILY) I don't think this should be iOS-only > Source/WebKit/UIProcess/WebPageProxy.cpp:-7880 > - Nit: Unneeded whitespace change. > Source/WebKit/UIProcess/WebPageProxy.h:1842 > +#if PLATFORM(IOS_FAMILY) I don't thin this should be iOS-only > Source/WebKit/WebProcess/WebPage/WebPage.cpp:517 > +#if PLATFORM(IOS_FAMILY) Not iOS-only > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1624 > +#if PLATFORM(IOS_FAMILY) Ditto > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6427 > +#if PLATFORM(IOS_FAMILY) Ditto > Source/WebKit/WebProcess/WebPage/WebPage.h:1411 > +#if PLATFORM(IOS_FAMILY) Ditto > Source/WebKit/WebProcess/WebPage/WebPage.h:2236 > +#if PLATFORM(IOS_FAMILY) Ditto > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:633 > +#if PLATFORM(IOS_FAMILY) Ditto >>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:51 >>> +#define APP_BOUND_REQUEST_ADDITIONS >> >> This isn't needed. > > It will always use the Internal SDK? Your test says APPLE_INTERNAL_SDK only, so it seems like yes? > Tools/TestWebKitAPI/cocoa/TestWKWebView.h:70 > +#if PLATFORM(IOS_FAMILY) Not iOS-only > Tools/TestWebKitAPI/cocoa/TestWKWebView.mm:242 > +#if PLATFORM(IOS_FAMILY) Ditto.
Created attachment 420794 [details] Patch
(In reply to Brent Fulgham from comment #22) > Comment on attachment 420505 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420505&action=review > > I think this looks great, but I don't think we want to limit this concept to > iOS-only. r- to fix that, but otherwise this is ready to go. > Thanks! Latest patch makes these changes.
Comment on attachment 420794 [details] Patch Excellent! r=me
Committed r273102: <https://commits.webkit.org/r273102> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420794 [details].