Bug 86522 - [Chromium] Store HTTP version in WebURLResponse
Summary: [Chromium] Store HTTP version in WebURLResponse
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ami Fischman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-15 14:02 PDT by Ami Fischman
Modified: 2012-05-17 19:36 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.79 KB, patch)
2012-05-15 14:14 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2012-05-16 13:33 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (6.32 KB, patch)
2012-05-17 15:58 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2012-05-17 16:28 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (7.30 KB, patch)
2012-05-17 18:02 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ami Fischman 2012-05-15 14:02:30 PDT
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).
Comment 1 Ami Fischman 2012-05-15 14:14:30 PDT
Created attachment 142050 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-15 14:17:20 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Ami Fischman 2012-05-15 14:25:48 PDT
Chromium-side CL is https://chromiumcodereview.appspot.com/10383201 (depends on this one).
Comment 4 Adam Barth 2012-05-15 14:51:04 PDT
Generally speaking, it's tricky to add things to ResourceResponseBase that aren't available in CFNetwork.
Comment 5 Ami Fischman 2012-05-15 14:56:36 PDT
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?
Comment 6 Adam Barth 2012-05-15 15:04:26 PDT
> 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.
Comment 7 Ami Fischman 2012-05-15 15:11:49 PDT
Thanks Adam.

Darin: mind taking a look?  (I'll need you for OWNERS approval on the chromium side (linked in comment #3) anyway).
Comment 8 Alexey Proskuryakov 2012-05-15 15:18:59 PDT
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?
Comment 9 Ami Fischman 2012-05-15 15:37:34 PDT
(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?
Comment 10 Alexey Proskuryakov 2012-05-15 16:24:45 PDT
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.
Comment 11 Alexey Proskuryakov 2012-05-15 16:25:36 PDT
Generally, people have one or two build environments, and rely on EWS bots to catch build breakage in others.
Comment 12 Ami Fischman 2012-05-16 13:31:05 PDT
Narrowing this bug to be chromium-only.
platformLazyInit() doesn't work for chromium because the actual loader code lives outside webkit (in chromium).
Comment 13 Ami Fischman 2012-05-16 13:33:28 PDT
Created attachment 142335 [details]
Patch
Comment 14 Ami Fischman 2012-05-16 13:34:42 PDT
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)
Comment 15 James Robinson 2012-05-16 14:42:03 PDT
(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.
Comment 16 Ami Fischman 2012-05-16 15:02:45 PDT
(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)
Comment 17 James Robinson 2012-05-16 15:21:00 PDT
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
Comment 18 James Robinson 2012-05-16 15:22:38 PDT
(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.
Comment 19 Adam Barth 2012-05-16 20:20:07 PDT
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.
Comment 20 Ami Fischman 2012-05-17 15:58:47 PDT
Created attachment 142574 [details]
Patch
Comment 21 Ami Fischman 2012-05-17 15:59:41 PDT
Thanks Adam & James.  Uploaded new patch that maintains different types in Platform and in WebCore/platform/network.
Comment 22 James Robinson 2012-05-17 16:20:24 PDT
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)
Comment 23 Ami Fischman 2012-05-17 16:28:46 PDT
Created attachment 142583 [details]
Patch
Comment 24 Ami Fischman 2012-05-17 16:31:20 PDT
(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.
Comment 25 Adam Barth 2012-05-17 17:57:30 PDT
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.
Comment 26 James Robinson 2012-05-17 17:58:08 PDT
Comment on attachment 142583 [details]
Patch

R=me
Comment 27 Adam Barth 2012-05-17 17:58:23 PDT
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.
Comment 28 Ami Fischman 2012-05-17 18:02:39 PDT
Created attachment 142599 [details]
Patch
Comment 29 Ami Fischman 2012-05-17 18:04:16 PDT
(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).
Comment 30 Adam Barth 2012-05-17 18:11:22 PDT
Fair enough.  Do you think it's worth adding a comment to the API that non-HTTP URL requests return Unknown?
Comment 31 Ami Fischman 2012-05-17 18:16:19 PDT
(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.
Comment 32 WebKit Review Bot 2012-05-17 18:21:38 PDT
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
Comment 33 WebKit Review Bot 2012-05-17 19:36:03 PDT
Comment on attachment 142599 [details]
Patch

Clearing flags on attachment: 142599

Committed r117529: <http://trac.webkit.org/changeset/117529>
Comment 34 WebKit Review Bot 2012-05-17 19:36:09 PDT
All reviewed patches have been landed.  Closing bug.