Summary: | Reduce ResourceResponse copying | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | benjamin, berto, cdumez, cgarcia, cmarcelo, commit-queue, danw, gustavo, japhet, mcatanzaro, mrobinson, pnormand | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2016-05-31 13:39:53 PDT
Created attachment 280173 [details]
Patch
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? 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? 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 :) Created attachment 280181 [details]
Patch
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.
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. 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 Created attachment 280670 [details]
Patch
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.
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? 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. Created attachment 281031 [details]
Patch
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.
Created attachment 281036 [details]
Patch
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.
Created attachment 281038 [details]
Patch
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.
Created attachment 281041 [details]
Patch
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.
Created attachment 281046 [details]
Patch
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.
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) ? 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&&. |