Don't add default Accept-Language and Accept-Charset headers if specific ones were set in URLRequest

Modify URLRequestHttpJob to avoid adding default Accept-Language and Accept-Charset headers if the specific ones were set on URLRequest.
BUG=20894
TEST=enabled layout test LayoutTests/http/tests/xmlhttprequest/access-control-basic-whitelist-request-headers.html

[Dmitry Titov, 2009-09-17 20:48:26]

Darin, Pam - could you please take a look? This hardcoding always results in
adding the headers (or concatenating if the test supplies its own headers, via
XHR for example). 

This code is there since initial population of svn. I removed it and run layout
tests - and it seems there is nothing failing.

Perhaps you remember some  historic reason why this code is in there.

[darin (slow to review), 2009-09-17 21:14:30]

Looking back into the old repository, the change was made by Jungshik here:
http://chrome-corpsvn.mtv/viewvc?view=rev&root=chrome&revision=26165

(That link is not publicly visible.)

I believe it makes sense for TestShell to have default A-L and A-C headers.
 I think the correct fix for this bug is to make the network module smarter
to not use the default values when the application specifies particular
values.

While your patch corrects the layout test, it does nothing to help Chrome,
which would still have the bug that the layout test is trying to check.

-Darin



On Thu, Sep 17, 2009 at 1:48 PM, <dimich@chromium.org> wrote:

> Reviewers: darin, Pam,
>
> Message:
> Darin, Pam - could you please take a look? This hardcoding always results
> in
> adding the headers (or concatenating if the test supplies its own headers,
> via
> XHR for example).
>
> This code is there since initial population of svn. I removed it and run
> layout
> tests - and it seems there is nothing failing.
>
> Perhaps you remember some  historic reason why this code is in there.
>
> Description:
> Remove hardcoding of Accept-Language and Accept-Charset for TestShell.
> BUG=20894
> TEST=enabled layout test
>
>
LayoutTests/http/tests/xmlhttprequest/access-control-basic-whitelist-request-headers.html
>
> Please review this at http://codereview.chromium.org/213010
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     webkit/tools/layout_tests/test_expectations.txt
>  M     webkit/tools/test_shell/test_shell_request_context.cc
>
>
> Index: webkit/tools/layout_tests/test_expectations.txt
> ===================================================================
> --- webkit/tools/layout_tests/test_expectations.txt     (revision 26189)
> +++ webkit/tools/layout_tests/test_expectations.txt     (working copy)
> @@ -1647,7 +1647,6 @@
>  // New failures from 41660:41709
>  // Newly added tests.
>  BUG9161 MAC LINUX WIN : LayoutTests/fast/forms/basic-textareas.html = FAIL
> -BUG20894 :
>
LayoutTests/http/tests/xmlhttprequest/access-control-basic-whitelist-request-headers.html
> = FAIL
>
>  // Tests that fail in debug mode on linux
>  BUG10454 DEBUG LINUX : LayoutTests/svg/custom/mask-excessive-malloc.svg =
> CRASH
> Index: webkit/tools/test_shell/test_shell_request_context.cc
> ===================================================================
> --- webkit/tools/test_shell/test_shell_request_context.cc       (revision
> 26189)
> +++ webkit/tools/test_shell/test_shell_request_context.cc       (working
> copy)
> @@ -30,10 +30,6 @@
>     bool no_proxy) {
>   cookie_store_ = new net::CookieMonster();
>
> -  // hard-code A-L and A-C for test shells
> -  accept_language_ = "en-us,en";
> -  accept_charset_ = "iso-8859-1,*,utf-8";
> -
>   net::ProxyConfig proxy_config;
>  #if defined(OS_LINUX)
>   // Force no_proxy to true so as to use a fixed proxy configuration
>
>
>

[Dmitry Titov, 2009-09-18 00:06:36]

Replying again, in the bug:

Thanks Darin! After some poking around I think I understand it. WebKit DRT also
sends default languages if nothing was specifically specified. I'll change the
fix.

[Dmitry Titov, 2009-09-21 22:55:22]

Could you give it another look? I've changed the URLRequestHttpJob to avoid
adding default values if the request had those headers specified.

[darin (slow to review), 2009-09-21 22:58:22]

I'd like to defer review to Eric Roman.  If he's happy, I'm happy.
-Darin



On Mon, Sep 21, 2009 at 3:55 PM, <dimich@chromium.org> wrote:

> Could you give it another look? I've changed the URLRequestHttpJob to avoid
> adding default values if the request had those headers specified.
>
> http://codereview.chromium.org/213010
>

[eroman, 2009-09-22 01:02:45]

<General_Design>

I am OK with this approach, in order to fix the immediate bug.

However I am not satisfied with it. I have filed a follow-up bug on how I think
we should address this in the future (http://crbug.com/22588).

Note that I don't expect you to tackle 22588, but I did want to mention it:

This "std::string extra_headers" business has caused us grief before (with
duplicate headers). And I expect it will continue to. My proposed remedy is to
introduce a dedicated type for buiding the request headers, that is more like a
map than a string -- say "HttpRequestHeaders" or "HttpHeadersBuilder".

</General_Design>



<Testing>

Please add some tests in the network module for this new behavior.

I reckon the right place is in url_request_unittest.cc -- I can help you with
this if you have any questions.

</Testing>

http://codereview.chromium.org/213010/diff/6001/5003
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/213010/diff/6001/5003#newcode649
Line 649: static const std::string kAcceptLanguage = "Accept-Language";
Can you make this a const char* instead?

http://codereview.chromium.org/213010/diff/6001/5003#newcode651
Line 651: !net::HttpUtil::HasHeader(request_info_.extra_headers,
kAcceptLanguage.c_str()))
break lines at 80 characters.

http://codereview.chromium.org/213010/diff/6001/5003#newcode655
Line 655: static const std::string kAcceptCharset = "Accept-Charset";
Please only use static const char*.

http://codereview.chromium.org/213010/diff/6001/5003#newcode657
Line 657: !net::HttpUtil::HasHeader(request_info_.extra_headers,
kAcceptCharset.c_str()))
break lines at 80 characters.

http://codereview.chromium.org/213010/diff/6001/5003#newcode658
Line 658: request_info_.extra_headers += kAcceptCharset + ": " +
Since this is the same as for accept-language, can you extract it to a function?

Perhaps something like:

// If |*headers| already contains |header_name| do nothing. 
// Otherwise add <header_name> ":" <header_value> to the end of the list.
void AppendHeaderIfMissing(const char* header_name, const std::string&
header_value, std::string* headers);

[Dmitry Titov, 2009-09-24 01:43:28]

Fixed all, but it's now in http://codereview.chromium.org/218015