Bug 136701 – NetworkResourceLoader cleanups

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

After smashing it together with the clients it can be made nicer.

[Antti Koivisto]

Created attachment 237890 [details]
patch

[Darin Adler]

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()

[Antti Koivisto]

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.

[Antti Koivisto]

https://trac.webkit.org/r173468

[Alexey Proskuryakov]

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.

[Antti Koivisto]

> 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.

[Alexey Proskuryakov]

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?

[Antti Koivisto]

(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.

[Alexey Proskuryakov]

Thank you for your accurate, detailed and immensely helpful response.

[Joseph Pecoraro]

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?