We should switch between App-Bound and In-App-Browser Network Sessions
<rdar://problem/59434006>
Created attachment 391338 [details] Patch
Created attachment 391339 [details] Patch
Comment on attachment 391339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391339&action=review This looks very good! I have a few minor things I'd like addressed. > Source/WebKit/NetworkProcess/NetworkDataTask.cpp:53 > + return NetworkDataTaskCocoa::create(session, client, parameters.request, parameters.webFrameID, parameters.webPageID, parameters.storedCredentialsPolicy, parameters.contentSniffingPolicy, parameters.contentEncodingSniffingPolicy, parameters.shouldClearReferrerOnHTTPSToHTTPRedirect, parameters.shouldPreconnectOnly, parameters.isMainFrameNavigation, parameters.isMainResourceNavigationForAnyFrame, parameters.networkActivityTracker, parameters.isNavigatingToAppBoundDomain); At some point, it seems like we should just past the parameters object to the 'create' method! :-) > Source/WebKit/NetworkProcess/NetworkLoadParameters.h:64 > + bool isNavigatingToAppBoundDomain { true }; It seems like the default state should be false here (most traffic is likely not app-bound). > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:124 > + Extra whitespace. > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:77 > + bool isNavigatingToAppBoundDomain; Whitespace, and should have a default argument. > Source/WebKit/Shared/LoadParameters.h:72 > + bool isNavigatingToAppBoundDomain; Please provide a default initializer (false). > Source/WebKit/UIProcess/WebPageProxy.cpp:3064 > + void send(PolicyDecision&& policyDecision) Oops! :-) > Source/WebKit/UIProcess/WebPageProxy.cpp:3083 > +bool WebPageProxy::isAppBoundDomain(WebCore::RegistrableDomain domain) Could this be "const WebCore::RegistrableDomain&" ? > Source/WebKit/UIProcess/WebPageProxy.h:2259 > + bool isAppBoundDomain(WebCore::RegistrableDomain); Could this be const? Also seems like the argument (which is effectively a String) would benefit from being passed by reference. > Source/WebKit/UIProcess/WebPageProxy.h:2261 > + bool isNavigatingToAppBoundDomain() { return m_isNavigatingToAppBoundDomain; } This should be "bool isNavigatingToAppBoundDomain() const" > Source/WebKit/WebProcess/WebPage/WebPage.h:1290 > + bool isNavigatingToAppBoundDomain() { return m_isNavigatingToAppBoundDomain; } Should be const. > Source/WebKit/WebProcess/WebPage/WebPage.h:2073 > + bool m_isNavigatingToAppBoundDomain { true }; I think this should default to 'false'.
(In reply to Brent Fulgham from comment #4) > Comment on attachment 391339 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391339&action=review > > This looks very good! I have a few minor things I'd like addressed. > Thanks for the comments! I'll make these changes. > > Source/WebKit/NetworkProcess/NetworkDataTask.cpp:53 > > + return NetworkDataTaskCocoa::create(session, client, parameters.request, parameters.webFrameID, parameters.webPageID, parameters.storedCredentialsPolicy, parameters.contentSniffingPolicy, parameters.contentEncodingSniffingPolicy, parameters.shouldClearReferrerOnHTTPSToHTTPRedirect, parameters.shouldPreconnectOnly, parameters.isMainFrameNavigation, parameters.isMainResourceNavigationForAnyFrame, parameters.networkActivityTracker, parameters.isNavigatingToAppBoundDomain); > > At some point, it seems like we should just past the parameters object to > the 'create' method! :-) > Agreed! > > Source/WebKit/NetworkProcess/NetworkLoadParameters.h:64 > > + bool isNavigatingToAppBoundDomain { true }; > > It seems like the default state should be false here (most traffic is likely > not app-bound). > > > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:124 > > + > > Extra whitespace. > > > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:77 > > + bool isNavigatingToAppBoundDomain; > > Whitespace, and should have a default argument. > > > Source/WebKit/Shared/LoadParameters.h:72 > > + bool isNavigatingToAppBoundDomain; > > Please provide a default initializer (false). > > > Source/WebKit/UIProcess/WebPageProxy.cpp:3064 > > + void send(PolicyDecision&& policyDecision) > > Oops! :-) > Yep, my bad. This was my own mistake from a previous patch... > > Source/WebKit/WebProcess/WebPage/WebPage.h:2073 > > + bool m_isNavigatingToAppBoundDomain { true }; > > I think this should default to 'false'. I think there's a few other places where I default to true, I'll change those too!
Created attachment 391355 [details] Patch
Comment on attachment 391355 [details] Patch Looks great!
Comment on attachment 391355 [details] Patch r=me once you confirm that EWS is happy. Please do double-check with Chris or Youenn if they are available tomorrow.
(In reply to Brent Fulgham from comment #8) > Comment on attachment 391355 [details] > Patch > > r=me once you confirm that EWS is happy. Please do double-check with Chris > or Youenn if they are available tomorrow. Thanks! Sounds good, I also asked John to take a look.
View in context: https://bugs.webkit.org/attachment.cgi?id=391355&action=review First batch of comments. > Source/WebKit/ChangeLog:13 > + domains. This requires updating the webPage when a navigation occurs Weird camel casing of webPage. Is that a reference to a variable? > Source/WebKit/ChangeLog:103 > + the new webPage via the ProvisionalPageProxy. Same thing with the camel casing. > Source/WebKit/ChangeLog:112 > + These functions determine if the category is appBound and update the Should be app-bound. > Source/WebKit/NetworkProcess/NetworkProcess.h:346 > + void setInAppBrowserPrivacyEnabled(PAL::SessionID, bool, CompletionHandler<void()>&&); I would give the bool a name even though it is pretty obvious what it's for. > Source/WebKit/NetworkProcess/NetworkSession.h:100 > + Was there an edit here? > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:98 > + SessionWrapper& sessionWrapperForTask(const WebCore::ResourceRequest&, WebCore::StoredCredentialsPolicy, bool); The bool needs to have a name here. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:100 > + bool isInAppBrowserPrivacyEnabled() { return m_isInAppBrowserPrivacyEnabled; } Const. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1201 > + if (m_isInAppBrowserPrivacyEnabled) { This should be if (m_isInAppBrowserPrivacyEnabled && isNavigatingToAppBoundDomain) and then no curly braces. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1296 > + if (m_appBoundSession) This should be return !!m_appBoundSession. Could be moved to the header if you so wish. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:489 > + auto category = webPageProxy->configuration().webViewCategory(); Do we have to use a variable here or could the call be made like so: completionHandler(toWKWebViewCategory(webPageProxy->configuration().webViewCategory()) ? > Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:348 > + reply(PolicyDecision { identifier, true, WebCore::PolicyAction::Ignore, navigationID, DownloadID(), WTF::nullopt }); I think we need to add a boolean enum NavigatingToAppBoundDomain::{Yes, No} so that PolicyDecision.h can take one of those. Instantiating a PolicyDecision with just "true" makes it unclear what your code does. With such an enum you can make your getters isNavigatingToAppBoundDomain() return it instead of a boolean too and you don't have to name all those bool parameters in header files. Load parameters can use it too. > Source/WebKit/UIProcess/ProvisionalPageProxy.h:95 > + void loadRequest(API::Navigation&, WebCore::ResourceRequest&&, API::Object* userData, bool, Optional<WebsitePoliciesData>&& = WTF::nullopt); This bool needs a name.
Thanks John! Seems like Mac-wk2 tests also need some investigating, looking into those.
Comment on attachment 391355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391355&action=review Second batch of comments. Looks good overall! > Source/WebKit/ChangeLog:13 > + domains. This requires updating the webPage when a navigation occurs Weird camel casing of webPage. Is that a reference to a variable? > Source/WebKit/UIProcess/WebPageProxy.cpp:3083 > +bool WebPageProxy::isAppBoundDomain(WebCore::RegistrableDomain domain) RegistrableDomain& and const both for the parameter and the function? > Source/WebKit/UIProcess/WebPageProxy.cpp:3091 > + if (m_preferences->isInAppBrowserPrivacyEnabled()) { This should be if (m_preferences->isInAppBrowserPrivacyEnabled() && isMainFrame) so that you can drop one nested context. > Source/WebKit/UIProcess/WebPageProxy.cpp:3093 > + RegistrableDomain currentDomain { requestURL }; This variable is never used. > Source/WebKit/UIProcess/WebPageProxy.cpp:3094 > + if (!isAppBoundDomain(RegistrableDomain(requestURL))) { You could have a convenience function isAppBoundDomain(URL&) that does the conversion for you and calls isAppBoundDomain(isAppBoundDomain&). > Source/WebKit/UIProcess/WebPageProxy.h:2259 > + bool isAppBoundDomain(WebCore::RegistrableDomain); This should probably be isAppBoundDomain(const WebCore::RegistrableDomain&) const; > Source/WebKit/UIProcess/WebPageProxy.h:2260 > + void setIsNavigatingToAppBoundDomain(bool isMainFrame, const URL& requestURL); requestURL is unnecessary. > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:330 > + auto* webFrameLoaderClient = toWebFrameLoaderClient(resourceLoader.frame()->loader().client()); Can resourceLoader.frame() return null? > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:339 > // FIXME: Instead of passing userContentControllerIdentifier, the NetworkProcess should be able to get it using webPageId. Does this FIXME need to be moved along with the assignment statements? I don't think so. > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:687 > // FIXME: Instead of passing userContentControllerIdentifier, we should just pass webPageId to NetworkProcess. Ditto. > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:839 > + m_frame->didReceivePolicyDecision(listenerID, PolicyDecision { identifier, true, PolicyAction::Ignore, 0, { }, { } }); Again, we should not call with raw booleans like this. > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:996 > + m_frame->didReceivePolicyDecision(listenerID, PolicyDecision { requestIdentifier, false, PolicyAction::Ignore, 0, { }, { } }); Ditto. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3265 > +void WebPage::didReceivePolicyDecision(FrameIdentifier frameID, uint64_t listenerID, PolicyDecision&& policyDecision) Good catch. > Tools/ChangeLog:11 > + manually for the test. One ensures that setting the webView category Weird camel casing of webView. > Tools/WebKitTestRunner/TestController.h:263 > + void getWebViewCategory(); Const? > Tools/WebKitTestRunner/TestController.h:303 > + bool hasAppBoundSession(); Const? > LayoutTests/platform/mac-wk2/TestExpectations:61 > +http/tests/in-app-browser-privacy/ [ Pass ] You should probably have the same comment here.
(In reply to John Wilander from comment #12) > You could have a convenience function isAppBoundDomain(URL&) that does the > conversion for you and calls isAppBoundDomain(isAppBoundDomain&). I mean to say "… calls isAppBoundDomain(RegistrableDomain&)."
Created attachment 391418 [details] Patch
Created attachment 391421 [details] Patch
Created attachment 391426 [details] Patch
Created attachment 391429 [details] Patch
(In reply to katherine_cheney from comment #17) > Created attachment 391429 [details] > Patch trying to get this new mac-wk2 test to pass (runs fine locally, not sure what's going on here)
Created attachment 391430 [details] Patch
Comment on attachment 391430 [details] Patch Clearing flags on attachment: 391430 Committed r257185: <https://trac.webkit.org/changeset/257185>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59691947>