Summary: | [Chromium] Store HTTP version in WebURLResponse | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ami Fischman <fischman> | ||||||||||||
Component: | Platform | Assignee: | Ami Fischman <fischman> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, ap, dglazkov, fischman, fishd, jamesr, tkent+wkapi, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Ami Fischman
2012-05-15 14:02:30 PDT
Created attachment 142050 [details]
Patch
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. Chromium-side CL is https://chromiumcodereview.appspot.com/10383201 (depends on this one). Generally speaking, it's tricky to add things to ResourceResponseBase that aren't available in CFNetwork. 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? > 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.
Thanks Adam. Darin: mind taking a look? (I'll need you for OWNERS approval on the chromium side (linked in comment #3) anyway). 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? (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? 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. Generally, people have one or two build environments, and rely on EWS bots to catch build breakage in others. Narrowing this bug to be chromium-only. platformLazyInit() doesn't work for chromium because the actual loader code lives outside webkit (in chromium). Created attachment 142335 [details]
Patch
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) (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. (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 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 (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 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. Created attachment 142574 [details]
Patch
Thanks Adam & James. Uploaded new patch that maintains different types in Platform and in WebCore/platform/network. 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) Created attachment 142583 [details]
Patch
(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 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 on attachment 142583 [details]
Patch
R=me
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. Created attachment 142599 [details]
Patch
(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). Fair enough. Do you think it's worth adding a comment to the API that non-HTTP URL requests return Unknown? (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 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 on attachment 142599 [details] Patch Clearing flags on attachment: 142599 Committed r117529: <http://trac.webkit.org/changeset/117529> All reviewed patches have been landed. Closing bug. |