Move applyUserAgentIfNeeded calls to a more central place.
Created attachment 394585 [details] Patch
Created attachment 394589 [details] Patch
Created attachment 394594 [details] Patch
Committed r259116: <https://trac.webkit.org/changeset/259116> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394594 [details].
<rdar://problem/60970124>
Reverted r259116 for reason: Broke login flow on some apple-internal sites (rdar://problem/61905262) Committed r260511: <https://trac.webkit.org/changeset/260511>
We know that this revision caused the login failures. We don’t know what behavior changed. Need to figure that out, make tests, to make sure future refactoring doesn’t break this again.
(In reply to Darin Adler from comment #7) > We know that this revision caused the login failures. We don’t know what > behavior changed. Need to figure that out, make tests, to make sure future > refactoring doesn’t break this again. Understood. I will follow-up with more investigation and hopefully a test. For now, my priority is to get us back into a working state.
(In reply to Chris Dumez from comment #8) > (In reply to Darin Adler from comment #7) > > We know that this revision caused the login failures. We don’t know what > > behavior changed. Need to figure that out, make tests, to make sure future > > refactoring doesn’t break this again. > > Understood. I will follow-up with more investigation and hopefully a test. > For now, my priority is to get us back into a working state. I did some digging with Web Inspector and it looks like if you do a navigation like so: "window.location = 'test.html';" then WebCore no longer sets the User-Agent header on the main resource request to test.html
Comment on attachment 394594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394594&action=review > Source/WebCore/loader/FrameLoader.cpp:-2979 > - applyUserAgentIfNeeded(request); For the scenario I mentioned, this used to add the user-agent header.
(In reply to Chris Dumez from comment #10) > Comment on attachment 394594 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394594&action=review > > > Source/WebCore/loader/FrameLoader.cpp:-2979 > > - applyUserAgentIfNeeded(request); > > For the scenario I mentioned, this used to add the user-agent header. I was wrong, the User-Agent header seems to be missing from sync XHR requests. I have a test almost ready.
(In reply to Chris Dumez from comment #11) > (In reply to Chris Dumez from comment #10) > > Comment on attachment 394594 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=394594&action=review > > > > > Source/WebCore/loader/FrameLoader.cpp:-2979 > > > - applyUserAgentIfNeeded(request); > > > > For the scenario I mentioned, this used to add the user-agent header. > > I was wrong, the User-Agent header seems to be missing from sync XHR > requests. I have a test almost ready. Thanks for your work here! My patch probably did not add an applyUserAgentIfNeeded to FrameLoader::loadResourceSynchronously, to correct the applyUserAgentIfNeeded that was removed from addExtraFieldsToRequest.
(In reply to Darin Adler from comment #7) > We know that this revision caused the login failures. We don’t know what > behavior changed. Need to figure that out, make tests, to make sure future > refactoring doesn’t break this again. Added test that would have failed via https://bugs.webkit.org/show_bug.cgi?id=210863.
Created attachment 397229 [details] Patch
Comment on attachment 397229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397229&action=review > Source/WebCore/loader/cache/CachedResourceLoader.h:178 > - void updateHTTPRequestHeaders(CachedResource::Type, CachedResourceRequest&); > + void updateHTTPRequestHeaders(FrameLoader&, CachedResource::Type, CachedResourceRequest&); This doesn’t seem good.
Comment on attachment 397229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397229&action=review >> Source/WebCore/loader/cache/CachedResourceLoader.h:178 >> + void updateHTTPRequestHeaders(FrameLoader&, CachedResource::Type, CachedResourceRequest&); > > This doesn’t seem good. Can you expand on what is not good? This is the original patch plus the fix to loadResourceSynchronously.
Comment on attachment 397229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397229&action=review >>> Source/WebCore/loader/cache/CachedResourceLoader.h:178 >>> + void updateHTTPRequestHeaders(FrameLoader&, CachedResource::Type, CachedResourceRequest&); >> >> This doesn’t seem good. > > Can you expand on what is not good? This is the original patch plus the fix to loadResourceSynchronously. Having to pass a FrameLoader here bothers me. But it’s not really about this one patch.
Comment on attachment 397229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397229&action=review >>>> Source/WebCore/loader/cache/CachedResourceLoader.h:178 >>>> + void updateHTTPRequestHeaders(FrameLoader&, CachedResource::Type, CachedResourceRequest&); >>> >>> This doesn’t seem good. >> >> Can you expand on what is not good? This is the original patch plus the fix to loadResourceSynchronously. > > Having to pass a FrameLoader here bothers me. But it’s not really about this one patch. I see, one impovement may be to inline this more in CachedResourceLoader::requestResource, and make sure to order well and add the right comments and spec links. There is probably still quite some we can do to improve that method, I made a start here https://bugs.webkit.org/show_bug.cgi?id=210925.
Committed r260596: <https://trac.webkit.org/changeset/260596> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397229 [details].
This broke our API. See https://bugs.webkit.org/show_bug.cgi?id=213626