Add q-values to languages in Accept-Language HTTP header to be compatible wit...

Add q-values to languages in Accept-Language HTTP header to be compatible with Apache.
Add q-values to charsets in Accept-Charset header in the same way as Firefox does. 

BUG=5899
TEST=HttpUtilTest.Accept*  (net_unittest)



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

[wtc, 2009-01-13 01:42:49]

This CL seems OK, but I'd like to ask you some
questions first and then look at it again.  Here
are my initial review comments.

http://codereview.chromium.org/17340/diff/203/204
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/17340/diff/203/204#newcode9
Line 9: #include "base/string_tokenizer.h"
Nit: list "base/string_tokenizer.h" before
"base/string_util.h".

http://codereview.chromium.org/17340/diff/203/204#newcode36
Line 36: // 2. For 'll-R1, ll-R2', add 'll' after the first 'll-**' because
What are ll-R1, ll-R2, and ll-**?

http://codereview.chromium.org/17340/diff/203/204#newcode38
Line 38: std::string GetAcceptLangWithQvalue(const std::string&
raw_language_list) {
Nit: for consistency with the CreateProxyInfo function
below, make GetAcceptLangWithQvalue a static function
in the default namespace, or move CreateProxyInfo into
the unnamed namespace (and remove static).

http://codereview.chromium.org/17340/diff/203/204#newcode139
Line 139: //
http://mxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpHandl...
)
It'd be better to use a MXR link that will remain valid when
the source code changes, or just summarize what the Firefox
code does.

http://codereview.chromium.org/17340/diff/203/204#newcode144
Line 144: accept_charset_ += ",utf-8;q=0.6,*;q=0.6";
Would be nice to explain the magic value 0.6.
I guess it's the midpoint of 1.0 and 0.1, right?

[darin (slow to review), 2009-01-13 05:40:09]

http://codereview.chromium.org/17340/diff/203/204
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/17340/diff/203/204#newcode25
Line 25: // with some web servers (e.g. Apache 2.0. See http://crbug.com/5899).
given that all consumers of the network stack will likely need to deal with this
issue, perhaps we should really just push this code down into the network stack.
 maybe you could just add a method to HttpUtil that performs the q-value
insertion, and you could mention that function in the documentation on
URLRequestContext for the accept_language member.  (that way you can also easily
write a unit test for it.)

maybe there could also be a utility for fixing-up the accept-charset value too.

[jungshik at Google, 2009-01-14 00:22:28]

Wan-Teh and Darin, thank you for the comment. I think all of your comments were
addressed now. Writing a unit test helped me fix a bug in my oriignal
implementation (it's been a while since I dealt with floating point numbers
:-)). 

Can you take another look?


On 2009/01/13 05:40:09, darin wrote:
> http://codereview.chromium.org/17340/diff/203/204
> File chrome/browser/net/chrome_url_request_context.cc (right):
> 
> http://codereview.chromium.org/17340/diff/203/204#newcode25
> Line 25: // with some web servers (e.g. Apache 2.0. See
http://crbug.com/5899).
> given that all consumers of the network stack will likely need to deal with
this
> issue, perhaps we should really just push this code down into the network
stack.
>  maybe you could just add a method to HttpUtil that performs the q-value
> insertion, and you could mention that function in the documentation on
> URLRequestContext for the accept_language member.  (that way you can also
easily
> write a unit test for it.)
> 
> maybe there could also be a utility for fixing-up the accept-charset value
too.

[darin (slow to review), 2009-01-14 00:38:56]

http://codereview.chromium.org/17340/diff/601/604
File net/http/http_util.cc (right):

http://codereview.chromium.org/17340/diff/601/604#newcode473
Line 473: if (qvalue10 == 10)
minor nit: even though it is not required by the style guide, please add {}'s to
these if and else lines.  it is the convention we've been mostly following in
the network code.  same goes for the if-else block in the next function.

http://codereview.chromium.org/17340/diff/601/605
File net/http/http_util.h (right):

http://codereview.chromium.org/17340/diff/601/605#newcode107
Line 107: // the Preferences), return the list with a q-value appended to each
nit: in the network stack, the concept of Preferences doesn't exist.  maybe you
could just leave out this parenthetical?

http://codereview.chromium.org/17340/diff/601/605#newcode114
Line 114: // When making a http request, this should be used to determine what
nit: please add an extra blank line above this line for readability.

http://codereview.chromium.org/17340/diff/601/605#newcode119
Line 119: static std::string GetAcceptLangWithQvalue(
How about just naming these methods: GenerateAcceptLanguageHeader and
GenerateAcceptCharsetHeader?

[wtc, 2009-01-14 00:50:51]

LGTM.  Just some nits below.

Nit: I'd prefer that you use "qvalue" instead of "q-value"
to match how it's spelled in RFC 2616.

http://codereview.chromium.org/17340/diff/601/604
File net/http/http_util.cc (right):

http://codereview.chromium.org/17340/diff/601/604#newcode466
Line 466: // two floating point numbers.
I wondered about this issue yesterday...

http://codereview.chromium.org/17340/diff/601/604#newcode473
Line 473: if (qvalue10 == 10)
You can also use a bool variable "first_lang"
instead of comparing qvalue10 with 10.  This
would avoid the floating point number comparision issue.

http://codereview.chromium.org/17340/diff/601/604#newcode477
Line 477: StringAppendF(&lang_list_with_q, ",%s;q=0.%d", language.c_str(),
You should DCHECK that qvalue10 >= 0 && qvalue < 10,
which is required by the 0.%d format.

http://codereview.chromium.org/17340/diff/601/605
File net/http/http_util.h (right):

http://codereview.chromium.org/17340/diff/601/605#newcode104
Line 104: 
Nit: remove this extra blank line.

http://codereview.chromium.org/17340/diff/601/605#newcode107
Line 107: // the Preferences), return the list with a q-value appended to each
Nit: remove "(as stored in the Preferences)" from this
comment.

http://codereview.chromium.org/17340/diff/601/605#newcode109
Line 109: // starts with 1.0 and is decremented by 0.2 for each successive entry
Why did you change the decrement from 0.1 to 0.2?

http://codereview.chromium.org/17340/diff/601/605#newcode117
Line 117: // of them as having q=1.0 and pick one of them even though it may not
RFC 2616 says:
  The quality value defaults to "q=1".
So this is expected.  We should remove "appear to"
from the comment because correctly implemented web
servers should regard all of them as having q=1.0.

http://codereview.chromium.org/17340/diff/601/605#newcode123
Line 123: // it will return 'utf-8,*'. Otherwise (e.g. 'euc-jp'), it'll return
Is "utf-8,*" the right list to return?  Doesn't that mean
all charsets, including utf-8, get the default q=1?

http://codereview.chromium.org/17340/diff/601/605#newcode129
Line 129: // does not really matter.
If the actual value doesn't matter, I'd prefer that
we use a more obvious value of 0.5 and delete the
comment about matching what Firefox does.

[jungshik at Google, 2009-01-14 02:07:00]

Thank you for going over again. 

In the latest patch set, I addressed the concerns of both of you.

On 2009/01/14 00:50:51, wtc wrote:

> Why did you change the decrement from 0.1 to 0.2?

I thought even having 5 entries would be relatively rare. 

As for qvaules in A-C header, I changed it to generate 

utf-8,*;q=0.5
euc-jp,utf-8;q=0.7,*;q=0.3

[wtc, 2009-01-14 02:21:48]

LGTM.

http://codereview.chromium.org/17340/diff/618/19
File chrome/browser/net/chrome_url_request_context.cc (left):

http://codereview.chromium.org/17340/diff/618/19#oldcode103
Line 103: accept_charset_ += ",*,utf-8";
In the old code, we put * before utf-8.  In
the new code, we put utf-8 before *.  Does the
order matter?

http://codereview.chromium.org/17340/diff/618/22
File net/http/http_util.h (right):

http://codereview.chromium.org/17340/diff/618/22#newcode113
Line 113: 
Nit: add // for the blank line.

[jungshik at Google, 2009-01-23 02:00:09]

Checked in with nit fixed. 

On 2009/01/14 02:21:48, wtc wrote:

> http://codereview.chromium.org/17340/diff/618/19#oldcode103
> Line 103: accept_charset_ += ",*,utf-8";
> In the old code, we put * before utf-8.  In
> the new code, we put utf-8 before *.  Does the
> order matter?

I think the new code is better in 2009 :-). If the default encoding of a user's
choice is not available, we'd better ask for UTF-8 rather than 'any encoding'.

[wtc, 2009-01-26 21:20:02]

On 2009/01/23 02:00:09, Jungshik Shin wrote:
>
> On 2009/01/14 02:21:48, wtc wrote:
> 
> > http://codereview.chromium.org/17340/diff/618/19#oldcode103
> > Line 103: accept_charset_ += ",*,utf-8";
> > In the old code, we put * before utf-8.  In
> > the new code, we put utf-8 before *.  Does the
> > order matter?
> 
> I think the new code is better in 2009 :-). If the default encoding of a
user's
> choice is not available, we'd better ask for UTF-8 rather than 'any encoding'.

Good.  I agree that the old code doesn't make
sense.  (I remember I asked you about it before.)