Http cache: Always preserve extra headers when dealing with...

Http cache: Always preserve extra headers when dealing with
byte range requests.

BUG=25755
TEST=unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30773

[eroman, 2009-11-02 21:30:59]

Do we have a test for the non-range case (that we include the extra request
headers with validation requests)? If not, could you add a TODO to create such a
test.

LGTM.

http://codereview.chromium.org/339088/diff/1/2
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/339088/diff/1/2#newcode581
Line 581: #define BLAH_HEADER "Blah: header\r\n"
I recommend choosing a different name. Perhaps EXTRA_HEADER, or
EXTRA_REQUEST_HEADER ? Or maybe make it more specific, like a Cookie header, and
then it can be called COOKIE_HEADER or REQUEST_COOKIES.

http://codereview.chromium.org/339088/diff/1/2#newcode595
Line 595: response_status->assign("HTTP/1.1 416 Requested Range Not
Satisfiable");
Nit: this seems a little awkward. How about doing a gtest-level assertion, or
returning a different status (like for example 401 unauthorized, if the extra
header represents a cookie).

http://codereview.chromium.org/339088/diff/1/2#newcode1911
Line 1911: transaction.request_headers = "If-None-Match: foo\n" BLAH_HEADER
style-nit: I suggest putting "BLAH_HEADER on a new line, so it looks like the
other header lines (even though we are string concatenating).

http://codereview.chromium.org/339088/diff/1/4
File net/http/partial_data.h (right):

http://codereview.chromium.org/339088/diff/1/4#newcode48
Line 48: void SetHeaders(const std::string& headers);
The distinction between this function and Init() is a bit confusing.

Could the |new_headers| parameter be removed from Init() so there aren't two
ways to do this? (and just call SetHeaders() in both cases)

[rvargas (doing something else), 2009-11-03 00:03:33]

Thanks, all done.

[eroman, 2009-11-03 04:39:56]

LGTM