Use HttpRequestHeaders for extra_headers.

Use HttpRequestHeaders for extra_headers.
BUG=22588
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45096

[willchan no longer on Chromium, 2010-04-04 10:23:06]

eroman: Please review it all in general.
rvargas: Please review all the HttpCache related files.

[eroman, 2010-04-04 22:14:42]

nice change!

my important comments are:

http://codereview.chromium.org/1604011/diff/1/12#newcode216
http://codereview.chromium.org/1604011/diff/1/14#newcode32
http://codereview.chromium.org/1604011/diff/1/29#newcode339

http://codereview.chromium.org/1604011/diff/1/7
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/1604011/diff/1/7#newcode84
net/http/http_cache_transaction.cc:84: const std::string* header_value =
headers.GetHeader(search->name);
The inner loop is now less efficient than in the original approach. Before it
was just iterating through all the headers once, whereas now it calls
GetHeader() each time, and GetHeader() is an O(n) function.

I wander if it wouldn't be better to preserve the original flow, and use
HttpRequestHeaders::Iterator.

Or, could optimize HttpRequestHeaders::GetHeader() to be O(1) (especially since
it is pretty common to call Has/Get).

Overall though, I still like your new implementation, since you avoid
constructing the |new_headers| string.

http://codereview.chromium.org/1604011/diff/1/7#newcode91
net/http/http_cache_transaction.cc:91: HttpUtil::ValuesIterator
v(header_value->begin(), header_value->end(),',');
nit: please add a space after the comma.

http://codereview.chromium.org/1604011/diff/1/7#newcode1507
net/http/http_cache_transaction.cc:1507:
custom_request_->extra_headers.SetHeader("If-Range", etag_value);
nit: below you use the constant kIfRange instead of "If-Range". Please use one
or the other approach throughout.

http://codereview.chromium.org/1604011/diff/1/7#newcode1509
net/http/http_cache_transaction.cc:1509:
custom_request_->extra_headers.SetHeader("If-None-Match", etag_value);
see comment above.

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

http://codereview.chromium.org/1604011/diff/1/8#newcode816
net/http/http_cache_unittest.cc:816: // Returns true if the response headers
(|response|) match a partial content
"Returns" is no longer applicable.

http://codereview.chromium.org/1604011/diff/1/12
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/1604011/diff/1/12#newcode17
net/http/http_request_headers.cc:17: const char
HttpRequestHeaders::kAvailDictionary[] = "Avail-Dictionary: ";
Hm, I dunno that I like this.

Avail-Dictionary is a header from SDCH, seems wrong to enumerate it under the
generic HTTP headers.

http://codereview.chromium.org/1604011/diff/1/12#newcode86
net/http/http_request_headers.cc:86: return out;
Not sure what the compiler is going to do here, but might be more efficient to
make this an out parameter of the function.

http://codereview.chromium.org/1604011/diff/1/12#newcode216
net/http/http_request_headers.cc:216: if (!base::strncasecmp(key.data(),
it->key.data(), key.length()))
Isn't this the same bug you had in an earlier revision?
... strncasecmp is a substring check, so must check the length of |key| as well.

http://codereview.chromium.org/1604011/diff/1/13
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/1604011/diff/1/13#newcode34
net/http/http_request_headers.h:34: class Iterator {
Nice.

http://codereview.chromium.org/1604011/diff/1/13#newcode93
net/http/http_request_headers.h:93: const std::string* GetHeader(const
base::StringPiece& key) const;
I think this should be called GetFirstHeader() to emphasize that there may be
duplicates.

Even better, why not just expose FindNextHeader() instead? IMO that API is
cleaner and more powerful. It also seems good to let the consumer see the
header's key, which may be different case than what they searched.

http://codereview.chromium.org/1604011/diff/1/13#newcode95
net/http/http_request_headers.h:95: // Gets the first header that matches |key|.
 Returns NULL if not found.
This comment is wrong.

http://codereview.chromium.org/1604011/diff/1/13#newcode138
net/http/http_request_headers.h:138: HeaderVector::iterator FindHeader(const
base::StringPiece& key);
It is worth documenting that this returns the iterator to the header *following*
the one found, and NOT the next header matching |key|.

(Somehow I read it as the former until seeing the implementation).

http://codereview.chromium.org/1604011/diff/1/14
File net/http/http_request_info.h (left):

http://codereview.chromium.org/1604011/diff/1/14#oldcode32
net/http/http_request_info.h:32: std::string user_agent;
TODO(eroman): I will have to review this difference more carefully; I am not
sure if it is going to have a behavioral impact. (although I am definitely in
favor of it).

http://codereview.chromium.org/1604011/diff/1/14
File net/http/http_request_info.h (right):

http://codereview.chromium.org/1604011/diff/1/14#newcode32
net/http/http_request_info.h:32: HttpRequestHeaders extra_headers;
It seems like a problem to me that |extra_headers| can also define the "method"
and "path", since this introduces some ambiguity and possibility for bugs.

Perhaps we can get rid of the the request line from HttpRequestHeaders?

(My other concern was how HttpRequestHeaders::IsEmpty() checks for those fields
too, see comment http://codereview.chromium.org/1604011/diff/1/24#newcode61).

http://codereview.chromium.org/1604011/diff/1/21
File net/http/http_vary_data.cc (right):

http://codereview.chromium.org/1604011/diff/1/21#newcode101
net/http/http_vary_data.cc:101: if (LowerCaseEqualsASCII(request_header,
"referer"))
same comment as below. (although perhaps this is best left to a post-cleanup.)

http://codereview.chromium.org/1604011/diff/1/21#newcode103
net/http/http_vary_data.cc:103: if (LowerCaseEqualsASCII(request_header,
"user-agent")) {
Can you use a constant from HttpRequestHeaders for this?

http://codereview.chromium.org/1604011/diff/1/24
File net/http/partial_data.cc (right):

http://codereview.chromium.org/1604011/diff/1/24#newcode25
net/http/partial_data.cc:25: HttpRequestHeaders* headers) {
indentation.

http://codereview.chromium.org/1604011/diff/1/24#newcode26
net/http/partial_data.cc:26: DCHECK(start >= 0 || end >= 0);
While you are modifying this, perhaps break this into two checks.
DCHECK_GE(start, 0);
DCHECK_GE(end, 0);

http://codereview.chromium.org/1604011/diff/1/24#newcode42
net/http/partial_data.cc:42: headers.GetHeader(HttpRequestHeaders::kRange);
Now that I have seen a number of these GetHeader() patterns in the code, I don't
much like the user of pointers.

What about:

std::string range_header_value;
if (!headers.GetHeader(HttpRequestHeaders::kRange, &range_header_value)
  return false;

http://codereview.chromium.org/1604011/diff/1/24#newcode61
net/http/partial_data.cc:61: DCHECK(extra_headers_.IsEmpty());
I will have to look at this more carefully, but I am a bit worried about the way
"empty" was defined to include both the header lines, and the method/version
etc...

Before this change, "empty" corresponded to just the extra headers, whereas now
it means something more. Hopefully that isn't being hit anywhere the translation
occurred.

http://codereview.chromium.org/1604011/diff/1/24#newcode110
net/http/partial_data.cc:110: headers->MergeFrom(extra_headers_);
I have seen this Clear() + MergeFrom() pattern used in several places. Consider
adding some Assign method, or even

*headers = extra_headers_;

(since I think you support the operator=.)

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

http://codereview.chromium.org/1604011/diff/1/25#newcode40
net/http/partial_data.h:40: // Performs initialization of the object by parsing
the request |headers|
"parsing" probably isn't the right word anymore.

http://codereview.chromium.org/1604011/diff/1/26
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1604011/diff/1/26#newcode1231
net/spdy/spdy_network_transaction_unittest.cc:1231: const char* header_key =
test_cases[i].extra_headers[0][ct *2];
Huh?
TODO(eroman): read this more carefully.

http://codereview.chromium.org/1604011/diff/1/26#newcode1232
net/spdy/spdy_network_transaction_unittest.cc:1232: const char* header_value =
test_cases[i].extra_headers[0][ct *2 + 1];
nit: add space between *2.

http://codereview.chromium.org/1604011/diff/1/27
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/1604011/diff/1/27#newcode135
net/spdy/spdy_session.cc:135: // TODO(willchan): It's not really necessarily to
convert from
necessarily --> necessary

http://codereview.chromium.org/1604011/diff/1/29
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/1604011/diff/1/29#newcode339
net/url_request/url_request_http_job.cc:339:
request_info_.extra_headers.RemoveHeader(
What if there are multiple "cookie" headers?

RemoveHeader is only going to remove the first, whereas before it would remove
all of them.

I think we need to think a little harder about how to deal with duplicate
headers. Maybe never allow them to begin with (coalesce with ','), and avoid a
lot of these questions.

[willchan no longer on Chromium, 2010-04-05 18:35:25]

I think I've addressed almost all your comments.  I haven't analyzed the
GetHeader() comments yet.  I'll look into seeing if it makes sense to change the
function prototype and renaming the function.

http://codereview.chromium.org/1604011/diff/1/7
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/1604011/diff/1/7#newcode84
net/http/http_cache_transaction.cc:84: const std::string* header_value =
headers.GetHeader(search->name);
On 2010/04/04 22:14:42, eroman wrote:
> The inner loop is now less efficient than in the original approach. Before it
> was just iterating through all the headers once, whereas now it calls
> GetHeader() each time, and GetHeader() is an O(n) function.
> 
> I wander if it wouldn't be better to preserve the original flow, and use
> HttpRequestHeaders::Iterator.
> 
> Or, could optimize HttpRequestHeaders::GetHeader() to be O(1) (especially
since
> it is pretty common to call Has/Get).
> 
> Overall though, I still like your new implementation, since you avoid
> constructing the |new_headers| string.

Is the overall implementation less fast though?  The old implementation was:

for each http header:
  compare with each kSpecialHeader
  compare with each kValidationHeader

My new implementation is:
for each kSpecialHeader
  search for matching http header
for each kValidationHeader
  search for matching http header

It seems to have the same big O time.  Perhaps I missed something.

http://codereview.chromium.org/1604011/diff/1/7#newcode91
net/http/http_cache_transaction.cc:91: HttpUtil::ValuesIterator
v(header_value->begin(), header_value->end(),',');
On 2010/04/04 22:14:42, eroman wrote:
> nit: please add a space after the comma.

Done.

http://codereview.chromium.org/1604011/diff/1/7#newcode1507
net/http/http_cache_transaction.cc:1507:
custom_request_->extra_headers.SetHeader("If-Range", etag_value);
On 2010/04/04 22:14:42, eroman wrote:
> nit: below you use the constant kIfRange instead of "If-Range". Please use one
> or the other approach throughout.

Done.

http://codereview.chromium.org/1604011/diff/1/7#newcode1509
net/http/http_cache_transaction.cc:1509:
custom_request_->extra_headers.SetHeader("If-None-Match", etag_value);
On 2010/04/04 22:14:42, eroman wrote:
> see comment above.

Done.

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

http://codereview.chromium.org/1604011/diff/1/8#newcode816
net/http/http_cache_unittest.cc:816: // Returns true if the response headers
(|response|) match a partial content
On 2010/04/04 22:14:42, eroman wrote:
> "Returns" is no longer applicable.

Done.

http://codereview.chromium.org/1604011/diff/1/12
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/1604011/diff/1/12#newcode17
net/http/http_request_headers.cc:17: const char
HttpRequestHeaders::kAvailDictionary[] = "Avail-Dictionary: ";
On 2010/04/04 22:14:42, eroman wrote:
> Hm, I dunno that I like this.
> 
> Avail-Dictionary is a header from SDCH, seems wrong to enumerate it under the
> generic HTTP headers.

Done.

http://codereview.chromium.org/1604011/diff/1/12#newcode86
net/http/http_request_headers.cc:86: return out;
On 2010/04/04 22:14:42, eroman wrote:
> Not sure what the compiler is going to do here, but might be more efficient to
> make this an out parameter of the function.

This fulfills the conditions for NRVO
(http://www.efnetcpp.org/wiki/Return_value_optimization).  It won't have an
unnecessary copy.

http://codereview.chromium.org/1604011/diff/1/12#newcode216
net/http/http_request_headers.cc:216: if (!base::strncasecmp(key.data(),
it->key.data(), key.length()))
On 2010/04/04 22:14:42, eroman wrote:
> Isn't this the same bug you had in an earlier revision?
> ... strncasecmp is a substring check, so must check the length of |key| as
well.

Sigh, yes.  I forgot to update this when I fixed the other one (was working on
this in a parallel git branch, so I forgot to port this fix when I fixed it in
the other branch).  I've updated the unittest to catch this case.

http://codereview.chromium.org/1604011/diff/1/13
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/1604011/diff/1/13#newcode138
net/http/http_request_headers.h:138: HeaderVector::iterator FindHeader(const
base::StringPiece& key);
On 2010/04/04 22:14:42, eroman wrote:
> It is worth documenting that this returns the iterator to the header
*following*
> the one found, and NOT the next header matching |key|.
> 
> (Somehow I read it as the former until seeing the implementation).

I don't understand this comment.  AFAICT, this has the same semantics as
std::find().

http://codereview.chromium.org/1604011/diff/1/14
File net/http/http_request_info.h (right):

http://codereview.chromium.org/1604011/diff/1/14#newcode32
net/http/http_request_info.h:32: HttpRequestHeaders extra_headers;
On 2010/04/04 22:14:42, eroman wrote:
> It seems like a problem to me that |extra_headers| can also define the
"method"
> and "path", since this introduces some ambiguity and possibility for bugs.
> 
> Perhaps we can get rid of the the request line from HttpRequestHeaders?
> 
> (My other concern was how HttpRequestHeaders::IsEmpty() checks for those
fields
> too, see comment http://codereview.chromium.org/1604011/diff/1/24#newcode61).

Done.

I had intended to mitigate this with DCHECKs in MergeFrom(), but I guess that
removing the special case from HttpRequestHeaders is indeed the cleanest
solution, since the special case is only used in HttpNetworkTransaction.

http://codereview.chromium.org/1604011/diff/1/21
File net/http/http_vary_data.cc (right):

http://codereview.chromium.org/1604011/diff/1/21#newcode101
net/http/http_vary_data.cc:101: if (LowerCaseEqualsASCII(request_header,
"referer"))
On 2010/04/04 22:14:42, eroman wrote:
> same comment as below. (although perhaps this is best left to a post-cleanup.)

Done.

http://codereview.chromium.org/1604011/diff/1/21#newcode103
net/http/http_vary_data.cc:103: if (LowerCaseEqualsASCII(request_header,
"user-agent")) {
On 2010/04/04 22:14:42, eroman wrote:
> Can you use a constant from HttpRequestHeaders for this?

Done.

http://codereview.chromium.org/1604011/diff/1/24
File net/http/partial_data.cc (right):

http://codereview.chromium.org/1604011/diff/1/24#newcode25
net/http/partial_data.cc:25: HttpRequestHeaders* headers) {
On 2010/04/04 22:14:42, eroman wrote:
> indentation.

Done.

http://codereview.chromium.org/1604011/diff/1/24#newcode26
net/http/partial_data.cc:26: DCHECK(start >= 0 || end >= 0);
On 2010/04/04 22:14:42, eroman wrote:
> While you are modifying this, perhaps break this into two checks.
> DCHECK_GE(start, 0);
> DCHECK_GE(end, 0);

This DCHECK is an ||, not an &&.  Is the && statement more valid?  If so, I can
break this as you say.

http://codereview.chromium.org/1604011/diff/1/24#newcode61
net/http/partial_data.cc:61: DCHECK(extra_headers_.IsEmpty());
On 2010/04/04 22:14:42, eroman wrote:
> I will have to look at this more carefully, but I am a bit worried about the
way
> "empty" was defined to include both the header lines, and the method/version
> etc...
> 
> Before this change, "empty" corresponded to just the extra headers, whereas
now
> it means something more. Hopefully that isn't being hit anywhere the
translation
> occurred.

Done.

http://codereview.chromium.org/1604011/diff/1/24#newcode110
net/http/partial_data.cc:110: headers->MergeFrom(extra_headers_);
On 2010/04/04 22:14:42, eroman wrote:
> I have seen this Clear() + MergeFrom() pattern used in several places.
Consider
> adding some Assign method, or even
> 
> *headers = extra_headers_;
> 
> (since I think you support the operator=.)

I'm using CopyFrom() now.  Let me know if you see any places where I should
switch to using CopyFrom().  I created CopyFrom() since I don't want to use
operator= in general.

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

http://codereview.chromium.org/1604011/diff/1/25#newcode40
net/http/partial_data.h:40: // Performs initialization of the object by parsing
the request |headers|
On 2010/04/04 22:14:42, eroman wrote:
> "parsing" probably isn't the right word anymore.

Done.

http://codereview.chromium.org/1604011/diff/1/26
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1604011/diff/1/26#newcode1232
net/spdy/spdy_network_transaction_unittest.cc:1232: const char* header_value =
test_cases[i].extra_headers[0][ct *2 + 1];
On 2010/04/04 22:14:42, eroman wrote:
> nit: add space between *2.

Done.

http://codereview.chromium.org/1604011/diff/1/27
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/1604011/diff/1/27#newcode135
net/spdy/spdy_session.cc:135: // TODO(willchan): It's not really necessarily to
convert from
On 2010/04/04 22:14:42, eroman wrote:
> necessarily --> necessary

Done.

http://codereview.chromium.org/1604011/diff/1/29
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/1604011/diff/1/29#newcode339
net/url_request/url_request_http_job.cc:339:
request_info_.extra_headers.RemoveHeader(
On 2010/04/04 22:14:42, eroman wrote:
> What if there are multiple "cookie" headers?
> 
> RemoveHeader is only going to remove the first, whereas before it would remove
> all of them.
> 
> I think we need to think a little harder about how to deal with duplicate
> headers. Maybe never allow them to begin with (coalesce with ','), and avoid a
> lot of these questions.

Didn't we discuss this that we aren't allowing duplicate headers?  We examined
webkit and they use a map, so unless we're setting headers multiple times in
chrome, then it's not a problem.  I guess it's possible that we could do it with
the cookie code (I'd have to double check).  I do think we should never allow
repeated request headers in chrome (we should always comma coalesce).

[willchan no longer on Chromium, 2010-04-05 20:28:43]

Updated GetHeader() now.

http://codereview.chromium.org/1604011/diff/1/13
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/1604011/diff/1/13#newcode93
net/http/http_request_headers.h:93: const std::string* GetHeader(const
base::StringPiece& key) const;
On 2010/04/04 22:14:42, eroman wrote:
> I think this should be called GetFirstHeader() to emphasize that there may be
> duplicates.
> 
> Even better, why not just expose FindNextHeader() instead? IMO that API is
> cleaner and more powerful. It also seems good to let the consumer see the
> header's key, which may be different case than what they searched.

I kept it called GetHeader().  I did switch it up to take a return value of bool
and use an out parameter.

I don't want to have GetFirstHeader() nor expose FindNextHeader().  I don't want
to make it known that the header can be set multiple times.  I think we want to
make it so we only ever set it once.  I'd like the key to only exist once in the
vector.  I'd like to remove the GetHeaders() API eventually.  I don't know that
it's useful for the consumer to see the header's key, since it's not supposed to
matter.  Code that depends on case is broken, so I'd prefer to somewhat restrict
the consumer's ability to screw this up (although it's obviously not perfect).

http://codereview.chromium.org/1604011/diff/1/13#newcode95
net/http/http_request_headers.h:95: // Gets the first header that matches |key|.
 Returns NULL if not found.
On 2010/04/04 22:14:42, eroman wrote:
> This comment is wrong.

Done.

[eroman, 2010-04-06 05:49:54]

lgtm

http://codereview.chromium.org/1604011/diff/1/7
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/1604011/diff/1/7#newcode84
net/http/http_cache_transaction.cc:84: const std::string* header_value =
headers.GetHeader(search->name);
On 2010/04/05 18:35:25, willchan wrote:
> On 2010/04/04 22:14:42, eroman wrote:
> > The inner loop is now less efficient than in the original approach. Before
it
> > was just iterating through all the headers once, whereas now it calls
> > GetHeader() each time, and GetHeader() is an O(n) function.
> > 
> > I wander if it wouldn't be better to preserve the original flow, and use
> > HttpRequestHeaders::Iterator.
> > 
> > Or, could optimize HttpRequestHeaders::GetHeader() to be O(1) (especially
> since
> > it is pretty common to call Has/Get).
> > 
> > Overall though, I still like your new implementation, since you avoid
> > constructing the |new_headers| string.
> 
> Is the overall implementation less fast though?  The old implementation was:
> 
> for each http header:
>   compare with each kSpecialHeader
>   compare with each kValidationHeader
> 
> My new implementation is:
> for each kSpecialHeader
>   search for matching http header
> for each kValidationHeader
>   search for matching http header
> 
> It seems to have the same big O time.  Perhaps I missed something.

Yeah you are right, dunno what i was thinking!

http://codereview.chromium.org/1604011/diff/1/12
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/1604011/diff/1/12#newcode216
net/http/http_request_headers.cc:216: if (!base::strncasecmp(key.data(),
it->key.data(), key.length()))
On 2010/04/05 18:35:25, willchan wrote:
> On 2010/04/04 22:14:42, eroman wrote:
> > Isn't this the same bug you had in an earlier revision?
> > ... strncasecmp is a substring check, so must check the length of |key| as
> well.
> 
> Sigh, yes.  I forgot to update this when I fixed the other one (was working on
> this in a parallel git branch, so I forgot to port this fix when I fixed it in
> the other branch).  I've updated the unittest to catch this case.

Where is the unittest?

http://codereview.chromium.org/1604011/diff/1/13
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/1604011/diff/1/13#newcode93
net/http/http_request_headers.h:93: const std::string* GetHeader(const
base::StringPiece& key) const;
On 2010/04/05 20:28:44, willchan wrote:
> On 2010/04/04 22:14:42, eroman wrote:
> > I think this should be called GetFirstHeader() to emphasize that there may
be
> > duplicates.
> > 
> > Even better, why not just expose FindNextHeader() instead? IMO that API is
> > cleaner and more powerful. It also seems good to let the consumer see the
> > header's key, which may be different case than what they searched.
> 
> I kept it called GetHeader().  I did switch it up to take a return value of
bool
> and use an out parameter.
> 
> I don't want to have GetFirstHeader() nor expose FindNextHeader().  I don't
want
> to make it known that the header can be set multiple times.  I think we want
to
> make it so we only ever set it once.  I'd like the key to only exist once in
the
> vector.  I'd like to remove the GetHeaders() API eventually.  I don't know
that
> it's useful for the consumer to see the header's key, since it's not supposed
to
> matter.  Code that depends on case is broken, so I'd prefer to somewhat
restrict
> the consumer's ability to screw this up (although it's obviously not perfect).

That sounds reasonable to me.

http://codereview.chromium.org/1604011/diff/1/14
File net/http/http_request_info.h (right):

http://codereview.chromium.org/1604011/diff/1/14#newcode32
net/http/http_request_info.h:32: HttpRequestHeaders extra_headers;
On 2010/04/05 18:35:25, willchan wrote:
> On 2010/04/04 22:14:42, eroman wrote:
> > It seems like a problem to me that |extra_headers| can also define the
> "method"
> > and "path", since this introduces some ambiguity and possibility for bugs.
> > 
> > Perhaps we can get rid of the the request line from HttpRequestHeaders?
> > 
> > (My other concern was how HttpRequestHeaders::IsEmpty() checks for those
> fields
> > too, see comment
http://codereview.chromium.org/1604011/diff/1/24#newcode61).
> 
> Done.
> 
> I had intended to mitigate this with DCHECKs in MergeFrom(), but I guess that
> removing the special case from HttpRequestHeaders is indeed the cleanest
> solution, since the special case is only used in HttpNetworkTransaction.

Thanks. I like this better.

http://codereview.chromium.org/1604011/diff/1/24
File net/http/partial_data.cc (right):

http://codereview.chromium.org/1604011/diff/1/24#newcode26
net/http/partial_data.cc:26: DCHECK(start >= 0 || end >= 0);
On 2010/04/05 18:35:25, willchan wrote:
> On 2010/04/04 22:14:42, eroman wrote:
> > While you are modifying this, perhaps break this into two checks.
> > DCHECK_GE(start, 0);
> > DCHECK_GE(end, 0);
> 
> This DCHECK is an ||, not an &&.  Is the && statement more valid?  If so, I
can
> break this as you say.

You are right, I misread that.

http://codereview.chromium.org/1604011/diff/1/29
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/1604011/diff/1/29#newcode339
net/url_request/url_request_http_job.cc:339:
request_info_.extra_headers.RemoveHeader(
On 2010/04/05 18:35:25, willchan wrote:
> On 2010/04/04 22:14:42, eroman wrote:
> > What if there are multiple "cookie" headers?
> > 
> > RemoveHeader is only going to remove the first, whereas before it would
remove
> > all of them.
> > 
> > I think we need to think a little harder about how to deal with duplicate
> > headers. Maybe never allow them to begin with (coalesce with ','), and avoid
a
> > lot of these questions.
> 
> Didn't we discuss this that we aren't allowing duplicate headers?  We examined
> webkit and they use a map, so unless we're setting headers multiple times in
> chrome, then it's not a problem.  I guess it's possible that we could do it
with
> the cookie code (I'd have to double check).  I do think we should never allow
> repeated request headers in chrome (we should always comma coalesce).

Given that HttpRequestHeaders supports adding duplicate headers, I think it is
prudent to assume that it might have happened.

Would you be cool with adding a
CHECK(!request_info_.extra_headers.HasHeader(kCookie)) ?

http://codereview.chromium.org/1604011/diff/16001/17007
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/1604011/diff/16001/17007#newcode752
net/http/http_cache_unittest.cc:752: net::HttpRequestHeaders::kRange,
&range_header) ||
nit: this indentation looks like it should get some more.

http://codereview.chromium.org/1604011/diff/16001/17009
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1604011/diff/16001/17009#newcode940
net/http/http_network_transaction.cc:940: request_headers_ =
StringPrintf("%s%s", request_line.c_str(),
seems like this would be simpler as |request_line + request_headers.ToString()|

http://codereview.chromium.org/1604011/diff/16001/17012
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/1604011/diff/16001/17012#newcode5
net/http/http_request_headers.h:5: // HttpRequestHeaders manages the request
headers (including the request line).
Remove the comment about "request line"

http://codereview.chromium.org/1604011/diff/16001/17012#newcode88
net/http/http_request_headers.h:88: // Gets the first header that matches |key|.
 Returns NULL if not found.
This comment is still wrong...

http://codereview.chromium.org/1604011/diff/16001/17012#newcode92
net/http/http_request_headers.h:92: // Clears all the headers, including the
request line.
This comment needs to be updated.

http://codereview.chromium.org/1604011/diff/16001/17013
File net/http/http_request_headers_unittest.cc (right):

http://codereview.chromium.org/1604011/diff/16001/17013#newcode46
net/http/http_request_headers_unittest.cc:46: const HttpRequestHeaders&
headers_ref = headers;
Why this extra variable?

[rvargas (doing something else), 2010-04-07 21:50:17]

LGTM.
Http-cache-unittest.cc(2373): nit. It looks better with "\r\n" added to the
previous line.

Http-cache-unittest.cc(767): removing this check makes me realize my only
concern with this change: having two if-range headers is a valid use case
(although we don't do it), and that cannot be coalesced into a single header
with commas. There could be more cases like that... and right now we are not
merging the headers in SetHeader, just replacing the previous value so we are
removing a test case that was worth having somewhere. Do we use the code that
replaces a value or can we just DCHECK there?.

Also, make sure you perform some manual tests with PDFs to make sure they still
work :)

[willchan no longer on Chromium, 2010-04-07 22:01:52]

On 2010/04/07 21:50:17, rvargas wrote:
> LGTM.
> Http-cache-unittest.cc(2373): nit. It looks better with "\r\n" added to the
> previous line.
> 
> Http-cache-unittest.cc(767): removing this check makes me realize my only
> concern with this change: having two if-range headers is a valid use case
> (although we don't do it), and that cannot be coalesced into a single header
> with commas. There could be more cases like that... and right now we are not
> merging the headers in SetHeader, just replacing the previous value so we are
> removing a test case that was worth having somewhere. Do we use the code that
> replaces a value or can we just DCHECK there?.

We don't use it, although it's unclear whether or not WebKit may set a header we
already have, via MergeFrom(extra_headers).  I think it's more reasonable to
DCHECK that chrome does not set the any header multiple times, but I don't know
if it's reasonable to force WebKit not to collide with Chrome's headers.

> 
> Also, make sure you perform some manual tests with PDFs to make sure they
still
> work :)

Er, I'm not clear on what this would entail.  What manual tests would you like
me to run?  Just downloading a pdf?  Is there anything windows specific, or can
I test on a Linux box?

[rvargas1, 2010-04-07 23:58:19]

Just make sure that the PDF plugin works as usual... displaying pdfs instead
of downloading them... we have code that inserts range headers on behalf of
Acrobat.

On Apr 7, 2010 5:02 PM, <willchan@chromium.org> wrote:

On 2010/04/07 21:50:17, rvargas wrote:
>
> LGTM.
> Http-cache-unittest.cc(2373): nit. It looks bette...
We don't use it, although it's unclear whether or not WebKit may set a
header we
already have, via MergeFrom(extra_headers).  I think it's more reasonable to
DCHECK that chrome does not set the any header multiple times, but I don't
know
if it's reasonable to force WebKit not to collide with Chrome's headers.




> Also, make sure you perform some manual tests with PDFs to make sure they

still
>
> work :)

Er, I'm not clear on what this would entail.  What manual tests would you
like
me to run?  Just downloading a pdf?  Is there anything windows specific, or
can
I test on a Linux box?



http://codereview.chromium.org/1604011/show

[willchan no longer on Chromium, 2010-04-08 00:10:41]

On Wed, Apr 7, 2010 at 2:50 PM,  <rvargas@chromium.org> wrote:
> LGTM.
> Http-cache-unittest.cc(2373): nit. It looks better with "\r\n" added to the
> previous line.
>
> Http-cache-unittest.cc(767): removing this check makes me realize my only
> concern with this change: having two if-range headers is a valid use case
> (although we don't do it), and that cannot be coalesced into a single header
> with commas. There could be more cases like that... and right now we are not

I'm afraid I don't fully understand If-Range.  Doesn't the HTTP spec
say that message headers with duplicate field-names are not allowed
unless they can be coalesced?  Here's the text from the spec:

"Multiple message-header fields with the same field-name MAY be
present in a message if and only if the entire field-value for that
header field is defined as a comma-separated list [i.e., #(values)].
It MUST be possible to combine the multiple header fields into one
"field-name: field-value" pair, without changing the semantics of the
message, by appending each subsequent field-value to the first, each
separated by a comma. The order in which header fields with the same
field-name are received is therefore significant to the interpretation
of the combined field value, and thus a proxy MUST NOT change the
order of these field values when a message is forwarded."

> merging the headers in SetHeader, just replacing the previous value so we
> are
> removing a test case that was worth having somewhere. Do we use the code
> that
> replaces a value or can we just DCHECK there?.
>
> Also, make sure you perform some manual tests with PDFs to make sure they
> still
> work :)
>
> http://codereview.chromium.org/1604011/show
>

[rvargas1, 2010-04-08 00:32:08]

That basically answers the question about other headers. There are two types
of values for an if-range header and the server can tell them apart by
inspection so maybe they can be coalesced, but I'd say that would be
weird... in any case we don't want to do that so it looks like there is no
issue. I still think that a DCHECK in SetHeader would be the best thing to
do.

On Apr 7, 2010 7:10 PM, "William Chan (陈智昌)" <willchan@chromium.org> wrote:

On Wed, Apr 7, 2010 at 2:50 PM, <rvargas@chromium.org> wrote:
> LGTM.
> Http-cache-unittest.cc(2373...
I'm afraid I don't fully understand If-Range.  Doesn't the HTTP spec
say that message headers with duplicate field-names are not allowed
unless they can be coalesced?  Here's the text from the spec:

"Multiple message-header fields with the same field-name MAY be
present in a message if and only if the entire field-value for that
header field is defined as a comma-separated list [i.e., #(values)].
It MUST be possible to combine the multiple header fields into one
"field-name: field-value" pair, without changing the semantics of the
message, by appending each subsequent field-value to the first, each
separated by a comma. The order in which header fields with the same
field-name are received is therefore significant to the interpretation
of the combined field value, and thus a proxy MUST NOT change the
order of these field values when a message is forwarded."


> merging the headers in SetHeader, just replacing the previous value so we
> are
> removing a test...

[willchan no longer on Chromium, 2010-04-08 07:31:32]

Oops, I had dropped everyone else.  I've added them back.  Eric,
please chime in if you have an opinion.  I'll think some more about
this and reply tomorrow.

2010/4/8 Ricardo Vargas <rvargas@google.com>:
> I basically agree with everything you said.
> Maybe we can have a different method to replace a header, and another one to
> append a value at the end (plus the current set+dcheck). That way the caller
> can use the right one without having to split the headers in two: from net
> it doesn't matter who set a header, but usually there is a separate code
> path for a header already present.
> On second thought, we could have only two methods and require the use of
> remove+set to replace a header... at least from net I don't think we should
> replace something set by the caller unless we do it intentionally.
>
> On Apr 7, 2010 7:57 PM, "William Chan (陈智昌)" <willchan@chromium.org> wrote:
>
> On Wed, Apr 7, 2010 at 5:31 PM, Ricardo Vargas <rvargas@google.com> wrote:
>> That basically answers ...
>
> Hm, I'm trying to think what the right behavior is here.  An XHR could
> set any header in WebKit.  That would get passed to the RDH, which
> would set the extra request headers.  Then chrome code would call
> SetHeader().  What's the right behavior here?
>
> My instinct is, headers set by Chrome should be canonical.  Chrome
> should not be setting the same header multiple times (we should DCHECK
> on this).  Chrome headers should overwrite WebKit headers (not DCHECK,
> just overwrite and perhaps LOG(WARNING)).  Implementing this will
> require splitting out extra_headers into two separate fields, so it's
> clear which one is being set by Chrome and which is set by WebKit.  I
> note that SetExtraRequestHeaders() is called not just for WebKit
> generated headers, but also plugin and browser sync and what not, so
> perhaps I should just draw the distinction at chrome network stack
> generated headers and outside generated headers.  I think the addition
> of DCHECKs should be done in a follow up changelist, since that will
> require more refactoring of extra_headers (the splitting of it into
> two fields).
>
> Thoughts?
>
>>
>> On Apr 7, 2010 7:10 PM, "William Chan (陈智昌)" <willchan@chromium.org>
>> wrote:
>>
>> On Wed, Apr 7, ...

[willchan no longer on Chromium, 2010-04-16 00:10:36]

On 2010/04/08 07:31:32, willchan wrote:
> Oops, I had dropped everyone else.  I've added them back.  Eric,
> please chime in if you have an opinion.  I'll think some more about
> this and reply tomorrow.
> 
> 2010/4/8 Ricardo Vargas <rvargas@google.com>:
> > I basically agree with everything you said.
> > Maybe we can have a different method to replace a header, and another one to
> > append a value at the end (plus the current set+dcheck). That way the caller
> > can use the right one without having to split the headers in two: from net
> > it doesn't matter who set a header, but usually there is a separate code
> > path for a header already present.
> > On second thought, we could have only two methods and require the use of
> > remove+set to replace a header... at least from net I don't think we should
> > replace something set by the caller unless we do it intentionally.

I'm not sure either of these methods work unless you have two separate
HttpRequestHeaders instances.  Let me know if I'm missing something.  What I'm
thinking is that, if we keep the extra_headers field as our working copy, as
opposed to being a non_network_stack_set_headers, then as we update
extra_headers with SetHeader() and friends, we don't know if we are
deleting/overwriting/appending to a header set within the network stack as
opposed to one outside the network stack.  This is because, as we move later on
through the network stack code, we won't know, for a given header in
extra_headers, whether or not it was set by the net/ or non-net/ code.

I think what we need to do is:
Keep extra_headers as the HttpRequestHeaders as we received them at the start of
the URLRequest.  Never modify this within net/ except to call RemoveHeader() as
needed.
Add a field to URLRequest that is for net_headers.  Use that within net/.  Call
SetHeader() as needed.  SetHeader(header) should DCHECK(!HasHeader(header)), and
overwrite.  At the end, we do net_headers.MergeFrom(extra_headers);  We change
the semantics of MergeFrom() such that it does not set a header that already
exists, but does LOG(WARNING).  Then we do any final filtering of net_headers
headers (see kExtraHeadersToBeStripped in http_network_transaction.cc) and then
serialize to a string and send to the user.

I think that if we use two fields, we can meet the goals I listed earlier
regarding non-net headers and net headers.  It also seems it is unnecessary to
add any more member functions to HttpRequestHeaders(), although we may need to
adjust the semantics for MergeFrom().  Let me know what you think.

> >
> > On Apr 7, 2010 7:57 PM, "William Chan (陈智昌)" <mailto:willchan@chromium.org>
wrote:
> >
> > On Wed, Apr 7, 2010 at 5:31 PM, Ricardo Vargas <mailto:rvargas@google.com>
wrote:
> >> That basically answers ...
> >
> > Hm, I'm trying to think what the right behavior is here.  An XHR could
> > set any header in WebKit.  That would get passed to the RDH, which
> > would set the extra request headers.  Then chrome code would call
> > SetHeader().  What's the right behavior here?
> >
> > My instinct is, headers set by Chrome should be canonical.  Chrome
> > should not be setting the same header multiple times (we should DCHECK
> > on this).  Chrome headers should overwrite WebKit headers (not DCHECK,
> > just overwrite and perhaps LOG(WARNING)).  Implementing this will
> > require splitting out extra_headers into two separate fields, so it's
> > clear which one is being set by Chrome and which is set by WebKit.  I
> > note that SetExtraRequestHeaders() is called not just for WebKit
> > generated headers, but also plugin and browser sync and what not, so
> > perhaps I should just draw the distinction at chrome network stack
> > generated headers and outside generated headers.  I think the addition
> > of DCHECKs should be done in a follow up changelist, since that will
> > require more refactoring of extra_headers (the splitting of it into
> > two fields).
> >
> > Thoughts?
> >
> >>
> >> On Apr 7, 2010 7:10 PM, "William Chan (陈智昌)" <mailto:willchan@chromium.org>
> >> wrote:
> >>
> >> On Wed, Apr 7, ...
>

[willchan no longer on Chromium, 2010-04-16 00:10:55]

http://codereview.chromium.org/1604011/diff/1/12
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/1604011/diff/1/12#newcode216
net/http/http_request_headers.cc:216: if (!base::strncasecmp(key.data(),
it->key.data(), key.length()))
On 2010/04/06 05:49:54, eroman wrote:
> On 2010/04/05 18:35:25, willchan wrote:
> > On 2010/04/04 22:14:42, eroman wrote:
> > > Isn't this the same bug you had in an earlier revision?
> > > ... strncasecmp is a substring check, so must check the length of |key| as
> > well.
> > 
> > Sigh, yes.  I forgot to update this when I fixed the other one (was working
on
> > this in a parallel git branch, so I forgot to port this fix when I fixed it
in
> > the other branch).  I've updated the unittest to catch this case.
> 
> Where is the unittest?

I'm moving it to HasHeader now.

http://codereview.chromium.org/1604011/diff/16001/17007
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/1604011/diff/16001/17007#newcode752
net/http/http_cache_unittest.cc:752: net::HttpRequestHeaders::kRange,
&range_header) ||
On 2010/04/06 05:49:54, eroman wrote:
> nit: this indentation looks like it should get some more.

Er, where?

http://codereview.chromium.org/1604011/diff/16001/17009
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1604011/diff/16001/17009#newcode940
net/http/http_network_transaction.cc:940: request_headers_ =
StringPrintf("%s%s", request_line.c_str(),
On 2010/04/06 05:49:54, eroman wrote:
> seems like this would be simpler as |request_line +
request_headers.ToString()|

Done.

http://codereview.chromium.org/1604011/diff/16001/17012
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/1604011/diff/16001/17012#newcode5
net/http/http_request_headers.h:5: // HttpRequestHeaders manages the request
headers (including the request line).
On 2010/04/06 05:49:54, eroman wrote:
> Remove the comment about "request line"

Done.

http://codereview.chromium.org/1604011/diff/16001/17012#newcode88
net/http/http_request_headers.h:88: // Gets the first header that matches |key|.
 Returns NULL if not found.
On 2010/04/06 05:49:54, eroman wrote:
> This comment is still wrong...

oops, i forgot to delete that code.  gone now.

http://codereview.chromium.org/1604011/diff/16001/17012#newcode92
net/http/http_request_headers.h:92: // Clears all the headers, including the
request line.
On 2010/04/06 05:49:54, eroman wrote:
> This comment needs to be updated.

Done.

http://codereview.chromium.org/1604011/diff/16001/17013
File net/http/http_request_headers_unittest.cc (right):

http://codereview.chromium.org/1604011/diff/16001/17013#newcode46
net/http/http_request_headers_unittest.cc:46: const HttpRequestHeaders&
headers_ref = headers;
On 2010/04/06 05:49:54, eroman wrote:
> Why this extra variable?

I thought this was testing the const pathway, but it turns out I was completely
on crack.

[rvargas (doing something else), 2010-04-16 00:54:53]

On 2010/04/16 00:10:36, willchan wrote:
> On 2010/04/08 07:31:32, willchan wrote:
> > Oops, I had dropped everyone else.  I've added them back.  Eric,
> > please chime in if you have an opinion.  I'll think some more about
> > this and reply tomorrow.
> > 
> > 2010/4/8 Ricardo Vargas <rvargas@google.com>:
> > > I basically agree with everything you said.
> > > Maybe we can have a different method to replace a header, and another one
to
> > > append a value at the end (plus the current set+dcheck). That way the
caller
> > > can use the right one without having to split the headers in two: from net
> > > it doesn't matter who set a header, but usually there is a separate code
> > > path for a header already present.
> > > On second thought, we could have only two methods and require the use of
> > > remove+set to replace a header... at least from net I don't think we
should
> > > replace something set by the caller unless we do it intentionally.
> 
> I'm not sure either of these methods work unless you have two separate
> HttpRequestHeaders instances.  Let me know if I'm missing something.  What I'm
> thinking is that, if we keep the extra_headers field as our working copy, as
> opposed to being a non_network_stack_set_headers, then as we update
> extra_headers with SetHeader() and friends, we don't know if we are
> deleting/overwriting/appending to a header set within the network stack as
> opposed to one outside the network stack.  This is because, as we move later
on
> through the network stack code, we won't know, for a given header in
> extra_headers, whether or not it was set by the net/ or non-net/ code.

We could have separate headers as you describe, but my point is that for a given
piece of code at layer x on the net stack:

1. If we are interested in a given header, we should see if that header is
already set by a higher layer (net/browser/renderer/plugin) and if the previous
setting is something that we want to override, or something that changes our
behavior somehow. In other words I don't see clearly why net_layer_x should care
if the header was set by net_layer_y or not.
2. If we decide to override, we do it explicitly
3. If we can keep both values we append to this header
4. If we are just setting the header, we'll DCHECK if the header was already
set.

If we keep two set of headers we still have the problem of deciding how to merge
them (do we take the user provided one or the one set by net?), and we are
probably making the decision at the wrong place (because the merger is not the
one who set the net header). I guess you mean that we take the user provided one
so override(2) is RemoveHeader() + SetHeader(), so the code looks almost the
same in both cases except that there is no way to do (3).

> 
> I think what we need to do is:
> Keep extra_headers as the HttpRequestHeaders as we received them at the start
of
> the URLRequest.  Never modify this within net/ except to call RemoveHeader()
as
> needed.
> Add a field to URLRequest that is for net_headers.  Use that within net/. 
Call
> SetHeader() as needed.  SetHeader(header) should DCHECK(!HasHeader(header)),
and
> overwrite.  At the end, we do net_headers.MergeFrom(extra_headers);  We change
> the semantics of MergeFrom() such that it does not set a header that already
> exists, but does LOG(WARNING).  Then we do any final filtering of net_headers
> headers (see kExtraHeadersToBeStripped in http_network_transaction.cc) and
then
> serialize to a string and send to the user.
> 
> I think that if we use two fields, we can meet the goals I listed earlier
> regarding non-net headers and net headers.  It also seems it is unnecessary to
> add any more member functions to HttpRequestHeaders(), although we may need to
> adjust the semantics for MergeFrom().  Let me know what you think.
> 
> > >
> > > On Apr 7, 2010 7:57 PM, "William Chan (陈智昌)"
<mailto:willchan@chromium.org>
> wrote:
> > >
> > > On Wed, Apr 7, 2010 at 5:31 PM, Ricardo Vargas <mailto:rvargas@google.com>
> wrote:
> > >> That basically answers ...
> > >
> > > Hm, I'm trying to think what the right behavior is here.  An XHR could
> > > set any header in WebKit.  That would get passed to the RDH, which
> > > would set the extra request headers.  Then chrome code would call
> > > SetHeader().  What's the right behavior here?
> > >
> > > My instinct is, headers set by Chrome should be canonical.  Chrome
> > > should not be setting the same header multiple times (we should DCHECK
> > > on this).  Chrome headers should overwrite WebKit headers (not DCHECK,
> > > just overwrite and perhaps LOG(WARNING)).  Implementing this will
> > > require splitting out extra_headers into two separate fields, so it's
> > > clear which one is being set by Chrome and which is set by WebKit.  I
> > > note that SetExtraRequestHeaders() is called not just for WebKit
> > > generated headers, but also plugin and browser sync and what not, so
> > > perhaps I should just draw the distinction at chrome network stack
> > > generated headers and outside generated headers.  I think the addition
> > > of DCHECKs should be done in a follow up changelist, since that will
> > > require more refactoring of extra_headers (the splitting of it into
> > > two fields).
> > >
> > > Thoughts?
> > >
> > >>
> > >> On Apr 7, 2010 7:10 PM, "William Chan (陈智昌)"
<mailto:willchan@chromium.org>
> > >> wrote:
> > >>
> > >> On Wed, Apr 7, ...
> >

[willchan no longer on Chromium, 2010-04-16 01:09:23]

On 2010/04/16 00:54:53, rvargas wrote:
> On 2010/04/16 00:10:36, willchan wrote:
> > On 2010/04/08 07:31:32, willchan wrote:
> > > Oops, I had dropped everyone else.  I've added them back.  Eric,
> > > please chime in if you have an opinion.  I'll think some more about
> > > this and reply tomorrow.
> > > 
> > > 2010/4/8 Ricardo Vargas <rvargas@google.com>:
> > > > I basically agree with everything you said.
> > > > Maybe we can have a different method to replace a header, and another
one
> to
> > > > append a value at the end (plus the current set+dcheck). That way the
> caller
> > > > can use the right one without having to split the headers in two: from
net
> > > > it doesn't matter who set a header, but usually there is a separate code
> > > > path for a header already present.
> > > > On second thought, we could have only two methods and require the use of
> > > > remove+set to replace a header... at least from net I don't think we
> should
> > > > replace something set by the caller unless we do it intentionally.
> > 
> > I'm not sure either of these methods work unless you have two separate
> > HttpRequestHeaders instances.  Let me know if I'm missing something.  What
I'm
> > thinking is that, if we keep the extra_headers field as our working copy, as
> > opposed to being a non_network_stack_set_headers, then as we update
> > extra_headers with SetHeader() and friends, we don't know if we are
> > deleting/overwriting/appending to a header set within the network stack as
> > opposed to one outside the network stack.  This is because, as we move later
> on
> > through the network stack code, we won't know, for a given header in
> > extra_headers, whether or not it was set by the net/ or non-net/ code.
> 
> We could have separate headers as you describe, but my point is that for a
given
> piece of code at layer x on the net stack:
> 
> 1. If we are interested in a given header, we should see if that header is
> already set by a higher layer (net/browser/renderer/plugin) and if the
previous
> setting is something that we want to override, or something that changes our
> behavior somehow. In other words I don't see clearly why net_layer_x should
care
> if the header was set by net_layer_y or not.
> 2. If we decide to override, we do it explicitly
> 3. If we can keep both values we append to this header
> 4. If we are just setting the header, we'll DCHECK if the header was already
> set.

I guess I don't see how 4 is ever valid, since it's possible for _any_ header to
already have been set by an XHR, so I don't think we can ever DCHECK on it. 
Let's take Cache-Control/Content-Length/Referer/Host/etc for example.  I think
we these headers should only be set in BuildRequestHeaders() in
net/http/http_network_transaction.cc.  I want to DCHECK if other net code is
setting this.  If an XHR set it, I do not want to DCHECK (since I suspect it's
valid, albeit stupid, for an XHR to set headers like this [or does WebKit strip
this guys out?]).  I don't see in your system how I would accomplish this.

> 
> If we keep two set of headers we still have the problem of deciding how to
merge
> them (do we take the user provided one or the one set by net?), and we are
> probably making the decision at the wrong place (because the merger is not the
> one who set the net header). I guess you mean that we take the user provided
one
> so override(2) is RemoveHeader() + SetHeader(), so the code looks almost the
> same in both cases except that there is no way to do (3).
> 
> > 
> > I think what we need to do is:
> > Keep extra_headers as the HttpRequestHeaders as we received them at the
start
> of
> > the URLRequest.  Never modify this within net/ except to call RemoveHeader()
> as
> > needed.
> > Add a field to URLRequest that is for net_headers.  Use that within net/. 
> Call
> > SetHeader() as needed.  SetHeader(header) should DCHECK(!HasHeader(header)),
> and
> > overwrite.  At the end, we do net_headers.MergeFrom(extra_headers);  We
change
> > the semantics of MergeFrom() such that it does not set a header that already
> > exists, but does LOG(WARNING).  Then we do any final filtering of
net_headers
> > headers (see kExtraHeadersToBeStripped in http_network_transaction.cc) and
> then
> > serialize to a string and send to the user.
> > 
> > I think that if we use two fields, we can meet the goals I listed earlier
> > regarding non-net headers and net headers.  It also seems it is unnecessary
to
> > add any more member functions to HttpRequestHeaders(), although we may need
to
> > adjust the semantics for MergeFrom().  Let me know what you think.
> > 
> > > >
> > > > On Apr 7, 2010 7:57 PM, "William Chan (陈智昌)"
> <mailto:willchan@chromium.org>
> > wrote:
> > > >
> > > > On Wed, Apr 7, 2010 at 5:31 PM, Ricardo Vargas
<mailto:rvargas@google.com>
> > wrote:
> > > >> That basically answers ...
> > > >
> > > > Hm, I'm trying to think what the right behavior is here.  An XHR could
> > > > set any header in WebKit.  That would get passed to the RDH, which
> > > > would set the extra request headers.  Then chrome code would call
> > > > SetHeader().  What's the right behavior here?
> > > >
> > > > My instinct is, headers set by Chrome should be canonical.  Chrome
> > > > should not be setting the same header multiple times (we should DCHECK
> > > > on this).  Chrome headers should overwrite WebKit headers (not DCHECK,
> > > > just overwrite and perhaps LOG(WARNING)).  Implementing this will
> > > > require splitting out extra_headers into two separate fields, so it's
> > > > clear which one is being set by Chrome and which is set by WebKit.  I
> > > > note that SetExtraRequestHeaders() is called not just for WebKit
> > > > generated headers, but also plugin and browser sync and what not, so
> > > > perhaps I should just draw the distinction at chrome network stack
> > > > generated headers and outside generated headers.  I think the addition
> > > > of DCHECKs should be done in a follow up changelist, since that will
> > > > require more refactoring of extra_headers (the splitting of it into
> > > > two fields).
> > > >
> > > > Thoughts?
> > > >
> > > >>
> > > >> On Apr 7, 2010 7:10 PM, "William Chan (陈智昌)"
> <mailto:willchan@chromium.org>
> > > >> wrote:
> > > >>
> > > >> On Wed, Apr 7, ...
> > >

[rvargas (doing something else), 2010-04-16 01:36:49]

On 2010/04/16 01:09:23, willchan wrote:
> On 2010/04/16 00:54:53, rvargas wrote:
> > On 2010/04/16 00:10:36, willchan wrote:
> > > On 2010/04/08 07:31:32, willchan wrote:
> > > > Oops, I had dropped everyone else.  I've added them back.  Eric,
> > > > please chime in if you have an opinion.  I'll think some more about
> > > > this and reply tomorrow.
> > > > 
> > > > 2010/4/8 Ricardo Vargas <rvargas@google.com>:
> > > > > I basically agree with everything you said.
> > > > > Maybe we can have a different method to replace a header, and another
> one
> > to
> > > > > append a value at the end (plus the current set+dcheck). That way the
> > caller
> > > > > can use the right one without having to split the headers in two: from
> net
> > > > > it doesn't matter who set a header, but usually there is a separate
code
> > > > > path for a header already present.
> > > > > On second thought, we could have only two methods and require the use
of
> > > > > remove+set to replace a header... at least from net I don't think we
> > should
> > > > > replace something set by the caller unless we do it intentionally.
> > > 
> > > I'm not sure either of these methods work unless you have two separate
> > > HttpRequestHeaders instances.  Let me know if I'm missing something.  What
> I'm
> > > thinking is that, if we keep the extra_headers field as our working copy,
as
> > > opposed to being a non_network_stack_set_headers, then as we update
> > > extra_headers with SetHeader() and friends, we don't know if we are
> > > deleting/overwriting/appending to a header set within the network stack as
> > > opposed to one outside the network stack.  This is because, as we move
later
> > on
> > > through the network stack code, we won't know, for a given header in
> > > extra_headers, whether or not it was set by the net/ or non-net/ code.
> > 
> > We could have separate headers as you describe, but my point is that for a
> given
> > piece of code at layer x on the net stack:
> > 
> > 1. If we are interested in a given header, we should see if that header is
> > already set by a higher layer (net/browser/renderer/plugin) and if the
> previous
> > setting is something that we want to override, or something that changes our
> > behavior somehow. In other words I don't see clearly why net_layer_x should
> care
> > if the header was set by net_layer_y or not.
> > 2. If we decide to override, we do it explicitly
> > 3. If we can keep both values we append to this header
> > 4. If we are just setting the header, we'll DCHECK if the header was already
> > set.
> 
> I guess I don't see how 4 is ever valid, since it's possible for _any_ header
to
> already have been set by an XHR, so I don't think we can ever DCHECK on it. 
> Let's take Cache-Control/Content-Length/Referer/Host/etc for example.  I think
> we these headers should only be set in BuildRequestHeaders() in
> net/http/http_network_transaction.cc.  I want to DCHECK if other net code is
> setting this.  If an XHR set it, I do not want to DCHECK (since I suspect it's
> valid, albeit stupid, for an XHR to set headers like this [or does WebKit
strip
> this guys out?]).  I don't see in your system how I would accomplish this.
> 

4 just means that 1 takes place... in other words, that we don't call
SetHeader() without first looking at the headers already set.

Yes, with one approach you cannot DCHECK if somebody in the net stack sets a
given header

With the other approach you cannot DCHECK if somebody in the net stack sets a
given header that is already provided by the browser without realizing it. 

It's probably as easy for somebody to add one of the headers you mention from
the net module as it is from the browser or plugin code, and there is no way to
DCHECK that.

To be fair, I see the two approaches very similar and don't mind either one (nor
do I think that they should be implemented with this CL).

[willchan no longer on Chromium, 2010-04-16 22:41:38]

Ok, does everyone agree to punt this next step to another CL?  Let me know, and
if so, I'll commit this, pending eroman's LGTM.

On 2010/04/16 01:36:49, rvargas wrote:
> On 2010/04/16 01:09:23, willchan wrote:
> > On 2010/04/16 00:54:53, rvargas wrote:
> > > On 2010/04/16 00:10:36, willchan wrote:
> > > > On 2010/04/08 07:31:32, willchan wrote:
> > > > > Oops, I had dropped everyone else.  I've added them back.  Eric,
> > > > > please chime in if you have an opinion.  I'll think some more about
> > > > > this and reply tomorrow.
> > > > > 
> > > > > 2010/4/8 Ricardo Vargas <rvargas@google.com>:
> > > > > > I basically agree with everything you said.
> > > > > > Maybe we can have a different method to replace a header, and
another
> > one
> > > to
> > > > > > append a value at the end (plus the current set+dcheck). That way
the
> > > caller
> > > > > > can use the right one without having to split the headers in two:
from
> > net
> > > > > > it doesn't matter who set a header, but usually there is a separate
> code
> > > > > > path for a header already present.
> > > > > > On second thought, we could have only two methods and require the
use
> of
> > > > > > remove+set to replace a header... at least from net I don't think we
> > > should
> > > > > > replace something set by the caller unless we do it intentionally.
> > > > 
> > > > I'm not sure either of these methods work unless you have two separate
> > > > HttpRequestHeaders instances.  Let me know if I'm missing something. 
What
> > I'm
> > > > thinking is that, if we keep the extra_headers field as our working
copy,
> as
> > > > opposed to being a non_network_stack_set_headers, then as we update
> > > > extra_headers with SetHeader() and friends, we don't know if we are
> > > > deleting/overwriting/appending to a header set within the network stack
as
> > > > opposed to one outside the network stack.  This is because, as we move
> later
> > > on
> > > > through the network stack code, we won't know, for a given header in
> > > > extra_headers, whether or not it was set by the net/ or non-net/ code.
> > > 
> > > We could have separate headers as you describe, but my point is that for a
> > given
> > > piece of code at layer x on the net stack:
> > > 
> > > 1. If we are interested in a given header, we should see if that header is
> > > already set by a higher layer (net/browser/renderer/plugin) and if the
> > previous
> > > setting is something that we want to override, or something that changes
our
> > > behavior somehow. In other words I don't see clearly why net_layer_x
should
> > care
> > > if the header was set by net_layer_y or not.
> > > 2. If we decide to override, we do it explicitly
> > > 3. If we can keep both values we append to this header
> > > 4. If we are just setting the header, we'll DCHECK if the header was
already
> > > set.
> > 
> > I guess I don't see how 4 is ever valid, since it's possible for _any_
header
> to
> > already have been set by an XHR, so I don't think we can ever DCHECK on it. 
> > Let's take Cache-Control/Content-Length/Referer/Host/etc for example.  I
think
> > we these headers should only be set in BuildRequestHeaders() in
> > net/http/http_network_transaction.cc.  I want to DCHECK if other net code is
> > setting this.  If an XHR set it, I do not want to DCHECK (since I suspect
it's
> > valid, albeit stupid, for an XHR to set headers like this [or does WebKit
> strip
> > this guys out?]).  I don't see in your system how I would accomplish this.
> > 
> 
> 4 just means that 1 takes place... in other words, that we don't call
> SetHeader() without first looking at the headers already set.
> 
> Yes, with one approach you cannot DCHECK if somebody in the net stack sets a
> given header
> 
> With the other approach you cannot DCHECK if somebody in the net stack sets a
> given header that is already provided by the browser without realizing it. 
> 
> It's probably as easy for somebody to add one of the headers you mention from
> the net module as it is from the browser or plugin code, and there is no way
to
> DCHECK that.
> 
> To be fair, I see the two approaches very similar and don't mind either one
(nor
> do I think that they should be implemented with this CL).