Remove WebURLResponse::initialize() [revised]

third_party/WebKit/Source/platform/exported/WebURLResponse.cpp

Index: third_party/WebKit/Source/platform/exported/WebURLResponse.cpp
diff --git a/third_party/WebKit/Source/platform/exported/WebURLResponse.cpp b/third_party/WebKit/Source/platform/exported/WebURLResponse.cpp
index 7a5173ec1b722ca2137f6a9fa2fb72c8d272dcb1..278abd36bace71521cadff61a1060bfad977d9b3 100644
--- a/third_party/WebKit/Source/platform/exported/WebURLResponse.cpp
+++ b/third_party/WebKit/Source/platform/exported/WebURLResponse.cpp
@@ -39,6 +39,7 @@
 #include "public/platform/WebURL.h"
 #include "public/platform/WebURLLoadTiming.h"
 #include "wtf/Allocator.h"
+#include "wtf/Assertions.h"
 #include "wtf/PtrUtil.h"
 #include "wtf/RefPtr.h"
 #include <memory>
@@ -71,180 +72,199 @@ private:
 class WebURLResponsePrivateImpl final : public WebURLResponsePrivate {
     USING_FAST_MALLOC(WebURLResponsePrivateImpl);
 public:
-    WebURLResponsePrivateImpl()
+    WebURLResponsePrivateImpl() {}
+    WebURLResponsePrivateImpl(const WebURLResponsePrivate& p)
+        : m_resourceResponseAllocation(*p.resourceResponse()) {}
+    ~WebURLResponsePrivateImpl() override {}
+
+    const ResourceResponse* resourceResponse() const override
     {
-        m_resourceResponse = &m_resourceResponseAllocation;
+        return &m_resourceResponseAllocation;
     }
 
-    WebURLResponsePrivateImpl(const WebURLResponsePrivate* p)
-        : m_resourceResponseAllocation(*p->m_resourceResponse)
+    ResourceResponse* resourceResponse() override
     {
-        m_resourceResponse = &m_resourceResponseAllocation;
+        return &m_resourceResponseAllocation;
     }
 
-    virtual void dispose() { delete this; }
+    void setResourceResponse(const ResourceResponse& response) override
+    {
+        m_resourceResponseAllocation = response;
+    }
 
 private:
-    virtual ~WebURLResponsePrivateImpl() { }
-
     ResourceResponse m_resourceResponseAllocation;
 };
 
-void WebURLResponse::initialize()
+WebURLResponse::~WebURLResponse()
 {
-    assign(new WebURLResponsePrivateImpl());
+    if (m_owningPrivate)
+        delete m_private;
 }
 
-void WebURLResponse::reset()
+WebURLResponse::WebURLResponse()
+    : m_private(new WebURLResponsePrivateImpl())
+    , m_owningPrivate(true)
 {
-    assign(0);
+}
+WebURLResponse::WebURLResponse(const WebURLResponse& r)
+    : m_private(new WebURLResponsePrivateImpl(*r.m_private))
+    , m_owningPrivate(true)
+{
+}
+
+WebURLResponse::WebURLResponse(const WebURL& url)
+    : WebURLResponse()
+{
+    setURL(url);
 }
 
-void WebURLResponse::assign(const WebURLResponse& r)
+WebURLResponse& WebURLResponse::operator=(const WebURLResponse& r)
 {
     if (&r != this)
-        assign(r.m_private ? new WebURLResponsePrivateImpl(r.m_private) : 0);
+        m_private->setResourceResponse(*r.m_private->resourceResponse());

[Adam Rice, 2016/07/06 01:39:00]

This changes behaviour if m_private is a WrappedResourceResponse::Handle. The
WebURLResponse will refer to the same underlying ResourceResponse, whereas
previously assignment always created a new ResourceResponse.

The tests seem to indicate that nothing actually cares about the change in
semantics, but I am not completely confident it is okay.

[kinuko, 2016/07/06 09:45:57]

This assignment operator doesn't work / wouldn't be called for
WrappedResourceResponse as it's non-copyable, and if the other WebURLResponse
(r) was WrappedResourceResponse we just copy the internal ResourceResponse by
value, am I right?

[Adam Rice, 2016/07/06 10:06:50]

I think WrappedResourceResponse is still copyable. It doesn't have
WTF_MAKE_NONCOPYABLE at the moment. Also, it could still be assigned if passed
by non-const reference to the base class.

I agree that there are no issues when (r) is WrappedResourceResponse. 

[kinuko, 2016/07/06 11:06:33]

No it is not because Handle is not copyable.  It's not very obvious, and that's
why I commented on your patch to explicitly mark it with WTF_MAKE_NONCOPYABLE
(which I haven't done that in my revised version yet-- I'll do so in next
upload).

https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...

[Adam Rice, 2016/07/07 01:32:47]

Sorry, I missed that point.

+    return *this;
 }
 
 bool WebURLResponse::isNull() const
 {
-    return !m_private || m_private->m_resourceResponse->isNull();
+    return !m_private->resourceResponse() || m_private->resourceResponse()->isNull();
 }
 
 WebURL WebURLResponse::url() const
 {
-    return m_private->m_resourceResponse->url();
+    return m_private->resourceResponse()->url();
 }
 
 void WebURLResponse::setURL(const WebURL& url)
 {
-    m_private->m_resourceResponse->setURL(url);
+    m_private->resourceResponse()->setURL(url);
 }
 
 unsigned WebURLResponse::connectionID() const
 {
-    return m_private->m_resourceResponse->connectionID();
+    return m_private->resourceResponse()->connectionID();
 }
 
 void WebURLResponse::setConnectionID(unsigned connectionID)
 {
-    m_private->m_resourceResponse->setConnectionID(connectionID);
+    m_private->resourceResponse()->setConnectionID(connectionID);
 }
 
 bool WebURLResponse::connectionReused() const
 {
-    return m_private->m_resourceResponse->connectionReused();
+    return m_private->resourceResponse()->connectionReused();
 }
 
 void WebURLResponse::setConnectionReused(bool connectionReused)
 {
-    m_private->m_resourceResponse->setConnectionReused(connectionReused);
+    m_private->resourceResponse()->setConnectionReused(connectionReused);
 }
 
 WebURLLoadTiming WebURLResponse::loadTiming()
 {
-    return WebURLLoadTiming(m_private->m_resourceResponse->resourceLoadTiming());
+    return WebURLLoadTiming(m_private->resourceResponse()->resourceLoadTiming());
 }
 
 void WebURLResponse::setLoadTiming(const WebURLLoadTiming& timing)
 {
     RefPtr<ResourceLoadTiming> loadTiming = PassRefPtr<ResourceLoadTiming>(timing);
-    m_private->m_resourceResponse->setResourceLoadTiming(loadTiming.release());
+    m_private->resourceResponse()->setResourceLoadTiming(loadTiming.release());
 }
 
 WebHTTPLoadInfo WebURLResponse::httpLoadInfo()
 {
-    return WebHTTPLoadInfo(m_private->m_resourceResponse->resourceLoadInfo());
+    return WebHTTPLoadInfo(m_private->resourceResponse()->resourceLoadInfo());
 }
 
 void WebURLResponse::setHTTPLoadInfo(const WebHTTPLoadInfo& value)
 {
-    m_private->m_resourceResponse->setResourceLoadInfo(value);
+    m_private->resourceResponse()->setResourceLoadInfo(value);
 }
 
 void WebURLResponse::setResponseTime(long long responseTime)
 {
-    m_private->m_resourceResponse->setResponseTime(static_cast<int64_t>(responseTime));
+    m_private->resourceResponse()->setResponseTime(static_cast<int64_t>(responseTime));
 }
 
 WebString WebURLResponse::mimeType() const
 {
-    return m_private->m_resourceResponse->mimeType();
+    return m_private->resourceResponse()->mimeType();
 }
 
 void WebURLResponse::setMIMEType(const WebString& mimeType)
 {
-    m_private->m_resourceResponse->setMimeType(mimeType);
+    m_private->resourceResponse()->setMimeType(mimeType);
 }
 
 long long WebURLResponse::expectedContentLength() const
 {
-    return m_private->m_resourceResponse->expectedContentLength();
+    return m_private->resourceResponse()->expectedContentLength();
 }
 
 void WebURLResponse::setExpectedContentLength(long long expectedContentLength)
 {
-    m_private->m_resourceResponse->setExpectedContentLength(expectedContentLength);
+    m_private->resourceResponse()->setExpectedContentLength(expectedContentLength);
 }
 
 WebString WebURLResponse::textEncodingName() const
 {
-    return m_private->m_resourceResponse->textEncodingName();
+    return m_private->resourceResponse()->textEncodingName();
 }
 
 void WebURLResponse::setTextEncodingName(const WebString& textEncodingName)
 {
-    m_private->m_resourceResponse->setTextEncodingName(textEncodingName);
+    m_private->resourceResponse()->setTextEncodingName(textEncodingName);
 }
 
 WebString WebURLResponse::suggestedFileName() const
 {
-    return m_private->m_resourceResponse->suggestedFilename();
+    return m_private->resourceResponse()->suggestedFilename();
 }
 
 void WebURLResponse::setSuggestedFileName(const WebString& suggestedFileName)
 {
-    m_private->m_resourceResponse->setSuggestedFilename(suggestedFileName);
+    m_private->resourceResponse()->setSuggestedFilename(suggestedFileName);
 }
 
 WebURLResponse::HTTPVersion WebURLResponse::httpVersion() const
 {
-    return static_cast<HTTPVersion>(m_private->m_resourceResponse->httpVersion());
+    return static_cast<HTTPVersion>(m_private->resourceResponse()->httpVersion());
 }
 
 void WebURLResponse::setHTTPVersion(HTTPVersion version)
 {
-    m_private->m_resourceResponse->setHTTPVersion(static_cast<ResourceResponse::HTTPVersion>(version));
+    m_private->resourceResponse()->setHTTPVersion(static_cast<ResourceResponse::HTTPVersion>(version));
 }
 
 int WebURLResponse::httpStatusCode() const
 {
-    return m_private->m_resourceResponse->httpStatusCode();
+    return m_private->resourceResponse()->httpStatusCode();
 }
 
 void WebURLResponse::setHTTPStatusCode(int httpStatusCode)
 {
-    m_private->m_resourceResponse->setHTTPStatusCode(httpStatusCode);
+    m_private->resourceResponse()->setHTTPStatusCode(httpStatusCode);
 }
 
 WebString WebURLResponse::httpStatusText() const
 {
-    return m_private->m_resourceResponse->httpStatusText();
+    return m_private->resourceResponse()->httpStatusText();
 }
 
 void WebURLResponse::setHTTPStatusText(const WebString& httpStatusText)
 {
-    m_private->m_resourceResponse->setHTTPStatusText(httpStatusText);
+    m_private->resourceResponse()->setHTTPStatusText(httpStatusText);
 }
 
 WebString WebURLResponse::httpHeaderField(const WebString& name) const
 {
-    return m_private->m_resourceResponse->httpHeaderField(name);
+    return m_private->resourceResponse()->httpHeaderField(name);
 }
 
 void WebURLResponse::setHTTPHeaderField(const WebString& name, const WebString& value)
 {
-    m_private->m_resourceResponse->setHTTPHeaderField(name, value);
+    m_private->resourceResponse()->setHTTPHeaderField(name, value);
 }
 
 void WebURLResponse::addHTTPHeaderField(const WebString& name, const WebString& value)
@@ -252,75 +272,75 @@ void WebURLResponse::addHTTPHeaderField(const WebString& name, const WebString&
     if (name.isNull() || value.isNull())
         return;
 
-    m_private->m_resourceResponse->addHTTPHeaderField(name, value);
+    m_private->resourceResponse()->addHTTPHeaderField(name, value);
 }
 
 void WebURLResponse::clearHTTPHeaderField(const WebString& name)
 {
-    m_private->m_resourceResponse->clearHTTPHeaderField(name);
+    m_private->resourceResponse()->clearHTTPHeaderField(name);
 }
 
 void WebURLResponse::visitHTTPHeaderFields(WebHTTPHeaderVisitor* visitor) const
 {
-    const HTTPHeaderMap& map = m_private->m_resourceResponse->httpHeaderFields();
+    const HTTPHeaderMap& map = m_private->resourceResponse()->httpHeaderFields();
     for (HTTPHeaderMap::const_iterator it = map.begin(); it != map.end(); ++it)
         visitor->visitHeader(it->key, it->value);
 }
 
 double WebURLResponse::lastModifiedDate() const
 {
-    return static_cast<double>(m_private->m_resourceResponse->lastModifiedDate());
+    return static_cast<double>(m_private->resourceResponse()->lastModifiedDate());
 }
 
 void WebURLResponse::setLastModifiedDate(double lastModifiedDate)
 {
-    m_private->m_resourceResponse->setLastModifiedDate(static_cast<time_t>(lastModifiedDate));
+    m_private->resourceResponse()->setLastModifiedDate(static_cast<time_t>(lastModifiedDate));
 }
 
 long long WebURLResponse::appCacheID() const
 {
-    return m_private->m_resourceResponse->appCacheID();
+    return m_private->resourceResponse()->appCacheID();
 }
 
 void WebURLResponse::setAppCacheID(long long appCacheID)
 {
-    m_private->m_resourceResponse->setAppCacheID(appCacheID);
+    m_private->resourceResponse()->setAppCacheID(appCacheID);
 }
 
 WebURL WebURLResponse::appCacheManifestURL() const
 {
-    return m_private->m_resourceResponse->appCacheManifestURL();
+    return m_private->resourceResponse()->appCacheManifestURL();
 }
 
 void WebURLResponse::setAppCacheManifestURL(const WebURL& url)
 {
-    m_private->m_resourceResponse->setAppCacheManifestURL(url);
+    m_private->resourceResponse()->setAppCacheManifestURL(url);
 }
 
 WebCString WebURLResponse::securityInfo() const
 {
     // FIXME: getSecurityInfo is misnamed.
-    return m_private->m_resourceResponse->getSecurityInfo();
+    return m_private->resourceResponse()->getSecurityInfo();
 }
 
 void WebURLResponse::setSecurityInfo(const WebCString& securityInfo)
 {
-    m_private->m_resourceResponse->setSecurityInfo(securityInfo);
+    m_private->resourceResponse()->setSecurityInfo(securityInfo);
 }
 
 void WebURLResponse::setHasMajorCertificateErrors(bool value)
 {
-    m_private->m_resourceResponse->setHasMajorCertificateErrors(value);
+    m_private->resourceResponse()->setHasMajorCertificateErrors(value);
 }
 
 WebURLResponse::SecurityStyle WebURLResponse::getSecurityStyle() const
 {
-    return static_cast<SecurityStyle>(m_private->m_resourceResponse->getSecurityStyle());
+    return static_cast<SecurityStyle>(m_private->resourceResponse()->getSecurityStyle());
 }
 
 void WebURLResponse::setSecurityStyle(SecurityStyle securityStyle)
 {
-    m_private->m_resourceResponse->setSecurityStyle(static_cast<ResourceResponse::SecurityStyle>(securityStyle));
+    m_private->resourceResponse()->setSecurityStyle(static_cast<ResourceResponse::SecurityStyle>(securityStyle));
 }
 
 void WebURLResponse::setSecurityDetails(const WebSecurityDetails& webSecurityDetails)
@@ -328,7 +348,7 @@ void WebURLResponse::setSecurityDetails(const WebSecurityDetails& webSecurityDet
     blink::ResourceResponse::SignedCertificateTimestampList sctList;
     for (const auto& iter : webSecurityDetails.sctList)
         sctList.append(static_cast<blink::ResourceResponse::SignedCertificateTimestamp>(iter));
-    m_private->m_resourceResponse->setSecurityDetails(
+    m_private->resourceResponse()->setSecurityDetails(
         webSecurityDetails.protocol,
         webSecurityDetails.keyExchange,
         webSecurityDetails.cipher,
@@ -342,165 +362,161 @@ void WebURLResponse::setSecurityDetails(const WebSecurityDetails& webSecurityDet
 
 ResourceResponse& WebURLResponse::toMutableResourceResponse()
 {
-    ASSERT(m_private);
-    ASSERT(m_private->m_resourceResponse);
-
-    return *m_private->m_resourceResponse;
+    DCHECK(m_private->resourceResponse());
+    return *m_private->resourceResponse();
 }
 
 const ResourceResponse& WebURLResponse::toResourceResponse() const
 {
-    ASSERT(m_private);
-    ASSERT(m_private->m_resourceResponse);
-
-    return *m_private->m_resourceResponse;
+    DCHECK(m_private->resourceResponse());
+    return *m_private->resourceResponse();
 }
 
 bool WebURLResponse::wasCached() const
 {
-    return m_private->m_resourceResponse->wasCached();
+    return m_private->resourceResponse()->wasCached();
 }
 
 void WebURLResponse::setWasCached(bool value)
 {
-    m_private->m_resourceResponse->setWasCached(value);
+    m_private->resourceResponse()->setWasCached(value);
 }
 
 bool WebURLResponse::wasFetchedViaSPDY() const
 {
-    return m_private->m_resourceResponse->wasFetchedViaSPDY();
+    return m_private->resourceResponse()->wasFetchedViaSPDY();
 }
 
 void WebURLResponse::setWasFetchedViaSPDY(bool value)
 {
-    m_private->m_resourceResponse->setWasFetchedViaSPDY(value);
+    m_private->resourceResponse()->setWasFetchedViaSPDY(value);
 }
 
 bool WebURLResponse::wasNpnNegotiated() const
 {
-    return m_private->m_resourceResponse->wasNpnNegotiated();
+    return m_private->resourceResponse()->wasNpnNegotiated();
 }
 
 void WebURLResponse::setWasNpnNegotiated(bool value)
 {
-    m_private->m_resourceResponse->setWasNpnNegotiated(value);
+    m_private->resourceResponse()->setWasNpnNegotiated(value);
 }
 
 bool WebURLResponse::wasAlternateProtocolAvailable() const
 {
-    return m_private->m_resourceResponse->wasAlternateProtocolAvailable();
+    return m_private->resourceResponse()->wasAlternateProtocolAvailable();
 }
 
 void WebURLResponse::setWasAlternateProtocolAvailable(bool value)
 {
-    m_private->m_resourceResponse->setWasAlternateProtocolAvailable(value);
+    m_private->resourceResponse()->setWasAlternateProtocolAvailable(value);
 }
 
 bool WebURLResponse::wasFetchedViaProxy() const
 {
-    return m_private->m_resourceResponse->wasFetchedViaProxy();
+    return m_private->resourceResponse()->wasFetchedViaProxy();
 }
 
 void WebURLResponse::setWasFetchedViaProxy(bool value)
 {
-    m_private->m_resourceResponse->setWasFetchedViaProxy(value);
+    m_private->resourceResponse()->setWasFetchedViaProxy(value);
 }
 
 bool WebURLResponse::wasFetchedViaServiceWorker() const
 {
-    return m_private->m_resourceResponse->wasFetchedViaServiceWorker();
+    return m_private->resourceResponse()->wasFetchedViaServiceWorker();
 }
 
 void WebURLResponse::setWasFetchedViaServiceWorker(bool value)
 {
-    m_private->m_resourceResponse->setWasFetchedViaServiceWorker(value);
+    m_private->resourceResponse()->setWasFetchedViaServiceWorker(value);
 }
 
 bool WebURLResponse::wasFallbackRequiredByServiceWorker() const
 {
-    return m_private->m_resourceResponse->wasFallbackRequiredByServiceWorker();
+    return m_private->resourceResponse()->wasFallbackRequiredByServiceWorker();
 }
 
 void WebURLResponse::setWasFallbackRequiredByServiceWorker(bool value)
 {
-    m_private->m_resourceResponse->setWasFallbackRequiredByServiceWorker(value);
+    m_private->resourceResponse()->setWasFallbackRequiredByServiceWorker(value);
 }
 
 WebServiceWorkerResponseType WebURLResponse::serviceWorkerResponseType() const
 {
-    return m_private->m_resourceResponse->serviceWorkerResponseType();
+    return m_private->resourceResponse()->serviceWorkerResponseType();
 }
 
 void WebURLResponse::setServiceWorkerResponseType(WebServiceWorkerResponseType value)
 {
-    m_private->m_resourceResponse->setServiceWorkerResponseType(value);
+    m_private->resourceResponse()->setServiceWorkerResponseType(value);
 }
 
 WebURL WebURLResponse::originalURLViaServiceWorker() const
 {
-    return m_private->m_resourceResponse->originalURLViaServiceWorker();
+    return m_private->resourceResponse()->originalURLViaServiceWorker();
 }
 
 void WebURLResponse::setOriginalURLViaServiceWorker(const WebURL& url)
 {
-    m_private->m_resourceResponse->setOriginalURLViaServiceWorker(url);
+    m_private->resourceResponse()->setOriginalURLViaServiceWorker(url);
 }
 
 void WebURLResponse::setMultipartBoundary(const char* bytes, size_t size)
 {
-    m_private->m_resourceResponse->setMultipartBoundary(bytes, size);
+    m_private->resourceResponse()->setMultipartBoundary(bytes, size);
 }
 
 WebString WebURLResponse::cacheStorageCacheName() const
 {
-    return m_private->m_resourceResponse->cacheStorageCacheName();
+    return m_private->resourceResponse()->cacheStorageCacheName();
 }
 
 void WebURLResponse::setCacheStorageCacheName(const WebString& cacheStorageCacheName)
 {
-    m_private->m_resourceResponse->setCacheStorageCacheName(cacheStorageCacheName);
+    m_private->resourceResponse()->setCacheStorageCacheName(cacheStorageCacheName);
 }
 
 void WebURLResponse::setCorsExposedHeaderNames(const WebVector<WebString>& headerNames)
 {
     Vector<String> exposedHeaderNames;
     exposedHeaderNames.append(headerNames.data(), headerNames.size());
-    m_private->m_resourceResponse->setCorsExposedHeaderNames(exposedHeaderNames);
+    m_private->resourceResponse()->setCorsExposedHeaderNames(exposedHeaderNames);
 }
 
 WebString WebURLResponse::downloadFilePath() const
 {
-    return m_private->m_resourceResponse->downloadedFilePath();
+    return m_private->resourceResponse()->downloadedFilePath();
 }
 
 void WebURLResponse::setDownloadFilePath(const WebString& downloadFilePath)
 {
-    m_private->m_resourceResponse->setDownloadedFilePath(downloadFilePath);
+    m_private->resourceResponse()->setDownloadedFilePath(downloadFilePath);
 }
 
 WebString WebURLResponse::remoteIPAddress() const
 {
-    return m_private->m_resourceResponse->remoteIPAddress();
+    return m_private->resourceResponse()->remoteIPAddress();
 }
 
 void WebURLResponse::setRemoteIPAddress(const WebString& remoteIPAddress)
 {
-    m_private->m_resourceResponse->setRemoteIPAddress(remoteIPAddress);
+    m_private->resourceResponse()->setRemoteIPAddress(remoteIPAddress);
 }
 
 unsigned short WebURLResponse::remotePort() const
 {
-    return m_private->m_resourceResponse->remotePort();
+    return m_private->resourceResponse()->remotePort();
 }
 
 void WebURLResponse::setRemotePort(unsigned short remotePort)
 {
-    m_private->m_resourceResponse->setRemotePort(remotePort);
+    m_private->resourceResponse()->setRemotePort(remotePort);
 }
 
 WebURLResponse::ExtraData* WebURLResponse::getExtraData() const
 {
-    RefPtr<ResourceResponse::ExtraData> data = m_private->m_resourceResponse->getExtraData();
+    RefPtr<ResourceResponse::ExtraData> data = m_private->resourceResponse()->getExtraData();
     if (!data)
         return 0;
     return static_cast<ExtraDataContainer*>(data.get())->getExtraData();
@@ -508,18 +524,14 @@ WebURLResponse::ExtraData* WebURLResponse::getExtraData() const
 
 void WebURLResponse::setExtraData(WebURLResponse::ExtraData* extraData)
 {
-    m_private->m_resourceResponse->setExtraData(ExtraDataContainer::create(extraData));
+    m_private->resourceResponse()->setExtraData(ExtraDataContainer::create(extraData));
 }
 
-void WebURLResponse::assign(WebURLResponsePrivate* p)
+WebURLResponse::WebURLResponse(WebURLResponsePrivate* p)
+    : m_private(p)
+    , m_owningPrivate(false)
 {
-    // Subclasses may call this directly so a self-assignment check is needed
-    // here as well as in the public assign method.
-    if (m_private == p)
-        return;
-    if (m_private)
-        m_private->dispose();
-    m_private = p;
+    DCHECK(p);
 }
 
 } // namespace blink