Provide mutable members of UrlRequestContext via pure-virtual interface

chrome/browser/net/chrome_url_request_context.cc

Index: chrome/browser/net/chrome_url_request_context.cc
diff --git a/chrome/browser/net/chrome_url_request_context.cc b/chrome/browser/net/chrome_url_request_context.cc
index 6b8bbc803dab9db0c6e11b00cfee894183238334..40142c3ffed95b45f73a43d805270853d4e65de5 100644
--- a/chrome/browser/net/chrome_url_request_context.cc
+++ b/chrome/browser/net/chrome_url_request_context.cc
@@ -8,8 +8,10 @@
 #include "base/compiler_specific.h"
 #include "base/message_loop.h"
 #include "base/message_loop_proxy.h"
+#include "base/string_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/io_thread.h"
+#include "chrome/browser/net/basic_http_user_agent_settings.h"
 #include "chrome/browser/net/load_time_stats.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -37,6 +39,43 @@ class ChromeURLRequestContextFactory {
   DISALLOW_COPY_AND_ASSIGN(ChromeURLRequestContextFactory);
 };
 
+// Implementation of HttpUserAgentSettings for ChromeURLRequestContext
+// that mirrors changes in Accept-Language and Accept-Charset from
+// content's PrefService.
+class ChromeHttpUserAgentSettings : public net::HttpUserAgentSettings {
+ public:
+  ChromeHttpUserAgentSettings() {}
+  virtual ~ChromeHttpUserAgentSettings() {}
+
+  // net::HttpUserAgentSettings implementation.
+  virtual std::string GetAcceptLanguage() OVERRIDE {
+    return accept_language_;
+  }
+  virtual std::string GetAcceptCharset() OVERRIDE {
+    return accept_charset_;
+  }
+  virtual std::string GetUserAgent(const GURL& url) OVERRIDE {
+    return content::GetUserAgent(url);
+  }
+

[willchan no longer on Chromium, 2012/10/06 19:32:49]

too many newlines

+
+  // Sets the value of 'Accept-Charset' header field.
+  void set_accept_charset(const std::string& accept_charset) {
+    accept_charset_ = accept_charset;
+  }
+
+  // Sets the value of 'Accept-Language' header field.
+  void set_accept_language(const std::string& accept_language) {
+    accept_language_ = accept_language;
+  }
+
+ private:
+  std::string accept_language_;
+  std::string accept_charset_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeHttpUserAgentSettings);
+};
+
 namespace {
 
 // ----------------------------------------------------------------------------
@@ -337,10 +376,12 @@ ChromeURLRequestContext::ChromeURLRequestContext(
     chrome_browser_net::LoadTimeStats* load_time_stats)
     : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
       is_incognito_(false),
-      load_time_stats_(load_time_stats) {
+      load_time_stats_(load_time_stats),
+      chrome_http_user_agent_settings_(new ChromeHttpUserAgentSettings) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (load_time_stats_)
     load_time_stats_->RegisterURLRequestContext(this, type);
+  set_http_user_agent_settings(chrome_http_user_agent_settings_.get());

[willchan no longer on Chromium, 2012/10/06 19:32:49]

Sorry if someone called this out earlier and there's an explanation. I didn't
read comments, just wondering about this here. If I've missed something, please
call out my stupidity.

How does this work exactly? It looks like net::URLRequestContext does not own
HttpUserAgentSettings, which I think is the correct thing to do. However,
ChromeURLRequestContext owns its own ChromeHttpUserAgentSettings, which it sets
|http_user_agent_settings_| to point to in its constructor, this line right
here. Afterward, when CopyFrom() gets called, net::URLRequestContext::CopyFrom()
will overwrite |http_user_agent_settings_| with the source URLRequestContext's
|http_user_agent_settings_|. That means this object's
|http_user_agent_settings_| will no longer point to
|chrome_http_user_agent_settings_|.

Is my understanding correct? If so, I think that's the wrong behavior. It seems
to me that, like we do for almost everything else in ChromeURLRequestContext, we
should have a single ChromeHttpUserAgentSettings instance, which is owned by
ProfileIOData, and have each of the ChromeHttpUserAgentSettings point to that
guy. CopyFrom() will then work correctly.

[erikwright (departed), 2012/10/09 15:24:46]

When you call CopyFrom, you get all of the other context's pointers, which
include a number of complex stateful objects.

This one is not any different, and I think the caller of CopyFrom gets the
behaviour they expect.

I'm actually surprised that anyone calls ChromeURLRequestContext::CopyFrom.. is
that part of a recent refactoring?

In any case, I think it is definitely more correct to have the
ChromeHttpUserAgentSettings owned by the ProfileIOData and for there to be a
single one.

[willchan no longer on Chromium, 2012/10/09 22:58:56]

I think it's been the case for a long time. See profile_impl_io_data.cc for
example.
Great.

 }
 
 ChromeURLRequestContext::~ChromeURLRequestContext() {
@@ -368,15 +409,10 @@ void ChromeURLRequestContext::set_chrome_url_data_manager_backend(
   chrome_url_data_manager_backend_ = backend;
 }
 
-const std::string& ChromeURLRequestContext::GetUserAgent(
-    const GURL& url) const {
-  return content::GetUserAgent(url);
-}
-
 void ChromeURLRequestContext::OnAcceptLanguageChange(
     const std::string& accept_language) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  set_accept_language(
+  chrome_http_user_agent_settings_->set_accept_language(
       net::HttpUtil::GenerateAcceptLanguageHeader(accept_language));
 }
 
@@ -384,6 +420,6 @@ void ChromeURLRequestContext::OnDefaultCharsetChange(
     const std::string& default_charset) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   set_referrer_charset(default_charset);
-  set_accept_charset(
+  chrome_http_user_agent_settings_->set_accept_charset(
       net::HttpUtil::GenerateAcceptCharsetHeader(default_charset));
 }