Bug 86522 – [Chromium] Store HTTP version in WebURLResponse

• Component: Platform
• Status: RESOLVED FIXED

[Ami Fischman]

Today the HTTP status code for a response is available (e.g. in WebURLResponse::httpStatusCode()) but the HTTP version number (typically 1.0 or 1.1) is not.  It can be useful to ports to know the HTTP version claimed in a response to judge its correctness, so expose it.
An example use-case is seeing that a 206 Partial code came back in a 1.0 response; that's not up to spec and different ports may make different decisions based on knowing the version (for example, today chromium will use such content for media playback, but will not effectively cache it).

[Ami Fischman]

Created attachment 142050 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Ami Fischman]

Chromium-side CL is https://chromiumcodereview.appspot.com/10383201 (depends on this one).

[Adam Barth]

Generally speaking, it's tricky to add things to ResourceResponseBase that aren't available in CFNetwork.

[Ami Fischman]

abarth: thanks for chiming in.  I'm completely ignorant of the os/x stack, but https://developer.apple.com/library/mac/#documentation/CoreFoundation/Reference/CFMessageRef/Reference/reference.html#//apple_ref/doc/c_ref/CFHTTPMessageCreateEmpty makes me think that it's possible to get the complete status line, which "includes the message’s protocol version" (matching HTTP's "status line" nomenclature that encompasses HTTP version, status code, and status text).

Are you saying something else is unavailable in CFNetwork, or is the above not CFNetwork?

[Adam Barth]

> Are you saying something else is unavailable in CFNetwork, or is the above not CFNetwork?

My comment was more of a drive-by comment when I saw this in my bugmail and an explanation to ap for why I was CCing him on this bug.

[Ami Fischman]

Thanks Adam.

Darin: mind taking a look?  (I'll need you for OWNERS approval on the chromium side (linked in comment #3) anyway).

[Alexey Proskuryakov]

Comment on attachment 142050 [details]
Patch

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

Seems reasonable to expose this information.

One can certainly parse HTTP version from information exposed by network back-end. This patch doesn't do that however - it just adds a getter and a setter(?!).

To add an httpVersionNumber() function to ResourceResponseBase, one needs to make it work with all network back-ends - adding a cross-platform function that just lies is not good. You can grep for how it's done for other data members, such as m_httpStatusLine.

> Source/WebCore/platform/network/ResourceResponseBase.h:75
> +    void httpVersionNumber(int* major, int* minor) const;

Can we use references or even just a BCD-coded number?

[Ami Fischman]

(In reply to comment #8)
> One can certainly parse HTTP version from information exposed by network back-end. This patch doesn't do that however - it just adds a getter and a setter(?!).

Yes; my (extremely) limited understanding is that each port is likely to set & get from their glue code.  For example for chromium I'm proposing to do it like this: https://chromiumcodereview.appspot.com/10383201
Are you saying that this bug must atomically land the equivalent of my change to Source/WebCore/platform/chromium/support/WebURLResponse.cpp for all ports at once?
(I'm asking clarifying questions since I'm surprised there's an expectation that a single developer be able to grok the network APIs on all of blackberry/efl/qt/CF/mac/chromium, and have build environments to support all of them)

> To add an httpVersionNumber() function to ResourceResponseBase, one needs to make it work with all network back-ends - adding a cross-platform function that just lies is not good. You can grep for how it's done for other data members, such as m_httpStatusLine.

Do you mean m_httpStatusText (m_httpStatusLine doesn't occur in chromium/webkit)?
Each port comes by that text in very platform-specific ways (after all, this is the point of being in port-specific code ;)).  I'm not sure how to go about testing changes to all of these files (or for that matter any of them other than chromium).

> 
> > Source/WebCore/platform/network/ResourceResponseBase.h:75
> > +    void httpVersionNumber(int* major, int* minor) const;
> Can we use references or even just a BCD-coded number?

Is BCD-coding common in WebCore? (if yes, can you point me at a typical usage and/or codec functions?)
I'm not sure why we'd use that here, though; is saving a word per HTTP request worthwhile at the cost of readability/obviousness/easy-of-debuggability?


Stepping back a bit, would it be better to add the version number info just to Source/Platform/chromium/public/WebURLResponse.h (or WebURLResponsePrivate) and avoid touching the cross-platform ResourceResponseBase?

[Alexey Proskuryakov]

In most important code path, a setter is not needed because a subclass just writes into ResourceResponseBase member variable in platformLazyInit(). It's fine to add it if it's needed for fake responses, like the blob ones here.

It is not necessary to land this in one commit, however it is necessary to have implementations before exposing a cross-platform function. It's just not sustainable to have a project where such low level functions magically work or do not work depending on your port.

> Is BCD-coding common in WebCore? (if yes, can you point me at a typical usage and/or codec functions?)

My suggestion is actually about convenience of use. You could write code like:

if (kHTTPVersion09 == response.httpVersionNumber()) {
  ...
}

> Stepping back a bit, would it be better to add the version number info just to 
> Source/Platform/chromium/public/WebURLResponse.h (or WebURLResponsePrivate)

There is also ResourceResponse.h in WebCore/network/chromium.

Adding this functionality to chromium specific files is certainly better than leaving it exposed and unimplemented. However, you'd be trading some upfront work for long-term difficulty - a chromium developer may expect this function to be available everywhere, build a patch using it, all to find that it breaks the build on every other platform. Also, when/if core developers add such functionality to ResourceResponseBase, that might end up disturbing chromium code base in undesirable ways.

[Alexey Proskuryakov]

Generally, people have one or two build environments, and rely on EWS bots to catch build breakage in others.

[Ami Fischman]

Narrowing this bug to be chromium-only.
platformLazyInit() doesn't work for chromium because the actual loader code lives outside webkit (in chromium).

[Ami Fischman]

Created attachment 142335 [details]
Patch

[Ami Fischman]

Comment on attachment 142335 [details]
Patch

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

> Source/WebCore/platform/network/chromium/ResourceResponse.h:30
> +#include "../../../../Platform/chromium/public/WebURLResponse.h"

I expect this is the wrong way to do this, but I'm not sure what the right way is.  Hopefully someone on this bug can enlighten me.

(jamesr@ suggested 
#include <public/WebURLResponse.h>
in IRC, but that doesn't compile: https://gist.github.com/2713600)

[James Robinson]

(In reply to comment #14)
> (From update of attachment 142335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142335&action=review
> 
> > Source/WebCore/platform/network/chromium/ResourceResponse.h:30
> > +#include "../../../../Platform/chromium/public/WebURLResponse.h"
> 
> I expect this is the wrong way to do this, but I'm not sure what the right way is.  Hopefully someone on this bug can enlighten me.
> 
> (jamesr@ suggested 
> #include <public/WebURLResponse.h>
> in IRC, but that doesn't compile: https://gist.github.com/2713600)

The issue here is that only translation units in webcore_platform can "see" the include path to use <public/WebURLResponse.h>, but the translation unit in question here is in WebCore/bindings.

I do not think the ../../../ is a good idea.  I think you should probably hide the include in the .cpp - since it looks like all you want it some types.  Adam might have some ideas about how to structure the network stuff in particular.

[Ami Fischman]

(In reply to comment #15)
> The issue here is that only translation units in webcore_platform can "see" the include path to use <public/WebURLResponse.h>, but the translation unit in question here is in WebCore/bindings.

Shouldn't -I paths be transitively inclusive?  IOW if WebCore/bindings can depend on headers in WebCore/platform and WebCore/platform can depend on Platform, mustn't WC/b include the -I for Platform?

> I do not think the ../../../ is a good idea.

I don't doubt that it's a horrible idea.

> I think you should probably hide the include in the .cpp

How?  The enum I'm looking for is part of the ResourceResponse interface.

> since it looks like all you want it some types.  Adam might have some ideas about how to structure the network stuff in particular.

That'd be great.

Note, BTW, that this could all have been avoided if WebURLResponse would store the version field explicitly (instead of plumbing it through ResourceResponse).  I didn't do that because WUR's use of a RR* smelled to me like maybe sharing is happening under the covers.  Is that wrong?  (and thus I should just add the version field to WUR itself)

[James Robinson]

Comment on attachment 142335 [details]
Patch

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

>> Source/WebCore/platform/network/chromium/ResourceResponse.h:30
>> +#include "../../../../Platform/chromium/public/WebURLResponse.h"
> 
> I expect this is the wrong way to do this, but I'm not sure what the right way is.  Hopefully someone on this bug can enlighten me.
> 
> (jamesr@ suggested 
> #include <public/WebURLResponse.h>
> in IRC, but that doesn't compile: https://gist.github.com/2713600)

Or simply define the types you need in WebCore/... and do the mapping to Platform types in WebCore/platform/chromium/support/WebURL*.cpp, which can see WebURLRequest.h

[James Robinson]

(In reply to comment #16)
> (In reply to comment #15)
> > The issue here is that only translation units in webcore_platform can "see" the include path to use <public/WebURLResponse.h>, but the translation unit in question here is in WebCore/bindings.
> 
> Shouldn't -I paths be transitively inclusive?  IOW if WebCore/bindings can depend on headers in WebCore/platform and WebCore/platform can depend on Platform, mustn't WC/b include the -I for Platform?
> 

We currently don't propagate this path (at least) transitively.  Maybe we'll do that someday, I'm not totally sure.  The dependency from WebCore/platform on Platform/ is new so we've been conservative so far in expanding visibility to try to feel things out.

[Adam Barth]

Comment on attachment 142335 [details]
Patch

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

> Source/WebCore/platform/network/chromium/ResourceResponse.h:47
> +            : m_httpVersion(WebKit::WebURLResponse::Unknown)

Yeah, we block including from Platform/chromium/public so that we don't leak these API concepts to the rest of WebCore.  In particular, this line isn't correct because only things inside WebCore/platform are allowed to know about the API.

Instead, you should create a WebCore/platform/network abstraction for HTTPVersion.  That way when we expand HTTPVersion to be used by other ports, we won't have Chromium-specific API types messing us up.

[Ami Fischman]

Created attachment 142574 [details]
Patch

[Ami Fischman]

Thanks Adam & James.  Uploaded new patch that maintains different types in Platform and in WebCore/platform/network.

[James Robinson]

Comment on attachment 142574 [details]
Patch

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

> Source/WebCore/platform/chromium/support/WebURLResponse.cpp:216
> +    switch (m_private->m_resourceResponse->httpVersion()) {

a more typical pattern for WebKit API enums that correspond to WebCore enums is to compile assert in Source/WebKit/chromium/src/AssertMatchingEnums.cpp that the values are the same and then just static_cast<> between them.  It saves a lot of boilerplate and provides a louder warning if the enums fall out of sync (compile time >>> runtime)

[Ami Fischman]

Created attachment 142583 [details]
Patch

[Ami Fischman]

(In reply to comment #22)
> (From update of attachment 142574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142574&action=review
> 
> > Source/WebCore/platform/chromium/support/WebURLResponse.cpp:216
> > +    switch (m_private->m_resourceResponse->httpVersion()) {
> 
> a more typical pattern for WebKit API enums that correspond to WebCore enums is to compile assert in Source/WebKit/chromium/src/AssertMatchingEnums.cpp that the values are the same and then just static_cast<> between them.  It saves a lot of boilerplate and provides a louder warning if the enums fall out of sync (compile time >>> runtime)

SGTM; done.

[Adam Barth]

Comment on attachment 142583 [details]
Patch

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

> Source/Platform/chromium/public/WebURLResponse.h:53
> +    enum HTTPVersion { Unknown, HTTP_0_9, HTTP_1_0, HTTP_1_1 };

Why no HTTP_2_0 ?  :)

> Source/WebCore/platform/network/chromium/ResourceResponse.h:39
> +        enum HTTPVersion { Unknown, HTTP_0_9, HTTP_1_0, HTTP_1_1 };

I'd add a FIXME about moving this enum to ResourceResponseBase and implementing it for other ports.

[James Robinson]

Comment on attachment 142583 [details]
Patch

R=me

[Adam Barth]

Comment on attachment 142583 [details]
Patch

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

>> Source/Platform/chromium/public/WebURLResponse.h:53
>> +    enum HTTPVersion { Unknown, HTTP_0_9, HTTP_1_0, HTTP_1_1 };
> 
> Why no HTTP_2_0 ?  :)

One further thought: what should this value be for non-HTTP URL requests (e.g., ftp)?  I wonder if there's a better name than Unknown that might cover that case.

[Ami Fischman]

Created attachment 142599 [details]
Patch

[Ami Fischman]

(In reply to comment #25)
> I'd add a FIXME about moving this enum to ResourceResponseBase and implementing it for other ports.

Done.


(In reply to comment #27)
> One further thought: what should this value be for non-HTTP URL requests (e.g., ftp)?  I wonder if there's a better name than Unknown that might cover that case.

It doesn't seem worthwhile to me to architect a multi-way selector scheme, esp. in the presence of ResourceResponseBase::httpStatusCode() (which just returns 0 for non-http responses that won't fake a code).

[Adam Barth]

Fair enough.  Do you think it's worth adding a comment to the API that non-HTTP URL requests return Unknown?

[Ami Fischman]

(In reply to comment #30)
> Fair enough.  Do you think it's worth adding a comment to the API that non-HTTP URL requests return Unknown?

I don't think so - the whole thing is a get/put API; if you don't put anything, there's nothing to get out.

[WebKit Review Bot]

Comment on attachment 142599 [details]
Patch

Rejecting attachment 142599 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/Platform/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12719671

[WebKit Review Bot]

Comment on attachment 142599 [details]
Patch

Clearing flags on attachment: 142599

Committed r117529: <http://trac.webkit.org/changeset/117529>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.