Bug 136545 – Serialize ResourceResponses using WebKit types

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

[Antti Koivisto]

Currently: (network process)  NSURLResponse -> serialization -> (web process) deserialization -> NSURLResponse -> WebKit types
After: (network process)  NSURLResponse ->  WebKit types -> serialization -> (web process) deserialization -> WebKit types

This is more efficient and in most cases we won't need to construct NSURLResponses in the web process at all.

[Antti Koivisto]

Created attachment 237691 [details]
patch

[Antti Koivisto]

Created attachment 237693 [details]
patch 2

[Alexey Proskuryakov]

Comment on attachment 237693 [details]
patch 2

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

> Source/WebCore/ChangeLog:3
> +        Serialize ResourceResponses using WebKit types

This seems like a good idea, but I honestly don't know how to evaluate correctness. Perhaps I'm too scarred from dealing with ResourceRequest, which is admittedly much more involved.

Did you check CFNetwork response serialization code for things that we don't extract into the WebCore class?

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:70
> -NSURLResponse *ResourceResponse::nsURLResponse() const
> +NSURLResponse* ResourceResponse::nsURLResponse() const

This star was positioned correctly.

> Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm:-154
> -    RetainPtr<CFDictionaryRef> dictionary = adoptCF(WKNSURLResponseCreateSerializableRepresentation(resourceResponse.nsURLResponse(), IPC::tokenNullTypeRef()));

WKNSURLResponseCreateSerializableRepresentation should be removed from WKSI now. This patch could remove a declaration from the open source copy.

> Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm:-173
> -    RetainPtr<NSURLResponse> nsURLResponse = WKNSURLResponseFromSerializableRepresentation(dictionary.get(), IPC::tokenNullTypeRef());

Ditto.

[Alexey Proskuryakov]

Comment on attachment 237693 [details]
patch 2

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

Antti checked what CFNetwork serialization code does, and it seems like we don't lose much.

r=me with the comments addressed, and Gtk build fixed.

> Source/WebCore/platform/network/ResourceResponseBase.h:232
> +    if (!decoder.decode(response.m_httpStatusText))
> +        return false;

We need a Radar and a FIXME somewhere (not here) about how we fail to add m_httpStatusText to NSURLResponse when re-creating it.

This patch fixes status text for XHR, but breaks it for API clients.

The XHR issue this fixes is rdar://problem/13751647, please unskip the tests.

[Antti Koivisto]

Created attachment 237737 [details]
for bots

[Antti Koivisto]

Created attachment 237738 [details]
for bots 2

[Antti Koivisto]

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

[Simon Fraser (smfr)]

17 test failures after this:
http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r173356%20(6649)/results.html

[Antti Koivisto]

Should be fixed by https://trac.webkit.org/r173360

[Alexey Proskuryakov]

> WKNSURLResponseCreateSerializableRepresentation should be removed from WKSI now. This patch could remove a declaration from the open source copy.

Do you intend to do this in a separate patch?

[Antti Koivisto]

Yep.