Stop sending Accept-Charset HTTP header.

Stop sending Accept-Charset HTTP header as its not relevant anymore.  Remove it from HttpUserAgentSettings as well as this is no longer needed.

BUG=112804
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188237

[pauljensen, 2013-03-08 03:14:39]

Matt, PTAL.

[willchan no longer on Chromium, 2013-03-08 04:11:42]

LGTM, nice! Thanks for doing this.

I don't believe we're near a major branchpoint, but make sure this doesn't go
out right near it. Since this has web compat concerns, I want to make sure it
gets time on a dev channel release so we can catch any regressions.

Please notify ConOps so they can keep an eye out on the support forums. Let me
know if you need a contact there. cbentzel@ should be able to help you
otherwise.

[joth, 2013-03-08 05:28:26]

a_w/ lgtm

branch point is just over two weeks
http://dev.chromium.org/developers/calendar


On 7 March 2013 20:11, <willchan@chromium.org> wrote:

> LGTM, nice! Thanks for doing this.
>
> I don't believe we're near a major branchpoint, but make sure this doesn't
> go
> out right near it. Since this has web compat concerns, I want to make sure
> it
> gets time on a dev channel release so we can catch any regressions.
>
> Please notify ConOps so they can keep an eye out on the support forums.
> Let me
> know if you need a contact there. cbentzel@ should be able to help you
> otherwise.
>
>
https://codereview.chromium.**org/12463021/<https://codereview.chromium.org/1...
>

[Nicholas Shanks, 2013-03-08 11:03:48]

As noted in the Firefox bug for this issue (linked from the crbug), removing the
ability to set the header entirely is probably out of scope for the bug, and may
hurt the Developer Tools, which I was hoping would receive overrides for more
headers than just User-Agent. [Show Developer Tools -> Settings (Gear button) ->
Overrides]

It is highly desirable for me as a web application developer to override Accept
and other Accept-* headers, including turning off Accept-Language.

I agree that the header should not be sent in the normal course of surfing the
web.

[pauljensen, 2013-03-08 15:01:19]

Nicholas, feel free to file a feature request for more header overrides in the
Developer Tools Overrides on crbug.com.

[pauljensen, 2013-03-08 15:12:24]

Brett, please review webkit/, content/ and chrome/ (skipping
chrome/browser/net/).
Fred, please review sync/.

[mmenke, 2013-03-08 15:37:48]

https://codereview.chromium.org/12463021/diff/14002/chrome/browser/net/chrome...
File chrome/browser/net/chrome_http_user_agent_settings.cc (left):

https://codereview.chromium.org/12463021/diff/14002/chrome/browser/net/chrome...
chrome/browser/net/chrome_http_user_agent_settings.cc:16:
pref_accept_charset_.Init(prefs::kDefaultCharset, prefs);
This preference value should be removed, right?

https://codereview.chromium.org/12463021/diff/14002/webkit/tools/test_shell/t...
File webkit/tools/test_shell/test_shell_request_context.cc (right):

https://codereview.chromium.org/12463021/diff/14002/webkit/tools/test_shell/t...
webkit/tools/test_shell/test_shell_request_context.cc:43: // hard-code
accept-language for test shells
nit:  Accept-Language

[mmenke, 2013-03-08 15:41:11]

On 2013/03/08 11:03:48, Nicholas Shanks wrote:
> As noted in the Firefox bug for this issue (linked from the crbug), removing
the
> ability to set the header entirely is probably out of scope for the bug, and
may
> hurt the Developer Tools, which I was hoping would receive overrides for more
> headers than just User-Agent. [Show Developer Tools -> Settings (Gear button)
->
> Overrides]
> 
> It is highly desirable for me as a web application developer to override
Accept
> and other Accept-* headers, including turning off Accept-Language.
> 
> I agree that the header should not be sent in the normal course of surfing the
> web.

This doesn't remove the ability to set the header.  It just removes code in net/
specifically for setting that header (And the profile preference as well).  Like
any other header, Accept-Charset can be set on a per-request basis.

[pauljensen, 2013-03-09 19:21:15]

https://codereview.chromium.org/12463021/diff/14002/chrome/browser/net/chrome...
File chrome/browser/net/chrome_http_user_agent_settings.cc (left):

https://codereview.chromium.org/12463021/diff/14002/chrome/browser/net/chrome...
chrome/browser/net/chrome_http_user_agent_settings.cc:16:
pref_accept_charset_.Init(prefs::kDefaultCharset, prefs);
On 2013/03/08 15:37:49, mmenke wrote:
> This preference value should be removed, right?

"git grep kDefaultCharset" turns up a few other uses.  Looks like it's used for
determining download file names.  Used for creating a machine finger-print but
that probably isn't critical.  It's also passed in WebPreferences off to WebKit.
 I feel like leaving the preference alone for now as it does seem to enact some
good in other parts of Chromium; if you disagree I can look into removing it;
let me know.

https://codereview.chromium.org/12463021/diff/14002/webkit/tools/test_shell/t...
File webkit/tools/test_shell/test_shell_request_context.cc (right):

https://codereview.chromium.org/12463021/diff/14002/webkit/tools/test_shell/t...
webkit/tools/test_shell/test_shell_request_context.cc:43: // hard-code
accept-language for test shells
On 2013/03/08 15:37:49, mmenke wrote:
> nit:  Accept-Language
Done, and I added some capitalization and punctuation to the comment to make it
a sentence like the other comment in this file and other files in Chromium.

[mmenke, 2013-03-10 19:02:32]

LGTM

[brettw, 2013-03-14 04:46:22]

lgtm

https://codereview.chromium.org/12463021/diff/41001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/12463021/diff/41001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:509: new BasicHttpUserAgentSettings(EmptyString()));
Can you change this existing code to just use std::string() instead? EmptyString
is really for when you need to return a ref and don't have an object with the
right lifetime.

https://codereview.chromium.org/12463021/diff/41001/chrome/browser/net/basic_...
File chrome/browser/net/basic_http_user_agent_settings.cc (right):

https://codereview.chromium.org/12463021/diff/41001/chrome/browser/net/basic_...
chrome/browser/net/basic_http_user_agent_settings.cc:10: const std::string&
accept_language) : accept_language_(accept_language) {
Put : on next line when not everything fits on one.

[pauljensen, 2013-03-14 14:40:23]

Will, the branch-point is 12 days away, is this too close or should I land
today?

[willchan no longer on Chromium, 2013-03-14 14:49:21]

Seems fine to land
On Mar 14, 2013 10:40 AM, <pauljensen@chromium.org> wrote:

> Will, the branch-point is 12 days away, is this too close or should I land
> today?
>
>
https://codereview.chromium.**org/12463021/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-03-14 15:56:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/12463021/53001

[commit-bot: I haz the power, 2013-03-14 15:56:18]

Failed to apply patch for chrome/browser/policy/device_management_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/policy/device_management_service.cc
  Hunk #1 FAILED at 221.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/policy/device_management_service.cc.rej

Patch:       chrome/browser/policy/device_management_service.cc
Index: chrome/browser/policy/device_management_service.cc
diff --git a/chrome/browser/policy/device_management_service.cc
b/chrome/browser/policy/device_management_service.cc
index
7cdc87d30f3cc1e425d12e65c6be76982812fd76..15accb67f035042da2fada67921e3fc502224f03
100644
--- a/chrome/browser/policy/device_management_service.cc
+++ b/chrome/browser/policy/device_management_service.cc
@@ -221,8 +221,8 @@ class DeviceManagementRequestContext : public
net::URLRequestContext {
 
 DeviceManagementRequestContext::DeviceManagementRequestContext(
     net::URLRequestContext* base_context)
-    // Use sane Accept-Language and Accept-Charset values for our purposes.
-    : basic_http_user_agent_settings_("*", "*") {
+    // Use sane Accept-Language value for our purposes.
+    : basic_http_user_agent_settings_("*") {
   // Share resolver, proxy service and ssl bits with the baseline context. This
   // is important so we don't make redundant requests (e.g. when resolving
proxy
   // auto configuration).

[commit-bot: I haz the power, 2013-03-14 16:31:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/12463021/56001

[commit-bot: I haz the power, 2013-03-14 16:32:09]

Presubmit check for 12463021-56001 failed and returned exit status 1.

INFO:root:Found 27 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/sync/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    sync/internal_api/http_bridge.cc
    sync/internal_api/public/http_bridge.h

Presubmit checks took 2.5s to calculate.

[akalin, 2013-03-14 17:17:53]

sync lgtm

[commit-bot: I haz the power, 2013-03-14 17:24:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/12463021/56001

[commit-bot: I haz the power, 2013-03-15 01:00:15]

Change committed as 188237