Bug 136545 - Serialize ResourceResponses using WebKit types
Summary: Serialize ResourceResponses using WebKit types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-04 13:36 PDT by Antti Koivisto
Modified: 2014-09-07 10:45 PDT (History)
11 users (show)

See Also:


Attachments
patch (18.42 KB, patch)
2014-09-05 08:00 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch 2 (18.43 KB, patch)
2014-09-05 08:15 PDT, Antti Koivisto
ap: review+
Details | Formatted Diff | Diff
for bots (18.77 KB, patch)
2014-09-06 07:50 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
for bots 2 (18.69 KB, patch)
2014-09-06 08:02 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2014-09-04 13:36:03 PDT
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.
Comment 1 Antti Koivisto 2014-09-05 08:00:49 PDT
Created attachment 237691 [details]
patch
Comment 2 Antti Koivisto 2014-09-05 08:15:32 PDT
Created attachment 237693 [details]
patch 2
Comment 3 Alexey Proskuryakov 2014-09-05 09:49:15 PDT
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.
Comment 4 Alexey Proskuryakov 2014-09-05 10:20:04 PDT
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.
Comment 5 Antti Koivisto 2014-09-06 07:50:05 PDT
Created attachment 237737 [details]
for bots
Comment 6 Antti Koivisto 2014-09-06 08:02:45 PDT
Created attachment 237738 [details]
for bots 2
Comment 7 Antti Koivisto 2014-09-06 08:33:49 PDT
http://trac.webkit.org/changeset/173356
Comment 8 Simon Fraser (smfr) 2014-09-06 10:55:16 PDT
17 test failures after this:
http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r173356%20(6649)/results.html
Comment 9 Antti Koivisto 2014-09-06 10:59:31 PDT
Should be fixed by https://trac.webkit.org/r173360
Comment 10 Alexey Proskuryakov 2014-09-06 17:06:26 PDT
> 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?
Comment 11 Antti Koivisto 2014-09-07 10:45:13 PDT
Yep.