WKWebView._negotiatedLegacyTLS should be correct after back/forward navigations
Created attachment 393396 [details] Patch
Created attachment 393409 [details] Patch
Comment on attachment 393409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393409&action=review > Source/WebCore/history/CachedFrame.cpp:322 > + if (cachedFrame && cachedFrame->usedLegacyTLS() == UsedLegacyTLS::Yes) s/cachedFrame &&// as probably we cannot have null child frames. We could use a Vector<UniqueRef> to make it clearer. > Source/WebKit/NetworkProcess/NetworkDataTask.cpp:114 > + response.setSource(ResourceResponse::Source::Network); Why do we need this change? We are still setting the source for redirections in NetworkLoad. It seems better to keep everything in one place. > Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:136 > + if (response.usedLegacyTLS()) { You probably also want to check the source, like if (response.usedLegacyTLS() && response.source() == Network). If it is from the network cache or service worker, we probably do not want to take that into account, as I understand the current implementation. If we wanted to take into account appcache/memorycache, the check should be done inside DocumentLoader. Service worker case is interesting since if a document is using it, it will never have usedLegacyTLS currently. Maybe we should fix this.
Comment on attachment 393409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393409&action=review >> Source/WebCore/history/CachedFrame.cpp:322 >> + if (cachedFrame && cachedFrame->usedLegacyTLS() == UsedLegacyTLS::Yes) > > s/cachedFrame &&// as probably we cannot have null child frames. > We could use a Vector<UniqueRef> to make it clearer. Good idea. Done. >> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:114 >> + response.setSource(ResourceResponse::Source::Network); > > Why do we need this change? > We are still setting the source for redirections in NetworkLoad. > It seems better to keep everything in one place. NetworkLoad is an unneeded abstraction that was originally representing either an NSURLSession load or an NSURLConnection load. Since we don't use NSURLConnection in modern WebKit anymore, we should move everything from it and remove it. As long as I'm changing the didReceiveResponse code and putting things here, This may as well go as well. >> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:136 >> + if (response.usedLegacyTLS()) { > > You probably also want to check the source, like if (response.usedLegacyTLS() && response.source() == Network). > If it is from the network cache or service worker, we probably do not want to take that into account, as I understand the current implementation. > > If we wanted to take into account appcache/memorycache, the check should be done inside DocumentLoader. > Service worker case is interesting since if a document is using it, it will never have usedLegacyTLS currently. > Maybe we should fix this. Good idea, but moving it to DocumentLoader doesn't fix this. Moving it to ResourceLoader does. That looks like a better place for it.
Created attachment 393506 [details] Patch
Comment on attachment 393506 [details] Patch lgtm As of setSource, it does not look great to have it in two places. I would leave that for a future refactoring that would add a platform independent redirection handler in NetworkDataTask so that setSource is no longer called in NetworkLoad. Also, I am not sure NetworkTask is the right place. Source is only useful for WebProcess so this seems more a NetworkResourceLoader thing
Comment on attachment 393506 [details] Patch Clearing flags on attachment: 393506 Committed r258458: <https://trac.webkit.org/changeset/258458>
All reviewed patches have been landed. Closing bug.
<rdar://problem/60443735>