After smashing it together with the clients it can be made nicer.
Created attachment 237890 [details] patch
Comment on attachment 237890 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=237890&action=review > Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:78 > + const WebCore::ResourceRequest& originalRequest() const { return m_parameters.request; } > + // Changes with redirects. > + WebCore::ResourceRequest& currentRequest() { return m_currentRequest; } The paragraphing here made it hard for me to understand the comment. I would put a blank line before the comment. > Source/WebKit2/Shared/Network/NetworkResourceLoadParameters.cpp:133 > + for (size_t i = 0, count = requestBodySandboxExtensionHandles.size(); i < count; ++i) { Modern for loop would be better. > Source/WebKit2/Shared/Network/NetworkResourceLoadParameters.cpp:135 > + result.requestBodySandboxExtensions.append(extension); extension.release()
Comment on attachment 237890 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=237890&action=review >> Source/WebKit2/Shared/Network/NetworkResourceLoadParameters.cpp:133 >> + for (size_t i = 0, count = requestBodySandboxExtensionHandles.size(); i < count; ++i) { > > Modern for loop would be better. Unfortunately HandleArray is a custom array type without iterators.
https://trac.webkit.org/r173468
Comment on attachment 237890 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=237890&action=review > Source/WebKit2/ChangeLog:21 > + Instead of copying everything out from NetworkResourceLoadParameters just include the whole thing as a const field. I guess it's OK to try how this feels, but this is not how other "parameters" classes work, and it's really a layering violation between IPC and loading logic to do this. > Source/WebKit2/ChangeLog:64 > + Use original request instead of the current one. This might fix a bug where we didn't use file backing over redirects. I'd love to hear Brady's take on this. To me, this sounds incorrect - caching is always about the final URL, because redirect chains can and do change. > Source/WebKit2/ChangeLog:69 > + Decode to SanboxExtensions to the actual type rather than a handle. Good stuff - the complications with sandbox extensions were caused by lack of C++11 move support at the time the code was written, and the class itself wasn't RefCounted if I remember correctly.
> I guess it's OK to try how this feels, but this is not how other "parameters" classes work, and it's really a layering violation between IPC and loading logic to do this. Alexey, I know what our layering is. You don't need to worry about me violating it.
Can you be more specific please? What makes you think that using a type that exists of IPC purposes in NetworkResourceLoader is not a layering violation?
(In reply to comment #7) > Can you be more specific please? What makes you think that using a type that exists of IPC purposes in NetworkResourceLoader is not a layering violation? Yes. Whether someone thinks something exists for "IPC purposes" is not a meaningful part of our layering.
Thank you for your accurate, detailed and immensely helpful response.
Comment on attachment 237890 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=237890&action=review > Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:89 > + bool defersLoading() const { return m_parameters.defersLoading; } Does this always match m_defersLoading? Would setDefersLoading(false) affect m_parameters.defersLoading?