Provide mutable members of UrlRequestContext via pure-virtual interface

Provide mutable members of UrlRequestContext via pure-virtual interface.  Create a pure-virtual interface called HttpUserAgentSettings that provides access to the Accept-Language, Accept-Charset, and User-Agent HTTP headers.  Each UrlRequestContext should have a HttpUserAgentSettings implementation attached via set_http_user_agent_settings().  


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

[pauljensen, 2012-09-20 21:04:40]

Hey Erik,

Here's my refactor to access the mutable members of UrlRequestContext via a
pure-virtual API.  Let me know what you think :)  Don't hesitate to let me know
if I'm way off and need to go rework it.

-Paul

[erikwright (departed), 2012-09-21 15:06:33]

Lots of small comments, but in general looking on track.

Thanks!

http://codereview.chromium.org/10918279/diff/22046/chrome/browser/net/basic_h...
File chrome/browser/net/basic_http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/22046/chrome/browser/net/basic_h...
chrome/browser/net/basic_http_user_agent_settings.h:12: // An implementation of
|HttpUserAgentSettings| (goes into |URLRequestContext|)
no need for the parenthetical.

http://codereview.chromium.org/10918279/diff/22046/chrome/browser/net/basic_h...
chrome/browser/net/basic_http_user_agent_settings.h:14: // Accept-Charset; and,
uses |content::GetUserAgent| to provide the HTTP
remove ';' and ','

http://codereview.chromium.org/10918279/diff/22046/chrome/browser/net/basic_h...
chrome/browser/net/basic_http_user_agent_settings.h:23: virtual const
std::string& GetUserAgent(const GURL& url) OVERRIDE;
Prefix with comment:
// ConstHttpAcceptLanguageAndCharset implementation

http://codereview.chromium.org/10918279/diff/22046/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/10918279/diff/22046/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.cc:45: class
ChromeHttpUserAgentSettings : public BasicHttpUserAgentSettings {
Hmm. It's kind of funny that this inherits from Basic... which inherits from the
Const one, but everything from the Const one is actually overridden.

I'd suggest just inheriting from the pure-virtual API and re-implementing
GetUserAgent (it is, after all, only a one-line delegation anyway).

http://codereview.chromium.org/10918279/diff/22046/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/22046/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:321:
http_user_agent_settings_.reset(new BasicHttpUserAgentSettings(
I'm not certain, but I think that the main_context (and probably the
extensions_request_context are ChromeURLRequestContext instances and therefore
already are configured with the ChromeUserAgentSettings.

The same might also apply to the OffTheRecord profile.

If that's the case, replacing it would break the preferences-tracking
functionality.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
File net/url_request/url_request_context.cc (right):

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.cc:19:
ConstHttpAcceptLanguageAndCharset::ConstHttpAcceptLanguageAndCharset(
move.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.cc:22: accept_charset_(accept_charset) {}
Feel free to check prior-art and tell me what is predominant in cases where the
initializer list is already wrapped (I'm not sure), but I'm pretty it's to put
the } on a newline.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.cc:111: // Gets the value of
'Accept-Charset' header field.
no implementation comment necessary.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.cc:113: if (!http_user_agent_settings_)
nit:
  return http_user_agent_settings_ ?
      http_user_agent_settings_->GetAcceptCharset() : EmptyString();

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.h:47: class NET_EXPORT HttpUserAgentSettings
{
Move to http_user_agent_settings.h

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.h:53: // Gets the UA string to use for the
given URL.  Pass an invalid URL (such as
"Pass an invalid URL (such as GURL())" -> Pass an empty URL...

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.h:60: HttpUserAgentSettings() {}
This can be public (no-one can call it because of the pure-virtual methods).

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.h:63:
DISALLOW_COPY_AND_ASSIGN(HttpUserAgentSettings);
This can be omitted on pure-virtual classes because they can't be constructed
anyways.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.h:70: class NET_EXPORT
ConstHttpAcceptLanguageAndCharset
move to separate file.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.h:91: class NET_EXPORT
ConstHttpUserAgentSettings
move to separate file.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.h:241: const std::string& accept_charset()
const;
Group these together with a single "Legacy accessors that delegate to ..."
comment.
Add a TODO(pauljensen): Remove after all clients are updated to directly access
...

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.h:279: // the HTTP Accept-Language,
Accept-Charset and User-Agent header values that
Clients should actually access HttpUserAgentSettings directly very soon.

So just remove the bits about accept_language() etc.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_context.h:281: // Note: This implementation is not
owned by |URLRequestContext|, so it is
The "Note" is not required since it's the same as all other members. If we
weren't removing URLRequestContext ASAP I'd double check that the class comments
say as much, but right now I'm indifferent.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
File net/url_request/url_request_test_util.cc (right):

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_test_util.cc:111: if (accept_language().empty()) {
It might be more correct to provide a method:

TestURLRequestContext::SetUserAgentValues(accept_charset, accept_language,
user_agent) {
  // set a constant user-agent settings, using the defaults "en-us, fr" etc.
}

Clients that call ::Init() obviously already know about the derived class, so
they can access that helper method to set these strings.

[pauljensen, 2012-09-24 20:21:15]

Erik,

Thanks for all the comments!  I think I addressed them all with the exception
below.

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
File net/url_request/url_request_test_util.cc (right):

http://codereview.chromium.org/10918279/diff/22046/net/url_request/url_reques...
net/url_request/url_request_test_util.cc:111: if (accept_language().empty()) {
On 2012/09/21 15:06:33, erikwright wrote:
> It might be more correct to provide a method:
> 
> TestURLRequestContext::SetUserAgentValues(accept_charset, accept_language,
> user_agent) {
>   // set a constant user-agent settings, using the defaults "en-us, fr" etc.
> }
> 
> Clients that call ::Init() obviously already know about the derived class, so
> they can access that helper method to set these strings.

Why would this be more correct?

[erikwright (departed), 2012-09-25 21:06:38]

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:39: #include
"content/public/common/content_client.h"
no longer required?

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:79: // Used for the "system" URLRequestContext. If
this grows more complicated, then
Comment from "If this grows..." is no longer relevant?

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:119: scoped_ptr<BasicHttpUserAgentSettings>
basic_http_user_agent_settings;
Does this need to be the derived type?

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:169:
http_user_agent_settings_.reset(new BasicHttpUserAgentSettings(
I think this is also incorrectly overriding the ChromeURLRequestContext's
existing HttpUserAgentSettings.

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:16: #include
"chrome/browser/net/basic_http_user_agent_settings.h"
no longer required?

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.h (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.h:179: mutable
scoped_ptr<net::HttpUserAgentSettings> http_user_agent_settings_;
no longer required?

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:587:
context->OnAcceptLanguageChange(profile_params_->accept_language);
Is there actually any compelling reason for these to appear in profile_params_
now?

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_service.cc:617:
url_request_context_->CopyFrom(
Is this copying the HttpUserAgentSettings from the system context? If so (I
think it probably should) the above set_http_user_agent_settings is meaningless.

It should either be moved after line 618 or even removed (since it appears the
legacy behaviour would have been to _freeze_ the accept_charset/language values,
not to unset them, and it's probably OK if, instead of freezing, they continue
mirroring).

http://codereview.chromium.org/10918279/diff/53001/net/url_request/const_http...
File net/url_request/const_http_user_agent_settings.cc (right):

http://codereview.chromium.org/10918279/diff/53001/net/url_request/const_http...
net/url_request/const_http_user_agent_settings.cc:10: const std::string&
accept_language, const std::string& accept_charset,
wrap accept_charset

http://codereview.chromium.org/10918279/diff/53001/net/url_request/http_user_...
File net/url_request/http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/53001/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:16: // A pure-virtual interface that
provides the HTTP Accept-Language,
"The interface used by HTTP jobs to retrieve HTTP Accept-Language... header
values."

My personal opinion is that "An instance ... can be used..." is probably
unnecessary.

http://codereview.chromium.org/10918279/diff/53001/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:23: virtual const std::string&
GetAcceptLanguage() = 0;
Matt/Will, I'd like an opinion on this:

Returning a string reference seems error-prone and also restrictive. I suppose
an implementation might want to return a generated value, which would currently
be tricky.

I did a grep and found the following:

src$ git grep -E '^\s+std::string\s+[A-Z][a-zA-Z0-9_]*\(' | wc -l
818
src$ git grep -E '^\s+const\s+std::string&\s+[A-Z][a-zA-Z0-9_]*\(' | wc -l
75
src$ git grep -E '^\s+const\s+std::string&\s+[a-z][a-zA-Z0-9_]*\(' | wc -l
535
src$ git grep -E '^\s+std::string\s+[a-z][a-zA-Z0-9_]*\(' | wc -l
1786

So apart from accessors, the convention seems to be 10x in favor of value-type
and for accessors it's still 3x.

On the other hand, I guess the pre-existing API was already a const-ref so we
aren't introducing new risk. And I suppose these will be accessed relatively
frequently (though I tend to be in the camp of not pre-maturely optimizing).

http://codereview.chromium.org/10918279/diff/53001/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:24: // Gets the value of
'Accept-Charset' header field.
Blank line between declarations.

[pauljensen, 2012-09-26 14:08:19]

I addressed all of your second round comments either with the requested changes
or responses below.  Do we need to add mmenke/willchan to the reviewers or CC
list so they can respond to your question about returning "const std::string&"?

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:119: scoped_ptr<BasicHttpUserAgentSettings>
basic_http_user_agent_settings;
On 2012/09/25 21:06:38, erikwright wrote:
> Does this need to be the derived type?

It doesn't need to be but it seems like the most debugable choice.  I don't have
a preference otherwise.

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:587:
context->OnAcceptLanguageChange(profile_params_->accept_language);
On 2012/09/25 21:06:38, erikwright wrote:
> Is there actually any compelling reason for these to appear in profile_params_
> now?

They come from the prefs so I believe they must be read on the UI thread but
they need to be put into the contexts which live on the IO thread and I believe
profile_params_ is used for moving this sort of initialization parameters from
the UI to IO thread.

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_service.cc:617:
url_request_context_->CopyFrom(
On 2012/09/25 21:06:38, erikwright wrote:
> Is this copying the HttpUserAgentSettings from the system context? If so (I
> think it probably should) the above set_http_user_agent_settings is
meaningless.
> 
> It should either be moved after line 618 or even removed (since it appears the
> legacy behaviour would have been to _freeze_ the accept_charset/language
values,
> not to unset them, and it's probably OK if, instead of freezing, they continue
> mirroring).

I was inserting a BasicHttpUserAgentSettings to provide the GetUserAgent()
implementation when !system_url_request_context_getter.  Now that I think about
it, if system_url_request_context_getter is only NULL for tests and I'd guess
tests don't need a proper User-Agent, then I can just get rid of this.

[erikwright (departed), 2012-09-26 14:19:36]

LGTM.

mmenke, PTAL.

pauljensen: Will has asked to be CC'd on all these CLs anyway. Added him now,
but go ahead and include him from the beginning on future CLs.

mmenke and willchan: I'd appreciate comments on
http://codereview.chromium.org/10918279/diff/53001/net/url_request/http_user_...

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:119: scoped_ptr<BasicHttpUserAgentSettings>
basic_http_user_agent_settings;
On 2012/09/26 14:08:19, pauljensen wrote:
> On 2012/09/25 21:06:38, erikwright wrote:
> > Does this need to be the derived type?
> 
> It doesn't need to be but it seems like the most debugable choice.  I don't
have
> a preference otherwise.

OK. I think the convention would be to use the least derived type possible to
promote encapsulation. It also makes it easier to change the implementation
later.

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/10918279/diff/53001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_service.cc:617:
url_request_context_->CopyFrom(
On 2012/09/26 14:08:19, pauljensen wrote:
> On 2012/09/25 21:06:38, erikwright wrote:
> > Is this copying the HttpUserAgentSettings from the system context? If so (I
> > think it probably should) the above set_http_user_agent_settings is
> meaningless.
> > 
> > It should either be moved after line 618 or even removed (since it appears
the
> > legacy behaviour would have been to _freeze_ the accept_charset/language
> values,
> > not to unset them, and it's probably OK if, instead of freezing, they
continue
> > mirroring).
> 
> I was inserting a BasicHttpUserAgentSettings to provide the GetUserAgent()
> implementation when !system_url_request_context_getter.

I see. Good catch, actually.

> Now that I think about
> it, if system_url_request_context_getter is only NULL for tests and I'd guess
> tests don't need a proper User-Agent, then I can just get rid of this.

Yes, assuming that this doesn't cause any tests to fail, I agree we don't need
it.

[mmenke, 2012-09-26 16:28:20]

I want to carefully go over this once more for correctness, but didn't notice
anything significant on my first pass.

http://codereview.chromium.org/10918279/diff/53001/net/url_request/http_user_...
File net/url_request/http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/53001/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:23: virtual const std::string&
GetAcceptLanguage() = 0;
On 2012/09/25 21:06:38, erikwright wrote:
> Matt/Will, I'd like an opinion on this:
> 
> Returning a string reference seems error-prone and also restrictive. I suppose
> an implementation might want to return a generated value, which would
currently
> be tricky.
> 
> I did a grep and found the following:
> 
> src$ git grep -E '^\s+std::string\s+[A-Z][a-zA-Z0-9_]*\(' | wc -l
> 818
> src$ git grep -E '^\s+const\s+std::string&\s+[A-Z][a-zA-Z0-9_]*\(' | wc -l
> 75
> src$ git grep -E '^\s+const\s+std::string&\s+[a-z][a-zA-Z0-9_]*\(' | wc -l
> 535
> src$ git grep -E '^\s+std::string\s+[a-z][a-zA-Z0-9_]*\(' | wc -l
> 1786
> 
> So apart from accessors, the convention seems to be 10x in favor of value-type
> and for accessors it's still 3x.
> 
> On the other hand, I guess the pre-existing API was already a const-ref so we
> aren't introducing new risk. And I suppose these will be accessed relatively
> frequently (though I tend to be in the camp of not pre-maturely optimizing).

Don't think your grepping is quite correct - any method name that starts with a
lowercase letter, and then later has a capital in it is not following the Chrome
style guide.

Correcting the search and skimming over the hits, looks to me like a lot of the
functions are actually not simple accessors, so are violating the style guide,
and others are local variable declarations of the form "std::string
blah(source_string);".

That having been said, I think your concern is a valid one, and I don't think
performance is a concern here.  If we're going to do this change, think it's
best to do in this CL, and do it to all three functions.  That way, when we're
getting rid of class to the old URLRequestContext functions, we can update call
sites as needed to avoid extra copying.

http://codereview.chromium.org/10918279/diff/54002/chrome/browser/net/basic_h...
File chrome/browser/net/basic_http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/54002/chrome/browser/net/basic_h...
chrome/browser/net/basic_http_user_agent_settings.h:14: : public
net::ConstHttpAcceptLanguageAndCharset {
nit:  4 space indent.

http://codereview.chromium.org/10918279/diff/54002/chrome/service/net/service...
File chrome/service/net/service_url_request_context.cc (right):

http://codereview.chromium.org/10918279/diff/54002/chrome/service/net/service...
chrome/service/net/service_url_request_context.cc:145: "en-us,fr",
"iso-8859-1,*,utf-8", user_agent));
nit:  4-space indent

http://codereview.chromium.org/10918279/diff/54002/net/url_request/const_http...
File net/url_request/const_http_accept_language_and_charset.cc (right):

http://codereview.chromium.org/10918279/diff/54002/net/url_request/const_http...
net/url_request/const_http_accept_language_and_charset.cc:10: const std::string&
accept_language, const std::string& accept_charset)
This should be indented 4 spaces (The indentation of the next line is correct).

http://codereview.chromium.org/10918279/diff/54002/net/url_request/const_http...
File net/url_request/const_http_accept_language_and_charset.h (right):

http://codereview.chromium.org/10918279/diff/54002/net/url_request/const_http...
net/url_request/const_http_accept_language_and_charset.h:19: : public
HttpUserAgentSettings {
4 space indent.

http://codereview.chromium.org/10918279/diff/54002/net/url_request/const_http...
File net/url_request/const_http_user_agent_settings.cc (right):

http://codereview.chromium.org/10918279/diff/54002/net/url_request/const_http...
net/url_request/const_http_user_agent_settings.cc:12: const std::string&
user_agent)
4-space indent.

http://codereview.chromium.org/10918279/diff/54002/net/url_request/const_http...
File net/url_request/const_http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/54002/net/url_request/const_http...
net/url_request/const_http_user_agent_settings.h:16: : public
ConstHttpAcceptLanguageAndCharset {
nit:  4 space indent.

http://codereview.chromium.org/10918279/diff/54002/net/url_request/url_reques...
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/10918279/diff/54002/net/url_request/url_reques...
net/url_request/url_request_context.h:221: http_user_agent_settings_ =
http_user_agent_settings;
nit:  Fix indent.  HttpUserAgentSettings* should be indented by 4, and
http_user_agent_settings_ should be indented 2 less than that.

http://codereview.chromium.org/10918279/diff/54002/net/url_request/url_reques...
File net/url_request/url_request_context_builder.cc (right):

http://codereview.chromium.org/10918279/diff/54002/net/url_request/url_reques...
net/url_request/url_request_context_builder.cc:209: EmptyString(),
EmptyString(), user_agent_));
4-space indent

http://codereview.chromium.org/10918279/diff/54002/webkit/tools/test_shell/te...
File webkit/tools/test_shell/test_shell_request_context.cc (right):

http://codereview.chromium.org/10918279/diff/54002/webkit/tools/test_shell/te...
webkit/tools/test_shell/test_shell_request_context.cc:38: : public
net::ConstHttpAcceptLanguageAndCharset {
4-space indent

[pauljensen, 2012-10-05 15:03:20]

I fixed the indents...sorry about that, I switched editors.
I changed the HttpUserAgentSettings functions to return std::string by value
rather than const reference; along with this I changed the corresponding
UrlRequestContext function return types and names (as they are no longer simple
accessors).

[mmenke, 2012-10-05 20:08:51]

Just a couple small suggestions.

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/automation/...
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/automation/...
chrome/browser/automation/url_request_automation_job.cc:426:
!request_->context()->GetAcceptLanguage().empty()) {
Suggest you avoid the double virtual call / string copy here and below by moving
it to a local.

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:25: class BasicHttpUserAgentSettings;
nit:  Alphabetize.

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_service.cc:613:
system_url_request_context_getter->GetURLRequestContext());
While you're here, mind adding braces to this if statement?

http://codereview.chromium.org/10918279/diff/59001/net/url_request/http_user_...
File net/url_request/http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/59001/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:31: virtual ~HttpUserAgentSettings()
{}
nit:  Per style guide, constructor and destructor should go first.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:31: virtual ~HttpUserAgentSettings()
{}
private:
 DISALLOW_COPY_AND_ASSIGN(HttpUserAgentSettings);

and add #include "base/basictypes.h"

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
File net/url_request/url_request_context.cc (right):

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
net/url_request/url_request_context.cc:61:
set_http_user_agent_settings(other->http_user_agent_settings_);
nit:  Move below set_throttler_manager, so they're in the same order here as in
the header.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
File net/url_request/url_request_context_builder.cc (right):

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
net/url_request/url_request_context_builder.cc:136: }
Remove this.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
net/url_request/url_request_context_builder.cc:162: std::string user_agent_;
Remove this.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:469: if
(!context->GetAcceptLanguage().empty()) {
Suggest you cache both of these to a local to avoid the double virtual call /
string copy.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
File net/url_request/url_request_test_util.cc (right):

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
net/url_request/url_request_test_util.cc:121: }
This isn't quite equivalent to the old code, since these both always clear the
user agent.

Given how the new code works, I think it would make the most sense just to check
if context_storage_.http_user_agent_settings is NULL.  This would require
mucking with url_request_unittest.cc a bit, but that seems to be the only test
that cares.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:3337: ConstHttpUserAgentSettings
settings("en", EmptyString(), EmptyString());
Just out of paranoia, let's create the settings (And thus destroy them) before
the context.

This goes for the rest of the file, too.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:3369: 
Since we allow a context's UserAgentSettings to be NULL, how about a test or two
for that, too?

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

Anything that Erik/Matt says should have higher precedence over what I say here.
So if there's controversy, I defer to them. I think my comments make sense, so
hopefully they agree :) I've covered everything that I saw that concerned me, so
don't wait for any further comments from me.

A few high-level comments:
* HttpUserAgentSettings is a crappy name. Unfortunately, I have no better one to
suggest :) So it's fine as is, but maybe come up with a better one?
* HttpUserAgentSettings should be removed from the URLRequestContext eventually
and be put directly in the HttpProtocolHandler. I think I've provided
suggestions in the comments on how to do this in the future. I feel like it's
not much work, so it'd be great to do a followup changelist to accomplish this.
* URLRequestContextBuilder doesn't seem to build the URLRequestJobFactory with
the appropriate ProtocolHandlers. We should do that eventually, so
URLRequestContextBuilder can be usable.

This is a great refactor and a good step in cleaning up URLRequestContext,
thanks Paul!

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/automation/...
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/automation/...
chrome/browser/automation/url_request_automation_job.cc:423: // didn't have them
specified.
Is this code simply redundant? Doesn't URLRequestHttpJob already do this via
SetHeaderIfMissing()? If I'm right, can we simply delete this code?

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.cc:60: 
too many newlines

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.cc:384:
set_http_user_agent_settings(chrome_http_user_agent_settings_.get());
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.

http://codereview.chromium.org/10918279/diff/59001/content/browser/speech/goo...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

http://codereview.chromium.org/10918279/diff/59001/content/browser/speech/goo...
content/browser/speech/google_one_shot_remote_engine.cc:180: std::string
accepted_language_list = request_context->GetAcceptLanguage();
Long-term, I think we should construct this guy with a reference to the
HttpUserAgentSettings directly, rather than going through the URLRequestContext.
Leave a TODO?

Since you're new, I'll explain that a TODO doesn't mean that you are going to do
it, but leave your username so that people know who to ask about it. Or explain
it in a bug and link that.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/const_http...
File net/url_request/const_http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/59001/net/url_request/const_http...
net/url_request/const_http_user_agent_settings.h:12: // An implementation of
|HttpUserAgentSettings| that always provides the
Please name this StaticHttpUserAgentSettings or HttpUserAgentSettingsFixed, as
that's more consistent with existing classes like
StaticSocketDataProvider/StaticCookiePolicy/FixedNonceGenerator/ProxyConfigServiceFixed/etc.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/const_http...
net/url_request/const_http_user_agent_settings.h:16: : public
ConstHttpAcceptLanguageAndCharset {
I don't like implementation inheritance (and the style guide discourages it). Do
you mind using composition instead?

Actually, I looked at this and I think the code sharing is so minimal that I'd
rather just duplicate it and eliminate this extra
ConstHttpAcceptLanguageAndCharset helper class. Its only gain was eliminating
boilerplate when using implementation inheritance, but if we were to use
composition (I hate implementation inheritance enough to be willing to put up
with this minor boilerplate), then the win is too minor to be worth it.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:466: const URLRequestContext* context =
request_->context();
Long-term, we should pass in the HttpUserAgentSettings here directly, rather
than going through the context, right Erik? I wonder if we should just do it
here right now.

[mmenke, 2012-10-08 15:06:30]

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/automation/...
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/automation/...
chrome/browser/automation/url_request_automation_job.cc:423: // didn't have them
specified.
On 2012/10/06 19:32:49, willchan wrote:
> Is this code simply redundant? Doesn't URLRequestHttpJob already do this via
> SetHeaderIfMissing()? If I'm right, can we simply delete this code?

That's done in URLRequestJob::Start (Which calls
URLRequestHttpJob::AddExtraHeaders).  Doesn't look to me like that's called for
AutomationJobs.

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

This CL keeps getting better and better. I love seeing the amount of code that's
been pulled out of ChromeURLRequestContext.

One concern I have is the fact that ChromeHttpUserAgentSettings is now
refcounted. This seems wrong and unnecessary to me.

Also, perhaps it's just me, but when multiple interfaces need to be implemented
(i.e. NotificationObserver and HttpUserAgentSettings) I prefer composition.

I suspect you could solve both problems together by defining a second class to
take care of observing the prefs and forwarding them to the settings object. I
think this class could be an implementation detail of the settings object,
possibly a friend class.

If it used a WeakPtr to get back to the settings object you could probably avoid
the CleanupOnUIThread stuff too, and I think that this would qualify as a
constrained leaf node under the law of Will's recent WeakPtr rant.

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.cc:384:
set_http_user_agent_settings(chrome_http_user_agent_settings_.get());
On 2012/10/06 19:32:49, willchan wrote:
> 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.

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.

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10918279/diff/59001/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:466: const URLRequestContext* context =
request_->context();
On 2012/10/06 19:32:49, willchan wrote:
> Long-term, we should pass in the HttpUserAgentSettings here directly, rather
> than going through the context, right Erik? I wonder if we should just do it
> here right now.

Correct. I think it would make a good statement about the intended design if at
least this file used the new object exclusively.

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

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/10918279/diff/59001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.cc:384:
set_http_user_agent_settings(chrome_http_user_agent_settings_.get());
On 2012/10/09 15:24:46, erikwright wrote:
> On 2012/10/06 19:32:49, willchan wrote:
> > 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.
> 
> 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?

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

> 
> 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.

Great.

[pauljensen, 2012-10-23 22:02:58]

I reworked ChromeHttpUserAgentSettings to avoid being RefCounted and got rid of
all the notification logic by using StringPrefMember. 
chrome_http_user_agent_settings.cc is about half its previous size now.  I tried
dividing it into two classes but that turned into an ugly mess considering how
StringPrefMember works; I think you'll find it fairly simple and straightforward
now.

I think I've addressed all of willchan's comments.  There will need to be some
follow-up CLs to finish some of it.

I also made URLRequestHttpJob's constructor take a HttpUserAgentSettings object.
 Should I do the same for  URLRequestAutomationJob?

I noticed I wasn't quite updating referrer_charset correctly so I launched and
landed another CL to deal with that (crrev.com/163652).

Please take another look now that I think I've addressed all the pending
comments.

[willchan no longer on Chromium, 2012-10-24 03:40:09]

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/chrome_...
chrome/browser/net/chrome_http_user_agent_settings.h:24: virtual
~ChromeHttpUserAgentSettings() {}
Define it in the .cc please. This "empty" destructor will need to inline the
calls to teardown the vtable entries and invoke the parent destructor and also
inline all the std::string destructors for the member variables. Thus, it's
non-trivial and causes code bloat.

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:149: std::string accept_language =
net::HttpUtil::GenerateAcceptLanguageHeader(
This |accept_language| useless now?

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:152: std::string accept_charset =
Ditto with these two stack vars too.

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
File net/url_request/http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:25: virtual std::string
GetAcceptLanguage() = 0;
Is there a reason these aren't const? I think they should be. If they're lazily
initialized, they can use mutable (safe since this should be single threaded).
Getters should generally be const.

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
net/url_request/url_request_context.h:219: HttpUserAgentSettings*
http_user_agent_settings() const {
Can we return const HttpUserAgentSettings*, or do we need it to be non-const
somewhere?

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
net/url_request/url_request_context.h:258: HttpUserAgentSettings*
http_user_agent_settings_;
Please move this next to HttpServerProperties.

I'd like to have things *sort* of resemble an order where the lowest level
dependencies (like NetLog, which all net submodules use) are declared first, and
make their way up to the URLRequest level stuff like URLRequestJobFactory.

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
File net/url_request/url_request_context_storage.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
net/url_request/url_request_context_storage.h:97:
scoped_ptr<HttpUserAgentSettings> http_user_agent_settings_;
Move this earlier. The destruction order may matter here. This should definitely
be destroyed before URLRequestJobFactory, and probably before
HttpTransactionFactory.

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:223: http_user_agent_settings)
we don't format like this, move all the other parameters to their own indent,
instead of aligning with the '('.

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:256:
http_user_agent_settings_(http_user_agent_settings) {
Should we DCHECK() that it's non-NULL?

[erikwright (departed), 2012-10-24 13:14:33]

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:119: scoped_ptr<BasicHttpUserAgentSettings>
basic_http_user_agent_settings;
Does this need to be the derived type? Seems it could be
scoped_ptr<HttpUserAgentSettings>.

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/basic_h...
File chrome/browser/net/basic_http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/basic_h...
chrome/browser/net/basic_http_user_agent_settings.h:31:
DISALLOW_COPY_AND_ASSIGN(BasicHttpUserAgentSettings);
include basictypes.h

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_http_user_agent_settings.cc (right):

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/chrome_...
chrome/browser/net/chrome_http_user_agent_settings.cc:28:
pref_accept_language_.Destroy();
DCHECK on UI thread

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/chrome_...
chrome/browser/net/chrome_http_user_agent_settings.cc:33: std::string
new_pref_accept_language = *pref_accept_language_;
DCHECK on IO thread

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/chrome_...
chrome/browser/net/chrome_http_user_agent_settings.cc:43: std::string
new_pref_accept_charset = *pref_accept_charset_;
DCHECK on IO thread

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/net/chrome_...
chrome/browser/net/chrome_http_user_agent_settings.h:24: virtual
~ChromeHttpUserAgentSettings() {}
I think this should not be inline. It's presumably non-trivial due to non-POD
members.

Is it to be destroyed on IO or UI thread? Please specify in comment and DCHECK.

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:17: #include
"chrome/browser/net/chrome_http_user_agent_settings.h"
no longer used, right?

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.h (right):

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.h:187:
chrome_http_user_agent_settings_;
no longer used, right?

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:149: std::string accept_language =
net::HttpUtil::GenerateAcceptLanguageHeader(
Not required anymore, right?

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:152: std::string accept_charset =
ditto

http://codereview.chromium.org/10918279/diff/94002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:604:
context->set_referrer_charset(profile_params_->referrer_charset);
presumably this should go away? I guess you just need to sync and merge?

http://codereview.chromium.org/10918279/diff/94002/content/browser/speech/goo...
File content/browser/speech/google_streaming_remote_engine.cc (right):

http://codereview.chromium.org/10918279/diff/94002/content/browser/speech/goo...
content/browser/speech/google_streaming_remote_engine.cc:570: std::string
accepted_language_list = request_context->GetAcceptLanguage();
Presumably the TODO from the one-shot version also applies here?

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
File net/url_request/http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:35:
DISALLOW_COPY_AND_ASSIGN(HttpUserAgentSettings);
I'm pretty sure we don't require this for pure-virtual classes...

http://codereview.chromium.org/10918279/diff/94002/net/url_request/static_htt...
File net/url_request/static_http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/static_htt...
net/url_request/static_http_user_agent_settings.h:23: virtual
~StaticHttpUserAgentSettings() {}
don't inline.

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:481: std::string accept_language =
http_user_agent_settings_->GetAcceptLanguage();
It seems from the unit test that the settings object is nullable... so should we
be checking for null here?

http://codereview.chromium.org/10918279/diff/94002/sync/internal_api/public/h...
File sync/internal_api/public/http_bridge.h (right):

http://codereview.chromium.org/10918279/diff/94002/sync/internal_api/public/h...
sync/internal_api/public/http_bridge.h:65:
scoped_ptr<net::StaticHttpUserAgentSettings>
This could be just HttpUserAgentSettings, right?

[erikwright (departed), 2012-10-24 13:15:21]

On 2012/10/23 22:02:58, pauljensen wrote:
> I reworked ChromeHttpUserAgentSettings to avoid being RefCounted and got rid
of
> all the notification logic by using StringPrefMember. 
> chrome_http_user_agent_settings.cc is about half its previous size now.  I
tried
> dividing it into two classes but that turned into an ugly mess considering how
> StringPrefMember works; I think you'll find it fairly simple and
straightforward
> now.

Yes, looks pretty good now.

> 
> I think I've addressed all of willchan's comments.  There will need to be some
> follow-up CLs to finish some of it.
> 
> I also made URLRequestHttpJob's constructor take a HttpUserAgentSettings
object.
>  Should I do the same for  URLRequestAutomationJob?

Please, yes.

> I noticed I wasn't quite updating referrer_charset correctly so I launched and
> landed another CL to deal with that (crrev.com/163652).
> 
> Please take another look now that I think I've addressed all the pending
> comments.

[pauljensen, 2012-10-24 19:18:59]

I think I addressed Erik and Will's comments.  I also made
URLRequestAutomationJob access HttpUserAgentSettings directly rather than via
the URLRequestContext.

There was some disagreement about whether we should support a NULL
HttpUserAgentSettings* in URLRequestHttpJob.  Will wondered if we should
DCHECK() to make sure its non-NULL while Erik suggested making it NULL-able
noting that the new unit test verified this behavior.  I don't think
HttpUserAgentSettings should be required for an HTTP request as the headers are
not required so I made it NULL-able.

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
File net/url_request/http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:25: virtual std::string
GetAcceptLanguage() = 0;
On 2012/10/24 03:40:10, willchan wrote:
> Is there a reason these aren't const? I think they should be. If they're
lazily
> initialized, they can use mutable (safe since this should be single threaded).
> Getters should generally be const.

Now that these are not simple accessors (hence the change in name and return
type) we want the freedom to calculate the values dynamically which may require
writing to members.  The ChromeHttpUserAgentSettings getters are examples of
this, they cannot be const as they write to members to cache the results.  They
are not lazily initialized so I don't think mutable is appropriate.

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:35:
DISALLOW_COPY_AND_ASSIGN(HttpUserAgentSettings);
On 2012/10/24 13:14:33, erikwright wrote:
> I'm pretty sure we don't require this for pure-virtual classes...

You suggested I take this out, then mmenke suggested I put it back.  The style
guide makes a pretty blanket statement that this should be used so I went with
mmenke's suggestion.  Is there a big downside to keeping it that I'm not seeing?

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
net/url_request/url_request_context.h:219: HttpUserAgentSettings*
http_user_agent_settings() const {
On 2012/10/24 03:40:10, willchan wrote:
> Can we return const HttpUserAgentSettings*, or do we need it to be non-const
> somewhere?

If the getters are not const, then this cannot be const.

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
File net/url_request/url_request_context_storage.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/url_reques...
net/url_request/url_request_context_storage.h:97:
scoped_ptr<HttpUserAgentSettings> http_user_agent_settings_;
On 2012/10/24 03:40:10, willchan wrote:
> Move this earlier. The destruction order may matter here. This should
definitely
> be destroyed before URLRequestJobFactory, and probably before
> HttpTransactionFactory.

By moving it earlier it will be destroyed *after* more items, not before.  I'm
guessing this is what you wanted? (so HttpUserAgentSettings dies after any
callers, avoiding use after free)

[mmenke, 2012-10-24 19:25:22]

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
File net/url_request/http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:35:
DISALLOW_COPY_AND_ASSIGN(HttpUserAgentSettings);
On 2012/10/24 19:19:00, pauljensen wrote:
> On 2012/10/24 13:14:33, erikwright wrote:
> > I'm pretty sure we don't require this for pure-virtual classes...
> 
> You suggested I take this out, then mmenke suggested I put it back.  The style
> guide makes a pretty blanket statement that this should be used so I went with
> mmenke's suggestion.  Is there a big downside to keeping it that I'm not
seeing?

My opinion is that it's easy for pure virtual classes to become non-pure
virtual, and to overlook adding it, so simpler to have it everywhere except when
copy and assign are desired.

The less thinking one has to do, the less often one break things.  There's a
life lesson somewhere in there, I'm sure.

[willchan no longer on Chromium, 2012-10-24 19:30:08]

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
File net/url_request/http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:25: virtual std::string
GetAcceptLanguage() = 0;
On 2012/10/24 19:19:00, pauljensen wrote:
> On 2012/10/24 03:40:10, willchan wrote:
> > Is there a reason these aren't const? I think they should be. If they're
> lazily
> > initialized, they can use mutable (safe since this should be single
threaded).
> > Getters should generally be const.
> 
> Now that these are not simple accessors (hence the change in name and return
> type) we want the freedom to calculate the values dynamically which may
require
> writing to members.  The ChromeHttpUserAgentSettings getters are examples of
> this, they cannot be const as they write to members to cache the results. 
They
> are not lazily initialized so I don't think mutable is appropriate.

I'm not sure I agree from a logical constness perspective. I think you may be
too embroiled in the actual implementation details. Take a look at it from the
user perspective...is there an ordering or something of calls to these getters
that would alter the results? I was too strict before when I said lazy
initialization, a more generalized way to say it would be lazy evaluation. It
sounds to me like you're just doing lazy evaluation, which is what mutable is
exactly there for (but be careful with multithreaded cases, which doesn't apply
here since this should be single threaded).

[pauljensen, 2012-10-24 20:21:29]

On 2012/10/24 19:30:08, willchan wrote:
>
http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
> File net/url_request/http_user_agent_settings.h (right):
> 
>
http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
> net/url_request/http_user_agent_settings.h:25: virtual std::string
> GetAcceptLanguage() = 0;
> On 2012/10/24 19:19:00, pauljensen wrote:
> > On 2012/10/24 03:40:10, willchan wrote:
> > > Is there a reason these aren't const? I think they should be. If they're
> > lazily
> > > initialized, they can use mutable (safe since this should be single
> threaded).
> > > Getters should generally be const.
> > 
> > Now that these are not simple accessors (hence the change in name and return
> > type) we want the freedom to calculate the values dynamically which may
> require
> > writing to members.  The ChromeHttpUserAgentSettings getters are examples of
> > this, they cannot be const as they write to members to cache the results. 
> They
> > are not lazily initialized so I don't think mutable is appropriate.
> 
> I'm not sure I agree from a logical constness perspective. I think you may be
> too embroiled in the actual implementation details. Take a look at it from the
> user perspective...is there an ordering or something of calls to these getters
> that would alter the results? I was too strict before when I said lazy
> initialization, a more generalized way to say it would be lazy evaluation. It
> sounds to me like you're just doing lazy evaluation, which is what mutable is
> exactly there for (but be careful with multithreaded cases, which doesn't
apply
> here since this should be single threaded).

I've come around and buy into the logical constness argument now.  I guess I was
just going by the style guide which says "Accessors should almost always be
const. Other methods should be const if they do not modify any data members..." 
The mutable keyword makes me feel dirty but I can live with that considering at
a higher level the results of the getters aren't affected.  I made the getters
and result of http_user_agent_settings() const.

[willchan no longer on Chromium, 2012-10-24 21:18:54]

Btw, I'm fine keeping them CamelCased if you prefer, since they're virtual.
I don't much care. Someone always cares though. I think there's something
on chromium-dev@ about virtual accessors. I'm just going to tell you that I
don't care, but someone may bug you in the future. I probably should
re-read the chromium-dev@ thread so I can do whatever people who cared
wanted.


On Wed, Oct 24, 2012 at 1:21 PM, <pauljensen@chromium.org> wrote:

> On 2012/10/24 19:30:08, willchan wrote:
>
> http://codereview.chromium.**org/10918279/diff/94002/net/**
>
url_request/http_user_agent_**settings.h<http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_agent_settings.h>
>
>> File net/url_request/http_user_**agent_settings.h (right):
>>
>
>
> http://codereview.chromium.**org/10918279/diff/94002/net/**
>
url_request/http_user_agent_**settings.h#newcode25<http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_agent_settings.h#newcode25>
>
>> net/url_request/http_user_**agent_settings.h:25: virtual std::string
>> GetAcceptLanguage() = 0;
>> On 2012/10/24 19:19:00, pauljensen wrote:
>> > On 2012/10/24 03:40:10, willchan wrote:
>> > > Is there a reason these aren't const? I think they should be. If
>> they're
>> > lazily
>> > > initialized, they can use mutable (safe since this should be single
>> threaded).
>> > > Getters should generally be const.
>> >
>> > Now that these are not simple accessors (hence the change in name and
>> return
>> > type) we want the freedom to calculate the values dynamically which may
>> require
>> > writing to members.  The ChromeHttpUserAgentSettings getters are
>> examples of
>> > this, they cannot be const as they write to members to cache the
>> results.
>> They
>> > are not lazily initialized so I don't think mutable is appropriate.
>>
>
>  I'm not sure I agree from a logical constness perspective. I think you
>> may be
>> too embroiled in the actual implementation details. Take a look at it
>> from the
>> user perspective...is there an ordering or something of calls to these
>> getters
>> that would alter the results? I was too strict before when I said lazy
>> initialization, a more generalized way to say it would be lazy
>> evaluation. It
>> sounds to me like you're just doing lazy evaluation, which is what
>> mutable is
>> exactly there for (but be careful with multithreaded cases, which doesn't
>>
> apply
>
>> here since this should be single threaded).
>>
>
> I've come around and buy into the logical constness argument now.  I guess
> I was
> just going by the style guide which says "Accessors should almost always be
> const. Other methods should be const if they do not modify any data
> members..."
> The mutable keyword makes me feel dirty but I can live with that
> considering at
> a higher level the results of the getters aren't affected.  I made the
> getters
> and result of http_user_agent_settings() const.
>
>
http://codereview.chromium.**org/10918279/<http://codereview.chromium.org/109...
>

[mmenke, 2012-10-24 21:22:18]

On 2012/10/24 21:18:54, willchan wrote:
> Btw, I'm fine keeping them CamelCased if you prefer, since they're virtual.
> I don't much care. Someone always cares though. I think there's something
> on chromium-dev@ about virtual accessors. I'm just going to tell you that I
> don't care, but someone may bug you in the future. I probably should
> re-read the chromium-dev@ thread so I can do whatever people who cared
> wanted.

I thought the conclusion was virtual accessors should be camel case.

[pauljensen, 2012-10-25 12:59:19]

On 2012/10/24 21:22:18, Matt Menke wrote:
> On 2012/10/24 21:18:54, willchan wrote:
> > Btw, I'm fine keeping them CamelCased if you prefer, since they're virtual.
> > I don't much care. Someone always cares though. I think there's something
> > on chromium-dev@ about virtual accessors. I'm just going to tell you that I
> > don't care, but someone may bug you in the future. I probably should
> > re-read the chromium-dev@ thread so I can do whatever people who cared
> > wanted.
> 
> I thought the conclusion was virtual accessors should be camel case.

I'm guessing this is what you guys are referring to:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/CzXaUIR3GIo/dis...
I'm not sure there is a clear conclusion to the thread other than to abide by
the style guide.  Erik and I decided to change to CamelCase because these are no
longer simple accessor functions like the original inlined URLRequestContext
functions, which I believe is in accordance with the style guide.

[mmenke, 2012-10-25 14:15:41]

On 2012/10/25 12:59:19, pauljensen wrote:
> On 2012/10/24 21:22:18, Matt Menke wrote:
> > On 2012/10/24 21:18:54, willchan wrote:
> > > Btw, I'm fine keeping them CamelCased if you prefer, since they're
virtual.
> > > I don't much care. Someone always cares though. I think there's something
> > > on chromium-dev@ about virtual accessors. I'm just going to tell you that
I
> > > don't care, but someone may bug you in the future. I probably should
> > > re-read the chromium-dev@ thread so I can do whatever people who cared
> > > wanted.
> > 
> > I thought the conclusion was virtual accessors should be camel case.
> 
> I'm guessing this is what you guys are referring to:
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/CzXaUIR3GIo/dis...
> I'm not sure there is a clear conclusion to the thread other than to abide by
> the style guide.  Erik and I decided to change to CamelCase because these are
no
> longer simple accessor functions like the original inlined URLRequestContext
> functions, which I believe is in accordance with the style guide.

Almost everyone agreed on using CammelCase for virtual functions.  Only one
person argued against it, and he "Wasn't really suggesting we change [it]."  I'd
call that a consensus on what style should be technically used (Though not a
consensus to be dogmatic about it).

[mmenke, 2012-10-25 18:06:32]

http://codereview.chromium.org/10918279/diff/117003/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/117003/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:50: 
nit:  Don't think this blank line is really needed any more.

http://codereview.chromium.org/10918279/diff/117003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/117003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:582:
chrome_http_user_agent_settings_.get());
nit:  4 space indent.  Actually, can this all fit on one line?

http://codereview.chromium.org/10918279/diff/117003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:667:
chrome_http_user_agent_settings_->CleanupOnUIThread();
Doesn't this always need to be called?  Otherwise, it will access prefs_ on the
IO thread when it's deleted there.

Also, I don't believe this is threadsafe for another reason. 
chrome_http_user_agent_settings_ is initialized on the IO thread, and here we're
accessin it on the UI thread.  Is there any guarantee we don't have a
LazyInitialize() in progress when we reach this point?

Think you can just set chrome_http_user_agent_settings_ on construction.

http://codereview.chromium.org/10918279/diff/117003/net/url_request/static_ht...
File net/url_request/static_http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/117003/net/url_request/static_ht...
net/url_request/static_http_user_agent_settings.h:10: #include
"base/compiler_specific.h"
nit:  Should probably be including basictypes as well, for
DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/10918279/diff/117003/net/url_request/url_reque...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10918279/diff/117003/net/url_request/url_reque...
net/url_request/url_request_http_job.h:43: 
nit:  Remove extra blank line.

http://codereview.chromium.org/10918279/diff/117003/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10918279/diff/117003/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:3500: StaticHttpUserAgentSettings
settings("", EmptyString(), EmptyString());
Shouldn't "" just be EmptyString()?  Suppose using "" does draw attention to it,
but that's what the test name is for, I'd assume.  Sames goes for
EmptyAcceptCharset.

http://codereview.chromium.org/10918279/diff/117003/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:3660: // headers to be sent.
That should be "corresponding header to not be sent", right?  At least it
doesn't look to me like we automatically add Language/Charset headers with a
value of "None".

[pauljensen, 2012-10-25 21:30:24]

I addressed mmenke's comments.

http://codereview.chromium.org/10918279/diff/117003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/117003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:667:
chrome_http_user_agent_settings_->CleanupOnUIThread();
On 2012/10/25 18:06:33, Matt Menke wrote:
> Doesn't this always need to be called?  Otherwise, it will access prefs_ on
the
> IO thread when it's deleted there.
> 
> Also, I don't believe this is threadsafe for another reason. 
> chrome_http_user_agent_settings_ is initialized on the IO thread, and here
we're
> accessin it on the UI thread.  Is there any guarantee we don't have a
> LazyInitialize() in progress when we reach this point?
> 
> Think you can just set chrome_http_user_agent_settings_ on construction.

I don't think we can construct ChromeHttpUserAgentSettings in ProfileIOData()
because the PrefService isn't constructed yet.  There are several alternatives:
1. We could construct ChromeHttpUserAgentSettings in ProfileIOData() and add
ChromeHttpUserAgentSettings::InitAfterPrefsLoaded() that we could call later.
2. We could construct ChromeHttpUserAgentSettings in
ProfileIODataImpl::Handle::Init() which is called from 
ProfileImpl::OnPrefsLoaded() but there isn't a corresponding 
OffTheRecordProfileImpl::Handle::Init().
3. We could construct ChromeHttpUserAgentSettings and set
chrome_http_user_agent_settings_ directly in
ProfileIOData::InitializeOnUIThread() instead of passing through the
profile_params_.  This way it can be initialized next to the other PrefMember
members of ProfileIOData.  This should avoid the threading issues.
4. We could yank the StringPrefMember's out of ChromeHttpUserAgentSettings and
put them in ProfileIOData, similar to enable_referrers_, and pass their
addresses into ChromeHttpUserAgentSettings().
I've implemented #3 because it seems to be the simplest and most
straightforward.

http://codereview.chromium.org/10918279/diff/117003/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10918279/diff/117003/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:3660: // headers to be sent.
On 2012/10/25 18:06:33, Matt Menke wrote:
> That should be "corresponding header to not be sent", right?  At least it
> doesn't look to me like we automatically add Language/Charset headers with a
> value of "None".

Its a mix of both empty or missing headers.  We always send a User-Agent header
even if empty but we don't send Accept-Language or Accept-Charset headers.  I've
adjusted the comment.

[mmenke, 2012-10-26 18:42:08]

LGTM

http://codereview.chromium.org/10918279/diff/117003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10918279/diff/117003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:667:
chrome_http_user_agent_settings_->CleanupOnUIThread();
On 2012/10/25 21:30:25, pauljensen wrote:
> On 2012/10/25 18:06:33, Matt Menke wrote:
> > Doesn't this always need to be called?  Otherwise, it will access prefs_ on
> the
> > IO thread when it's deleted there.
> > 
> > Also, I don't believe this is threadsafe for another reason. 
> > chrome_http_user_agent_settings_ is initialized on the IO thread, and here
> we're
> > accessin it on the UI thread.  Is there any guarantee we don't have a
> > LazyInitialize() in progress when we reach this point?
> > 
> > Think you can just set chrome_http_user_agent_settings_ on construction.
> 
> I don't think we can construct ChromeHttpUserAgentSettings in ProfileIOData()
> because the PrefService isn't constructed yet.  There are several
alternatives:
> 1. We could construct ChromeHttpUserAgentSettings in ProfileIOData() and add
> ChromeHttpUserAgentSettings::InitAfterPrefsLoaded() that we could call later.
> 2. We could construct ChromeHttpUserAgentSettings in
> ProfileIODataImpl::Handle::Init() which is called from 
> ProfileImpl::OnPrefsLoaded() but there isn't a corresponding 
> OffTheRecordProfileImpl::Handle::Init().
> 3. We could construct ChromeHttpUserAgentSettings and set
> chrome_http_user_agent_settings_ directly in
> ProfileIOData::InitializeOnUIThread() instead of passing through the
> profile_params_.  This way it can be initialized next to the other PrefMember
> members of ProfileIOData.  This should avoid the threading issues.
> 4. We could yank the StringPrefMember's out of ChromeHttpUserAgentSettings and
> put them in ProfileIOData, similar to enable_referrers_, and pass their
> addresses into ChromeHttpUserAgentSettings().
> I've implemented #3 because it seems to be the simplest and most
> straightforward.

Sounds like the best approach to me, too.

[erikwright (departed), 2012-10-26 21:10:45]

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
File net/url_request/http_user_agent_settings.h (right):

http://codereview.chromium.org/10918279/diff/94002/net/url_request/http_user_...
net/url_request/http_user_agent_settings.h:35:
DISALLOW_COPY_AND_ASSIGN(HttpUserAgentSettings);
On 2012/10/24 19:25:22, Matt Menke wrote:
> On 2012/10/24 19:19:00, pauljensen wrote:
> > On 2012/10/24 13:14:33, erikwright wrote:
> > > I'm pretty sure we don't require this for pure-virtual classes...
> > 
> > You suggested I take this out, then mmenke suggested I put it back.  The
style
> > guide makes a pretty blanket statement that this should be used so I went
with
> > mmenke's suggestion.  Is there a big downside to keeping it that I'm not
> seeing?
> 
> My opinion is that it's easy for pure virtual classes to become non-pure
> virtual, and to overlook adding it, so simpler to have it everywhere except
when
> copy and assign are desired.
> 
> The less thinking one has to do, the less often one break things.  There's a
> life lesson somewhere in there, I'm sure.

SGTM.

http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome...
File chrome/browser/net/chrome_http_user_agent_settings.cc (right):

http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome...
chrome/browser/net/chrome_http_user_agent_settings.cc:32:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Is there any kind of guarantee that GetAcceptLanguage or GetAcceptCharset won't
be called anymore?

Technically PrefMember says you shouldn't "use" it after calling Destroy.

[pauljensen, 2012-10-26 22:04:14]

http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome...
File chrome/browser/net/chrome_http_user_agent_settings.cc (right):

http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome...
chrome/browser/net/chrome_http_user_agent_settings.cc:32:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2012/10/26 21:10:46, erikwright wrote:
> Is there any kind of guarantee that GetAcceptLanguage or GetAcceptCharset
won't
> be called anymore?
No.

> 
> Technically PrefMember says you shouldn't "use" it after calling Destroy.
True.  Given the current implementation it will work fine after calling
Destroy() (given that we do unconditionally call operator*() prior to this) but
we're going against the comment so there's no guarantee this will continue to
work.  Sadly I see no test for PrefMember::Destroy() at all :(

I wonder if this is a problem for the other PrefMembers in ProfileIOData.
Possible resolutions:
1. Add some sort of guarantee we don't read the PrefMembers after Destroy()ing
them.  This would be easy and cheap if we had the ThreadSafeBool class I've
wanted.
2. Change PrefMember::Destroy() comments and add a unit test for this behavior. 
This is a little icky because I think it only continues to work if you've called
GetValue() prior.

[mmenke, 2012-10-26 22:22:15]

On 2012/10/26 22:04:14, pauljensen wrote:
>
http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome...
> File chrome/browser/net/chrome_http_user_agent_settings.cc (right):
> 
>
http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome...
> chrome/browser/net/chrome_http_user_agent_settings.cc:32:
> DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
> On 2012/10/26 21:10:46, erikwright wrote:
> > Is there any kind of guarantee that GetAcceptLanguage or GetAcceptCharset
> won't
> > be called anymore?
> No.
> 
> > 
> > Technically PrefMember says you shouldn't "use" it after calling Destroy.
> True.  Given the current implementation it will work fine after calling
> Destroy() (given that we do unconditionally call operator*() prior to this)
but
> we're going against the comment so there's no guarantee this will continue to
> work.  Sadly I see no test for PrefMember::Destroy() at all :(
> 
> I wonder if this is a problem for the other PrefMembers in ProfileIOData.
> Possible resolutions:
> 1. Add some sort of guarantee we don't read the PrefMembers after Destroy()ing
> them.  This would be easy and cheap if we had the ThreadSafeBool class I've
> wanted.
> 2. Change PrefMember::Destroy() comments and add a unit test for this
behavior. 
> This is a little icky because I think it only continues to work if you've
called
> GetValue() prior.

Skimming over it, it looks to me like MoveToThread does the equivalent of that
first GetValue() call.

If this usage really isn't safe, MoveToThread in general isn't terribly safe -
it can't be used on anything that's not either protected by a mutex on both
thread, or on a thread that's merged with the main thread on quit.

[mmenke, 2012-10-26 22:23:55]

On 2012/10/26 22:22:15, Matt Menke wrote:
> On 2012/10/26 22:04:14, pauljensen wrote:
> >
>
http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome...
> > File chrome/browser/net/chrome_http_user_agent_settings.cc (right):
> > 
> >
>
http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome...
> > chrome/browser/net/chrome_http_user_agent_settings.cc:32:
> > DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
> > On 2012/10/26 21:10:46, erikwright wrote:
> > > Is there any kind of guarantee that GetAcceptLanguage or GetAcceptCharset
> > won't
> > > be called anymore?
> > No.
> > 
> > > 
> > > Technically PrefMember says you shouldn't "use" it after calling Destroy.
> > True.  Given the current implementation it will work fine after calling
> > Destroy() (given that we do unconditionally call operator*() prior to this)
> but
> > we're going against the comment so there's no guarantee this will continue
to
> > work.  Sadly I see no test for PrefMember::Destroy() at all :(
> > 
> > I wonder if this is a problem for the other PrefMembers in ProfileIOData.
> > Possible resolutions:
> > 1. Add some sort of guarantee we don't read the PrefMembers after
Destroy()ing
> > them.  This would be easy and cheap if we had the ThreadSafeBool class I've
> > wanted.
> > 2. Change PrefMember::Destroy() comments and add a unit test for this
> behavior. 
> > This is a little icky because I think it only continues to work if you've
> called
> > GetValue() prior.
> 
> Skimming over it, it looks to me like MoveToThread does the equivalent of that
> first GetValue() call.
> 
> If this usage really isn't safe, MoveToThread in general isn't terribly safe -
> it can't be used on anything that's not either protected by a mutex on both
> thread, or on a thread that's merged with the main thread on quit.

Oh, thread merging wouldn't really work, unless the thread was merged before the
destroy call, which would be a little weird, and kinda negate the purpose for
the destroy call in the first place.

[erikwright (departed), 2012-10-26 23:30:33]

I suggest adding tests, comments, and any changes necessary to make it safe
to use after destroy (although obviously no longer actually linked to a
preference). A separate CL, I guess.

But it wouldn't hurt to first rope in an owner of prefs. I feel like we
must somehow be overlooking something because this class really doesn't
seem useful without that feature.

On Fri, Oct 26, 2012 at 6:23 PM, <mmenke@chromium.org> wrote:

> On 2012/10/26 22:22:15, Matt Menke wrote:
>
>> On 2012/10/26 22:04:14, pauljensen wrote:
>> >
>>
>
> http://codereview.chromium.**org/10918279/diff/102019/**
>
chrome/browser/net/chrome_**http_user_agent_settings.cc<http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome_http_user_agent_settings.cc>
>
>> > File chrome/browser/net/chrome_**http_user_agent_settings.cc (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/10918279/diff/102019/**
>
chrome/browser/net/chrome_**http_user_agent_settings.cc#**newcode32<http://codereview.chromium.org/10918279/diff/102019/chrome/browser/net/chrome_http_user_agent_settings.cc#newcode32>
>
>> > chrome/browser/net/chrome_**http_user_agent_settings.cc:**32:
>> > DCHECK(content::BrowserThread:**:CurrentlyOn(content::**
>> BrowserThread::UI));
>> > On 2012/10/26 21:10:46, erikwright wrote:
>> > > Is there any kind of guarantee that GetAcceptLanguage or
>> GetAcceptCharset
>> > won't
>> > > be called anymore?
>> > No.
>> >
>> > >
>> > > Technically PrefMember says you shouldn't "use" it after calling
>> Destroy.
>> > True.  Given the current implementation it will work fine after calling
>> > Destroy() (given that we do unconditionally call operator*() prior to
>> this)
>> but
>> > we're going against the comment so there's no guarantee this will
>> continue
>>
> to
>
>> > work.  Sadly I see no test for PrefMember::Destroy() at all :(
>> >
>> > I wonder if this is a problem for the other PrefMembers in
>> ProfileIOData.
>> > Possible resolutions:
>> > 1. Add some sort of guarantee we don't read the PrefMembers after
>>
> Destroy()ing
>
>> > them.  This would be easy and cheap if we had the ThreadSafeBool class
>> I've
>> > wanted.
>> > 2. Change PrefMember::Destroy() comments and add a unit test for this
>> behavior.
>> > This is a little icky because I think it only continues to work if
>> you've
>> called
>> > GetValue() prior.
>>
>
>  Skimming over it, it looks to me like MoveToThread does the equivalent of
>> that
>> first GetValue() call.
>>
>
>  If this usage really isn't safe, MoveToThread in general isn't terribly
>> safe -
>> it can't be used on anything that's not either protected by a mutex on
>> both
>> thread, or on a thread that's merged with the main thread on quit.
>>
>
> Oh, thread merging wouldn't really work, unless the thread was merged
> before the
> destroy call, which would be a little weird, and kinda negate the purpose
> for
> the destroy call in the first place.
>
>
http://codereview.chromium.**org/10918279/<http://codereview.chromium.org/109...
>

[pauljensen, 2012-10-31 00:34:49]

The change to the comment and test for PrefMember::Destroy() has landed
(http://crrev.com/165066).

[erikwright (departed), 2012-10-31 00:38:43]

LGTM.

[erikwright (departed), 2012-10-31 00:39:11]

Actually, LVGTM. This is a big improvement!

[mmenke, 2012-10-31 00:51:14]

On 2012/10/31 00:39:11, erikwright wrote:
> Actually, LVGTM. This is a big improvement!

Still LGTM as well.

[pauljensen, 2012-10-31 14:06:48]

Brett, could you please review the chrome/, content/ and webkit/ changes? 
They're small but plentiful.  You can skip the chrome/browser/net/ changes and
chrome/browser/profiles/profile_io_data.* changes if this makes life easier.
Tim, could you please review the sync/ changes?
Thank you in advance!

[pauljensen, 2012-10-31 17:34:36]

Jonathan, could you review
android_webview/browser/net/aw_url_request_context_getter.cc please?

[joth, 2012-11-01 12:04:55]

lgtm

[brettw, 2012-11-05 05:42:38]

LGTM I didn't check the details.

[tim (not reviewing), 2012-11-05 18:53:46]

sync/ LGTM

[commit-bot: I haz the power, 2012-11-06 12:42:36]

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

[commit-bot: I haz the power, 2012-11-06 12:42:45]

Failed to apply patch for chrome/browser/net/chrome_url_request_context.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/net/chrome_url_request_context.cc
  Hunk #10 FAILED at 264.
  Hunk #11 succeeded at 360 (offset -8 lines).
  1 out of 11 hunks FAILED -- saving rejects to file
chrome/browser/net/chrome_url_request_context.cc.rej

Patch:       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
8cb7c51b811f5fa796932cdd6e4a8877f28c2c8a..677ed0a7ca76103e7072a6364f309bbe6b5f7611
100644
--- a/chrome/browser/net/chrome_url_request_context.cc
+++ b/chrome/browser/net/chrome_url_request_context.cc
@@ -11,18 +11,11 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/io_thread.h"
 #include "chrome/browser/net/load_time_stats.h"
-#include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/profiles/storage_partition_descriptor.h"
-#include "chrome/common/chrome_notification_types.h"
-#include "chrome/common/pref_names.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_source.h"
-#include "content/public/common/content_client.h"
 #include "net/cookies/cookie_store.h"
-#include "net/http/http_util.h"
 
 using content::BrowserThread;
 
@@ -156,19 +149,12 @@ class FactoryForMedia : public
ChromeURLRequestContextFactory {
 // ----------------------------------------------------------------------------
 
 ChromeURLRequestContextGetter::ChromeURLRequestContextGetter(
-    Profile* profile,
     ChromeURLRequestContextFactory* factory)
     : factory_(factory) {
   DCHECK(factory);
-  DCHECK(profile);
-  RegisterPrefsObserver(profile);
 }
 
-ChromeURLRequestContextGetter::~ChromeURLRequestContextGetter() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
-  DCHECK(registrar_.IsEmpty()) << "Probably didn't call CleanupOnUIThread";
-}
+ChromeURLRequestContextGetter::~ChromeURLRequestContextGetter() {}
 
 // Lazily create a ChromeURLRequestContext using our factory.
 net::URLRequestContext* ChromeURLRequestContextGetter::GetURLRequestContext() {
@@ -198,7 +184,6 @@ ChromeURLRequestContextGetter*
ChromeURLRequestContextGetter::CreateOriginal(
     const ProfileIOData* profile_io_data) {
   DCHECK(!profile->IsOffTheRecord());
   return new ChromeURLRequestContextGetter(
-      profile,
       new FactoryForMain(profile_io_data));
 }
 
@@ -208,7 +193,6 @@ ChromeURLRequestContextGetter::CreateOriginalForMedia(
     Profile* profile, const ProfileIOData* profile_io_data) {
   DCHECK(!profile->IsOffTheRecord());
   return new ChromeURLRequestContextGetter(
-      profile,
       new FactoryForMedia(profile_io_data));
 }
 
@@ -218,7 +202,6 @@ ChromeURLRequestContextGetter::CreateOriginalForExtensions(
     Profile* profile, const ProfileIOData* profile_io_data) {
   DCHECK(!profile->IsOffTheRecord());
   return new ChromeURLRequestContextGetter(
-      profile,
       new FactoryForExtensions(profile_io_data));
 }
 
@@ -234,7 +217,6 @@ ChromeURLRequestContextGetter::CreateOriginalForIsolatedApp(
   ChromeURLRequestContextGetter* main_context =
      
static_cast<ChromeURLRequestContextGetter*>(profile->GetRequestContext());
   return new ChromeURLRequestContextGetter(
-      profile,
       new FactoryForIsolatedApp(profile_io_data, partition_descriptor,
            main_context, protocol_handler_interceptor.Pass()));
 }
@@ -248,7 +230,6 @@
ChromeURLRequestContextGetter::CreateOriginalForIsolatedMedia(
     const StoragePartitionDescriptor& partition_descriptor) {
   DCHECK(!profile->IsOffTheRecord());
   return new ChromeURLRequestContextGetter(
-      profile,
       new FactoryForIsolatedMedia(
           profile_io_data, partition_descriptor, app_context));
 }
@@ -259,7 +240,7 @@ ChromeURLRequestContextGetter::CreateOffTheRecord(
     Profile* profile, const ProfileIOData* profile_io_data) {
   DCHECK(profile->IsOffTheRecord());
   return new ChromeURLRequestContextGetter(
-      profile, new FactoryForMain(profile_io_data));
+      new FactoryForMain(profile_io_data));
 }
 
 // static
@@ -268,7 +249,7 @@
ChromeURLRequestContextGetter::CreateOffTheRecordForExtensions(
     Profile* profile, const ProfileIOData* profile_io_data) {
   DCHECK(profile->IsOffTheRecord());
   return new ChromeURLRequestContextGetter(
-      profile, new FactoryForExtensions(profile_io_data));
+      new FactoryForExtensions(profile_io_data));
 }
 
 // static
@@ -283,70 +264,10 @@
ChromeURLRequestContextGetter::CreateOffTheRecordForIsolatedApp(
   ChromeURLRequestContextGetter* main_context =
      
static_cast<ChromeURLRequestContextGetter*>(profile->GetRequestContext());
   return new ChromeURLRequestContextGetter(
-      profile,
       new FactoryForIsolatedApp(profile_io_data, partition_descriptor,
           main_context, protocol_handler_interceptor.Pass()));
 }
 
-void ChromeURLRequestContextGetter::CleanupOnUIThread() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // Unregister for pref notifications.
-  DCHECK(!registrar_.IsEmpty()) << "Called more than once!";
-  registrar_.RemoveAll();
-}
-
-// content::NotificationObserver implementation.
-void ChromeURLRequestContextGetter::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (chrome::NOTIFICATION_PREF_CHANGED == type) {
-    std::string* pref_name_in = content::Details<std::string>(details).ptr();
-    PrefService* prefs = content::Source<PrefService>(source).ptr();
-    DCHECK(pref_name_in && prefs);
-    if (*pref_name_in == prefs::kAcceptLanguages) {
-      std::string accept_language =
-          prefs->GetString(prefs::kAcceptLanguages);
-      BrowserThread::PostTask(
-          BrowserThread::IO, FROM_HERE,
-          base::Bind(
-              &ChromeURLRequestContextGetter::OnAcceptLanguageChange,
-              this,
-              accept_language));
-    } else if (*pref_name_in == prefs::kDefaultCharset) {
-      std::string default_charset = prefs->GetString(prefs::kDefaultCharset);
-      BrowserThread::PostTask(
-          BrowserThread::IO, FROM_HERE,
-          base::Bind(
-              &ChromeURLRequestContextGetter::OnDefaultCharsetChange,
-              this,
-              default_charset));
-    }
-  } else {
-    NOTREACHED();
-  }
-}
-
-void ChromeURLRequestContextGetter::RegisterPrefsObserver(Profile* profile) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  registrar_.Init(profile->GetPrefs());
-  registrar_.Add(prefs::kAcceptLanguages, this);
-  registrar_.Add(prefs::kDefaultCharset, this);
-}
-
-void ChromeURLRequestContextGetter::OnAcceptLanguageChange(
-    const std::string& accept_language) {
-  GetIOContext()->OnAcceptLanguageChange(accept_language);
-}
-
-void ChromeURLRequestContextGetter::OnDefaultCharsetChange(
-    const std::string& default_charset) {
-  GetIOContext()->OnDefaultCharsetChange(default_charset);
-}
-
 // ----------------------------------------------------------------------------
 // ChromeURLRequestContext
 // ----------------------------------------------------------------------------
@@ -387,22 +308,3 @@ void
ChromeURLRequestContext::set_chrome_url_data_manager_backend(
   DCHECK(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(
-      net::HttpUtil::GenerateAcceptLanguageHeader(accept_language));
-}
-
-void ChromeURLRequestContext::OnDefaultCharsetChange(
-    const std::string& default_charset) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  set_accept_charset(
-      net::HttpUtil::GenerateAcceptCharsetHeader(default_charset));
-}

[commit-bot: I haz the power, 2012-11-06 13:15:05]

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

[pauljensen, 2012-11-06 16:16:19]

Erik, can you take a quick look at chrome_frame/test/net/fake_external_tab.cc?

[erikwright (departed), 2012-11-06 16:34:22]

LGTM.

[commit-bot: I haz the power, 2012-11-06 16:38:54]

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

[commit-bot: I haz the power, 2012-11-06 17:18:57]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-06 17:45:00]

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

[commit-bot: I haz the power, 2012-11-06 18:11:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-06 19:30:02]

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

[commit-bot: I haz the power, 2012-11-07 13:44:05]

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

[commit-bot: I haz the power, 2012-11-07 15:08:45]

Change committed as 166425