Bug 158232 – Reduce ResourceResponse copying

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Reduce ResourceResponse copying

[Alex Christensen]

Created attachment 280173 [details]
Patch

[Chris Dumez]

Comment on attachment 280173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280173&action=review

> Source/WTF/wtf/Optional.h:184
> +    template<class Encoder> void encode(Encoder&) const;

What's wrong with the version that is already in ArgumentCoders.h?

[Chris Dumez]

Comment on attachment 280173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280173&action=review

> Source/WebCore/platform/network/ResourceResponseBase.cpp:184
>      m_certificateInfo = static_cast<const ResourceResponse*>(this)->platformCertificateInfo();

Presumably we no longer need the const in the cast here.

> Source/WebCore/platform/network/ResourceResponseBase.h:176
> +#define RESOURCE_RESPONSE_BASE_CERT_INFO_OPTIONAL

What's this?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:471
> +    webPage->send(Messages::WebPageProxy::DidCommitLoadForFrame(m_frame->frameID(), documentLoader.navigationID(), documentLoader.response().mimeType(), m_frameHasCustomContentProvider, static_cast<uint32_t>(m_frame->coreFrame()->loader().loadType()), documentLoader.response().certificateInfo().valueOr(CertificateInfo()), m_frame->coreFrame()->document()->isPluginDocument(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));

Is constructing a CertificateInfo() cheap enough that we want to do it every time?

> Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409
> +    return documentLoader->response().certificateInfo().valueOr(CertificateInfo());

Is constructing a CertificateInfo() cheap enough that we want to do it every time?

[Chris Dumez]

Comment on attachment 280173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280173&action=review

> Source/WebKit2/UIProcess/Authentication/AuthenticationChallengeProxy.cpp:74
> +    if (credential->certificateInfo() && credential->certificateInfo())

LoL :)

[Alex Christensen]

Created attachment 280181 [details]
Patch

[WebKit Commit Bot]

Attachment 280181 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 280181 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280181&action=review

> Source/WebCore/platform/network/ResourceResponseBase.h:176
> +#define RESOURCE_RESPONSE_BASE_CERT_INFO_OPTIONAL

This is temporary and to assist a change in another repository.

[Alex Christensen]

Comment on attachment 280181 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280181&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.h:30
> +#include "ArgumentCoders.h"

This isn't needed

[Alex Christensen]

Created attachment 280670 [details]
Patch

[WebKit Commit Bot]

Attachment 280670 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 280670 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280670&action=review

> Source/WebCore/ChangeLog:15
> +        (WebCore::ResourceResponseBase::includeCertificateInfo): Made non-const because it modifies the ResourceResponse.

I am not sure what your rationale was for changing this. The person who did the original work intentionally used const and mutable. Could you give the rationale?

> Source/WebCore/loader/SubresourceLoader.cpp:244
>      if (shouldIncludeCertificateInfo())
> -        response.includeCertificateInfo();
> +        const_cast<ResourceResponse&>(response).includeCertificateInfo(); // FIXME: response should not be const.

Why not fix this instead of adding the const_cast?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:194
> +Optional<CertificateInfo> ResourceResponseBase::certificateInfo() const
>  {
>      return m_certificateInfo;
>  }

I suggest we make this an inline in the header.

> Source/WebCore/platform/network/ResourceResponseBase.h:117
> +    WEBCORE_EXPORT Optional<CertificateInfo> certificateInfo() const;

For better efficiency, I suggest the return type const Optional<CertificateInfo>&. Otherwise, calling this will copy the certificate info. And so it will be much less efficient than calling containsCertificateInfo() was.

> Source/WebCore/platform/network/ResourceResponseBase.h:192
> +#define RESOURCE_RESPONSE_BASE_CERT_INFO_OPTIONAL

What is this for?

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:293
> +    // FIXME: receivedResponse should not be const.
> +    if (sharedDidReceiveResponse(const_cast<ResourceResponse&>(receivedResponse)) == NetworkLoadClient::ShouldContinueDidReceiveResponse::Yes)
>          m_handle->continueDidReceiveResponse();

Why a FIXME instead of fixing it?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.h:84
> +template<typename T> struct Coder<WTF::Optional<T>> {
> +    static void encode(Encoder& encoder, const WTF::Optional<T>& optional)

Can’t this just be Optional, rather than WTF::Optional?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.h:95
> +    static bool decode(Decoder& decoder, WTF::Optional<T>& optional)

Can’t this just be Optional, rather than WTF::Optional?

[Alex Christensen]

Comment on attachment 280670 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280670&action=review

>> Source/WebCore/ChangeLog:15
>> +        (WebCore::ResourceResponseBase::includeCertificateInfo): Made non-const because it modifies the ResourceResponse.
> 
> I am not sure what your rationale was for changing this. The person who did the original work intentionally used const and mutable. Could you give the rationale?

You're right.  This is not a good change.  It just copies things from the platform data to the ResourceResponseBase, which doesn't actually change the ResourceResponse conceptually.

>> Source/WebCore/platform/network/ResourceResponseBase.h:192
>> +#define RESOURCE_RESPONSE_BASE_CERT_INFO_OPTIONAL
> 
> What is this for?

This is so we don't break the internal build when I land this, because certificateInfo() is called from WebKitAdditions.  It's temporary and will be removed.

[Alex Christensen]

Created attachment 281031 [details]
Patch

[WebKit Commit Bot]

Attachment 281031 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 281036 [details]
Patch

[WebKit Commit Bot]

Attachment 281036 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 281038 [details]
Patch

[WebKit Commit Bot]

Attachment 281038 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 281041 [details]
Patch

[WebKit Commit Bot]

Attachment 281041 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 281046 [details]
Patch

[WebKit Commit Bot]

Attachment 281046 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:409:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

http://trac.webkit.org/changeset/201943

[Philippe Normand]

Comment on attachment 281046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281046&action=review

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1095
>      handleResponseReceived(response);

Shouldn't this use WTFMove(response) ?

[Chris Dumez]

Comment on attachment 281046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281046&action=review

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1095
>>      handleResponseReceived(response);
> 
> Shouldn't this use WTFMove(response) ?

It could but it would not help unless handleResponseReceived() is also updated to take a ResourceResponse&&.