Bug 160292

Created attachment 284779 [details] Patch

Comment on attachment 284779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284779&action=review > Source/WebCore/loader/DocumentThreadableLoader.cpp:99 > + m_options.setAllowCredentials((m_options.credentials == FetchOptions::Credentials::Include || (m_options.credentials == FetchOptions::Credentials::SameOrigin && m_sameOriginRequest)) ? AllowStoredCredentials : DoNotAllowStoredCredentials); Why don't we do this where the ThreadableLoaderOptions is created? It seems strange to create a ThreadableLoaderOptions, give it to the DocumentThreadableLoader, then compute the allowCredentials after we have copied it. > Source/WebCore/loader/DocumentThreadableLoader.cpp:376 > + newRequest.mutableResourceRequest().setAllowCookies(m_options.allowCredentials() == AllowStoredCredentials); FYI, in the NSURLSession loader, if a request doesn't allow credentials, it uses a stateless NSURLSession that has no cookie storage or credential storage. > Source/WebCore/loader/DocumentThreadableLoader.cpp:386 > + // If credentials mode is omit, we should disable cookie sending. omitted > Source/WebCore/workers/WorkerScriptLoader.cpp:90 > + // We should add an option to set credential mode. FIXME?

Thanks for the review. (In reply to comment #2) > Comment on attachment 284779 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284779&action=review > > > Source/WebCore/loader/DocumentThreadableLoader.cpp:99 > > + m_options.setAllowCredentials((m_options.credentials == FetchOptions::Credentials::Include || (m_options.credentials == FetchOptions::Credentials::SameOrigin && m_sameOriginRequest)) ? AllowStoredCredentials : DoNotAllowStoredCredentials); > > Why don't we do this where the ThreadableLoaderOptions is created? It seems > strange to create a ThreadableLoaderOptions, give it to the > DocumentThreadableLoader, then compute the allowCredentials after we have > copied it. I think allowCredentials should be removed from ResourceLoaderOptions. Loader clients should just think in terms of fetch parameters. Ideally, allowCredentials should be replaced by a credentialFlag which would be set or not according fetch options. Typically at ResourceLoader level. The only potential issue is the case of DocumentThreadableLoader which stops ResourceLoader from redirecting in case of cross origin redirections. Maybe that can be handled through fetch options. > > Source/WebCore/loader/DocumentThreadableLoader.cpp:376 > > + newRequest.mutableResourceRequest().setAllowCookies(m_options.allowCredentials() == AllowStoredCredentials); > > FYI, in the NSURLSession loader, if a request doesn't allow credentials, it > uses a stateless NSURLSession that has no cookie storage or credential > storage. That is a nice and consistent way of handling things. Maybe this should be widespread to all ports. Note though that CachedResourceLoader is checking whether allowCookies is set or not to evaluate whether resources can be reused or not. That is one item, amongst others, that would call for a good rationalization of CachedResourceLoader. > > > Source/WebCore/loader/DocumentThreadableLoader.cpp:386 > > + // If credentials mode is omit, we should disable cookie sending. > > omitted I'll rewrite as: If credentials mode is 'Omit', we should... > > Source/WebCore/workers/WorkerScriptLoader.cpp:90 > > + // We should add an option to set credential mode. > > FIXME? Yes, I'll update accordingly.

Created attachment 284862 [details] Patch for landing

Comment on attachment 284862 [details] Patch for landing Clearing flags on attachment: 284862 Committed r203900: <http://trac.webkit.org/changeset/203900>

All reviewed patches have been landed. Closing bug.