Extended: Add "system" URLRequestContext

This is an extension of http://codereview.chromium.org/6280018 that provides a proxy configuration which respects command line parameters and policies

This was committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78228, iyengar asked to commit again after fixing trunk.

BUG=67232, 70732
TEST=Start chrome, observe two PROXY_CONFIG_CHANGED events in about:net-internals (if you are on a corporate network  with PAC configurations), observe that everything behaves as usual. In particular the https://www.google.com/searchdomaincheck?format=domain&type=chrome request should not fail as it uses the new system URLRequestContext.

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

[battre, 2011-02-08 17:56:32]

Hi Will,

would you like to have a look at the current state? Is this what you had in
mind?

Best,
Dominic

[willchan no longer on Chromium, 2011-02-08 22:52:04]

This is good progress! Thanks for taking this on, I really appreciate it.

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.h:41: class ProxyConfigService;
No indent

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

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:286: globals_->system_proxy_service =
I think this code when I wrote it was a bit off. We need to be careful not to
use v8 if we're running in single process mode, so we don't collide with the v8
instance used by the renderer.

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
File chrome/browser/system_url_request_context_getter.cc (right):

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.cc:12:
io_message_loop_proxy_(io_thread->message_loop_proxy()) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.cc:17: net::URLRequestContext*
SystemURLRequestContextGetter::GetURLRequestContext() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
File chrome/browser/system_url_request_context_getter.h (right):

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.h:15: explicit
SystemURLRequestContextGetter(IOThread* io_thread);
We try to make sure there's only 1 URLRequestContextGetter per
URLRequestContext. This subtype does not appear to be able to enforce that,
since anyone can publicly create it.

I suggest having IOThread have a member variable:
mutable scoped_refptr<URLRequestContextGetter>
system_url_request_context_getter_;

Provide an accessor: scoped_refptr<URLRequestContextGetter>
IOThread::system_url_request_context_getter() const;

Have this one lazily instantiate it, with the definition in the .cc file.

In io_thread.cc, put the SystemURLRequestContextGetter class definition in the
anonymous namespace. Implement it there.

This way, SystemURLRequestContextGetter is completely hidden from users, the
only thing exposed is a way to get a system_url_request_context_getter from the
IOThread class.

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.h:23: IOThread* io_thread_;  //
Weak pointer, owned by BrowserProcess.
IOThread* const io_thread_;

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.h:25: };
Please add a LeakTracker for this class and assert in IOThread::CleanUp() that
it is deleted. We should make sure people aren't holding onto references to
SystemURLRequestContextGetter for too long.

[brettw, 2011-02-11 17:42:34]

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1456: // PrefService has been initialized.
Why do you need to add this to browser main? Normally such things are lazily
initialized so they don't slow down initial startup.

[battre, 2011-02-14 18:22:04]

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1456: // PrefService has been initialized.
On 2011/02/11 17:42:34, brettw wrote:
> Why do you need to add this to browser main? Normally such things are lazily
> initialized so they don't slow down initial startup.

It is important that the initialization is started on the UI thread (the
ProxyConfigService demands that) and finished on the IO thread (we need to
update the globals_ variable of IOThread).

I think it is possible to move the logic in a way, so that the initialization
can be done lazily. I will check that tomorrow.

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.h:41: class ProxyConfigService;
On 2011/02/08 22:52:04, willchan wrote:
> No indent

Done.

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

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:286: globals_->system_proxy_service =
On 2011/02/08 22:52:04, willchan wrote:
> I think this code when I wrote it was a bit off. We need to be careful not to
> use v8 if we're running in single process mode, so we don't collide with the
v8
> instance used by the renderer.

I don't see a code path that could instantiate a V8 Proxy Resolver. Should I add
this feature? (like ::CreateProxyService in chrome_url_request_context.cc)

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
File chrome/browser/system_url_request_context_getter.cc (right):

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.cc:12:
io_message_loop_proxy_(io_thread->message_loop_proxy()) {
On 2011/02/08 22:52:04, willchan wrote:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.cc:17: net::URLRequestContext*
SystemURLRequestContextGetter::GetURLRequestContext() {
On 2011/02/08 22:52:04, willchan wrote:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

Done.

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
File chrome/browser/system_url_request_context_getter.h (right):

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.h:15: explicit
SystemURLRequestContextGetter(IOThread* io_thread);
On 2011/02/08 22:52:04, willchan wrote:
> We try to make sure there's only 1 URLRequestContextGetter per
> URLRequestContext. This subtype does not appear to be able to enforce that,
> since anyone can publicly create it.
> 
> I suggest having IOThread have a member variable:
> mutable scoped_refptr<URLRequestContextGetter>
> system_url_request_context_getter_;
> 
> Provide an accessor: scoped_refptr<URLRequestContextGetter>
> IOThread::system_url_request_context_getter() const;
> 
> Have this one lazily instantiate it, with the definition in the .cc file.
> 
> In io_thread.cc, put the SystemURLRequestContextGetter class definition in the
> anonymous namespace. Implement it there.
> 
> This way, SystemURLRequestContextGetter is completely hidden from users, the
> only thing exposed is a way to get a system_url_request_context_getter from
the
> IOThread class.

Done.

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.h:23: IOThread* io_thread_;  //
Weak pointer, owned by BrowserProcess.
On 2011/02/08 22:52:04, willchan wrote:
> IOThread* const io_thread_;

Done.

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/system_url_re...
chrome/browser/system_url_request_context_getter.h:25: };
On 2011/02/08 22:52:04, willchan wrote:
> Please add a LeakTracker for this class and assert in IOThread::CleanUp() that
> it is deleted. We should make sure people aren't holding onto references to
> SystemURLRequestContextGetter for too long.

Done.

[willchan no longer on Chromium, 2011-02-15 01:05:58]

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

http://codereview.chromium.org/6292017/diff/5001/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:286: globals_->system_proxy_service =
On 2011/02/14 18:22:05, battre wrote:
> On 2011/02/08 22:52:04, willchan wrote:
> > I think this code when I wrote it was a bit off. We need to be careful not
to
> > use v8 if we're running in single process mode, so we don't collide with the
> v8
> > instance used by the renderer.
> 
> I don't see a code path that could instantiate a V8 Proxy Resolver. Should I
add
> this feature? (like ::CreateProxyService in chrome_url_request_context.cc)

Yes. Perhaps you could refactor them both to share the same code?

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.h:116: virtual void
InitSystemRequestContext();
Isn't this a BrowserProcess method? Should it be with the other BrowserProcess
overrides?

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

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:224: explicit SystemURLRequestContextGetter(IOThread
* io_thread);
We don't have spaces between types and '*', so I'd eliminate that here.

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:232: IOThread * const io_thread_;  // Weak pointer,
owned by BrowserProcess.
We don't have spaces between types and '*', so I'd eliminate that here.

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:246: net::URLRequestContext*
SystemURLRequestContextGetter::GetURLRequestContext() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:517: system_url_request_context_getter_ = NULL;
Please move this line to the end of "Step 1". See the comment on line 457.

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:518:
base::debug::LeakTracker<SystemURLRequestContextGetter>::CheckForLeaks();
Please move this to CleanUpAfterMessageLoopDestruction() like the other
CheckForLeaks().

[battre, 2011-02-15 19:32:49]

I addressed your comments.

This version is not 100% cleaned up nor have I checked whether the unit tests
pass (before moving stuff to proxy_service_factory.cc they were fine). You can
have a look if you are curious and want to comment already. Unfortunately, I
have to leave now.

Best,

Dominic

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.h:116: virtual void
InitSystemRequestContext();
On 2011/02/15 01:05:58, willchan wrote:
> Isn't this a BrowserProcess method? Should it be with the other BrowserProcess
> overrides?

This is gone.

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

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:224: explicit SystemURLRequestContextGetter(IOThread
* io_thread);
On 2011/02/15 01:05:58, willchan wrote:
> We don't have spaces between types and '*', so I'd eliminate that here.

Done.

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:232: IOThread * const io_thread_;  // Weak pointer,
owned by BrowserProcess.
On 2011/02/15 01:05:58, willchan wrote:
> We don't have spaces between types and '*', so I'd eliminate that here.

Done.

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:246: net::URLRequestContext*
SystemURLRequestContextGetter::GetURLRequestContext() {
On 2011/02/15 01:05:58, willchan wrote:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

Done.

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:517: system_url_request_context_getter_ = NULL;
On 2011/02/15 01:05:58, willchan wrote:
> Please move this line to the end of "Step 1". See the comment on line 457.

Done.

http://codereview.chromium.org/6292017/diff/16003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:518:
base::debug::LeakTracker<SystemURLRequestContextGetter>::CheckForLeaks();
On 2011/02/15 01:05:58, willchan wrote:
> Please move this to CleanUpAfterMessageLoopDestruction() like the other
> CheckForLeaks().

Done.

[willchan no longer on Chromium, 2011-02-16 20:02:27]

Please note when you merge that I've refactored chrome_url_request_context.cc
substantially, moving most of the code out into chrome/browser/profiles/. I've
moved the ProxyConfigService creation code to profile_io_data.cc. You should
update that code to invoke your new ProxyServiceFactory class static functions.

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:874: net::ProxyConfigService*
config_service =
There's also a SSLConfigService, so can you name this variable
proxy_config_service?

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:882: system_request_context_ =
io_thread()->system_url_request_context_getter();
Don't cache the pointer here. We want the IOThread::Globals to hold the last
reference.

You should probably change the above assertion to be
DCHECK(!created_system_request_context_). Then just call
io_thread()->system_url_request_context_getter() to initialize it.

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

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:363:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Add a assertion that system_proxy_config_service_ is set.

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.h (right):

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.h:14: class NetLog;
Don't indent.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.h:36: };
Use DISALLOW_IMPLICIT_CONSTRUCTORS to prevent this object from being
instantiated.

[kuan, 2011-02-18 16:29:30]

i focused on changes for chromeos's proxy config service, mostly nits, so lgtm
for chromeos related changes.

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:96: #include
"chrome/browser/chromeos/proxy_config_service.h"
any reason why u need this file instead of just proxy_config_service_impl.h?

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:42: //
chrome/browser/net/proxy_service_factor.cc::CreateProxyConfigService
typo: factor -> factory

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:43: // via
BrowserProcess::chomeos_proxy_config_service_impl, and stored in
typo: chomeos -> chromeos

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:53: //
BrowserProcess::chomeos_proxy_config_service_impl to allow user to read
same here

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:21: class ProxyConfigServiceImpl;
is this still needed?

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:114: #include
"chrome/browser/chromeos/proxy_config_service_impl.h"
is this still needed?

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:32: #endif  // defined(OS_CHROMEOS)
is this defined block still needed?
while we're on that, cld u also remove the same defined block in
chrome_url_request_context.cc?  thanks!

[brettw, 2011-02-21 06:22:47]

I'm really happy we could do without the addition to browser_main, thanks!

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:875: void
BrowserProcessImpl::CreatePrefProxyConfigTracker() {
It's not clear to me why this code is in the BrowserProcess object? It seems
kine of weird that it would handle the lazy creation of something in another
class without storing any local state (I realize in a previous version you did
cache this). Should this code live in the IO thread? (I'm not actually sure, I'm
interested in others' opinions.)

[battre, 2011-02-21 17:27:57]

Thanks for your reviews. This is a new version. I think several parts became
significantly simpler due to moving some code from browser_process_impl.cc to
io_thread.cc and Mattias' CL http://codereview.chromium.org/6542013/.

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:874: net::ProxyConfigService*
config_service =
On 2011/02/16 20:02:27, willchan wrote:
> There's also a SSLConfigService, so can you name this variable
> proxy_config_service?

Done.

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:882: system_request_context_ =
io_thread()->system_url_request_context_getter();
On 2011/02/16 20:02:27, willchan wrote:
> Don't cache the pointer here. We want the IOThread::Globals to hold the last
> reference.
> 
> You should probably change the above assertion to be
> DCHECK(!created_system_request_context_). Then just call
> io_thread()->system_url_request_context_getter() to initialize it.

Done.

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

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:363:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/02/16 20:02:27, willchan wrote:
> Add a assertion that system_proxy_config_service_ is set.

Done.

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.h (right):

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.h:14: class NetLog;
On 2011/02/16 20:02:27, willchan wrote:
> Don't indent.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Done.

http://codereview.chromium.org/6292017/diff/20003/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.h:36: };
On 2011/02/16 20:02:27, willchan wrote:
> Use DISALLOW_IMPLICIT_CONSTRUCTORS to prevent this object from being
> instantiated.

Done.

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:96: #include
"chrome/browser/chromeos/proxy_config_service.h"
On 2011/02/18 16:29:31, kuan wrote:
> any reason why u need this file instead of just proxy_config_service_impl.h?

Done.

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:875: void
BrowserProcessImpl::CreatePrefProxyConfigTracker() {
Thanks for nagging. This simplified things significantly. The only cost is that
I need to store a local_state pointer in IOThread. But I think this is ok,
because it local_state outlives the IOThread.

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:42: //
chrome/browser/net/proxy_service_factor.cc::CreateProxyConfigService
On 2011/02/18 16:29:31, kuan wrote:
> typo: factor -> factory

Done.

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:43: // via
BrowserProcess::chomeos_proxy_config_service_impl, and stored in
On 2011/02/18 16:29:31, kuan wrote:
> typo: chomeos -> chromeos

Done.

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:53: //
BrowserProcess::chomeos_proxy_config_service_impl to allow user to read
On 2011/02/18 16:29:31, kuan wrote:
> same here

Done.

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:21: class ProxyConfigServiceImpl;
On 2011/02/18 16:29:31, kuan wrote:
> is this still needed?

Done.

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:114: #include
"chrome/browser/chromeos/proxy_config_service_impl.h"
On 2011/02/18 16:29:31, kuan wrote:
> is this still needed?

Done.

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6292017/diff/27001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:32: #endif  // defined(OS_CHROMEOS)
On 2011/02/18 16:29:31, kuan wrote:
> is this defined block still needed?
> while we're on that, cld u also remove the same defined block in
> chrome_url_request_context.cc?  thanks!

Done.

[brettw, 2011-02-21 18:49:10]

I'm glad that cleaned it up! I'm going to be gone for the next few days so I'll
defer to Will's LGTM from here.

[battre, 2011-02-21 19:25:23]

+mnissler due to recent changes in ChromeOS proxy configuration
http://codereview.chromium.org/6549007/

[willchan no longer on Chromium, 2011-02-21 23:05:49]

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:43: // via
BrowserProcess::chromeos_proxy_config_service_impl, and stored in
Won't it be called multiple times? Once for the BrowserProcess and once per
Profile?

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/intranet_red...
File chrome/browser/intranet_redirect_detector.cc (right):

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/intranet_red...
chrome/browser/intranet_redirect_detector.cc:31:
g_browser_process->system_request_context() != NULL) {
Do we know whether or not this works? I believe there's the
Profile::GetDefaultRequestContext() check because this code can be invoked
before the default Profile exists. Is it possible that this code is invoked
before the BrowserProcess is instantiated? If so, g_browser_process might be
NULL. Have you investigated how this works?

You definitely don't need the NULL check, and you won't need the
DEFAULT_REQUEST_CONTEXT_AVAILABLE notification anymore, since you're switching
to the system request context instead of the "default" request context.

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:20: #if defined(OS_CHROMEOS)
You should include "build/build_config.h" for this macro definition.

[Mattias Nissler (ping if slow), 2011-02-22 09:58:05]

Drive by comments. Approach looks good.

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

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:366: pref_proxy_config_tracker_ = new
PrefProxyConfigTracker(local_state_);
why don't you just construct the PrefProxyConfigTracker in the constructor, so
you don't have to keep local_state around?

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:30: class EnterpriseExtensionObserver;
What's this?

[battre, 2011-03-08 17:38:58]

And the next iteration... thanks for your comments.

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:43: // via
BrowserProcess::chromeos_proxy_config_service_impl, and stored in
On 2011/02/21 23:05:49, willchan wrote:
> Won't it be called multiple times? Once for the BrowserProcess and once per
> Profile?

No, it is stored in g_browser_process. The instance is then recycled for the
profiles:

// static
net::ProxyConfigService* ProxyServiceFactory::CreateProxyConfigService(
    PrefProxyConfigTracker* proxy_config_tracker) {
...
#if defined(OS_CHROMEOS)
  base_service = new chromeos::ProxyConfigService(
      g_browser_process->chromeos_proxy_config_service_impl());
#else
...

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/intranet_red...
File chrome/browser/intranet_redirect_detector.cc (right):

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/intranet_red...
chrome/browser/intranet_redirect_detector.cc:31:
g_browser_process->system_request_context() != NULL) {
On 2011/02/21 23:05:49, willchan wrote:
> Do we know whether or not this works? I believe there's the
> Profile::GetDefaultRequestContext() check because this code can be invoked
> before the default Profile exists. Is it possible that this code is invoked
> before the BrowserProcess is instantiated? If so, g_browser_process might be
> NULL. Have you investigated how this works?
> 
> You definitely don't need the NULL check, and you won't need the
> DEFAULT_REQUEST_CONTEXT_AVAILABLE notification anymore, since you're switching
> to the system request context instead of the "default" request context.

The IntranetRedirectDetector is instantiated by the BrowserProcessImp.
Therefore, it is guaranteed that g_browser_process is non-null.

I have stripped everything related to waiting for the default request context.

Done.

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

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:366: pref_proxy_config_tracker_ = new
PrefProxyConfigTracker(local_state_);
On 2011/02/22 09:58:05, Mattias Nissler wrote:
> why don't you just construct the PrefProxyConfigTracker in the constructor, so
> you don't have to keep local_state around?

The idea was to have true lazy initialization but I agree that this is not worth
it.

Done.

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:20: #if defined(OS_CHROMEOS)
On 2011/02/21 23:05:49, willchan wrote:
> You should include "build/build_config.h" for this macro definition.

Are you sure about this? chrome/browser/net/chrome_url_request_context.cc has a
#if defined(OS_CHROMEOS) but does not include build_config.h and build_config.h
itself does not define OS_CHROMEOS.

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/6292017/diff/31003/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:30: class EnterpriseExtensionObserver;
On 2011/02/22 09:58:05, Mattias Nissler wrote:
> What's this?

Needed to compile.

[Mattias Nissler (ping if slow), 2011-03-09 11:04:55]

a couple of comments.

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/browser_proc...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/browser_proc...
chrome/browser/browser_process.h:93: virtual
scoped_refptr<URLRequestContextGetter> system_request_context() = 0;
Why do we return this wrapped in a scoped_refptr? Can't the caller just take
care of that if required?

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:12: #include
"chrome/browser/profiles/profile_manager.h"
please remove the profile-related includes.

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

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:130: friend class SystemURLRequestContextGetter;
Why do we need this? Add a comment explaining this?

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:205: PrefProxyConfigTracker*
pref_proxy_config_tracker_; // Weak pointer.
If you never construct the system request context, you'll leak this. Why not
just make it a scoped_refptr and be safe?

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:12: #include
"chrome/browser/io_thread.h"
needed?

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:13: #include
"chrome/browser/profiles/profile.h"
needed?

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.h (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.h:12: class MessageLoop;
not needed.

http://codereview.chromium.org/6292017/diff/38023/chrome/test/testing_browser...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:119: return NULL;
Are we prepared to handle that?

http://codereview.chromium.org/6292017/diff/38023/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:125: return NULL;
same here?

[battre, 2011-03-09 19:24:07]

Thanks for your comments.

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/browser_proc...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/browser_proc...
chrome/browser/browser_process.h:93: virtual
scoped_refptr<URLRequestContextGetter> system_request_context() = 0;
On 2011/03/09 11:04:56, Mattias Nissler wrote:
> Why do we return this wrapped in a scoped_refptr? Can't the caller just take
> care of that if required?

I think I have copied the pattern from somewhere, but I agree that in most cases
we don't return wrapped pointers. So I have deleted the wrapping.

Done.

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:12: #include
"chrome/browser/profiles/profile_manager.h"
On 2011/03/09 11:04:56, Mattias Nissler wrote:
> please remove the profile-related includes.

Done.

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

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:130: friend class SystemURLRequestContextGetter;
On 2011/03/09 11:04:56, Mattias Nissler wrote:
> Why do we need this? Add a comment explaining this?

Done.

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:205: PrefProxyConfigTracker*
pref_proxy_config_tracker_; // Weak pointer.
On 2011/03/09 11:04:56, Mattias Nissler wrote:
> If you never construct the system request context, you'll leak this. Why not
> just make it a scoped_refptr and be safe?

Done.

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:12: #include
"chrome/browser/io_thread.h"
On 2011/03/09 11:04:56, Mattias Nissler wrote:
> needed?

Yes. g_browser_process->io_thread()

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:13: #include
"chrome/browser/profiles/profile.h"
On 2011/03/09 11:04:56, Mattias Nissler wrote:
> needed?

Done.

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.h (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.h:12: class MessageLoop;
On 2011/03/09 11:04:56, Mattias Nissler wrote:
> not needed.

Done.

http://codereview.chromium.org/6292017/diff/38023/chrome/test/testing_browser...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/6292017/diff/38023/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:119: return NULL;
On 2011/03/09 11:04:56, Mattias Nissler wrote:
> Are we prepared to handle that?

Currently, there are only two use cases of system_request_context():
GoogleUrlTracker and IntranetRedirectDetector. The unit tests for
GoogleUrlTracker use a TestURLFetcherFactory which does not access the request
context. The latter has no unit tests.

In the past we had only the GetRequestContext() of TestingProfile which returned
NULL by default. I guess we can write something if we ever need it.

http://codereview.chromium.org/6292017/diff/38023/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:125: return NULL;
On 2011/03/09 11:04:56, Mattias Nissler wrote:
> same here?

This was copied literally from TestingProfile.

[willchan no longer on Chromium, 2011-03-10 00:40:01]

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/browser_proc...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/browser_proc...
chrome/browser/browser_process.h:52: #if defined(OS_CHROMEOS)
Make sure to include build_config.h for this definition.

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:454:
BrowserProcessImpl::chromeos_proxy_config_service_impl() {
DCHECK(CalledOnValidThread())

Perhaps you should have a created_foo_ bool like the others to make sure this
isn't double initialized?

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

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:516:
BrowserProcessSubThread::CleanUpAfterMessageLoopDestruction();
Bad merge?

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:523:
base::debug::LeakTracker<net::URLRequest>::CheckForLeaks();
Bad berge? This already exists above.

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:525:
base::debug::LeakTracker<SystemURLRequestContextGetter>::CheckForLeaks();
This is good.

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:9: #include
"content/browser/browser_thread.h"
alphabetical order

[battre, 2011-03-10 16:11:57]

And the next iteration. :-)

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/browser_proc...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/browser_proc...
chrome/browser/browser_process.h:52: #if defined(OS_CHROMEOS)
On 2011/03/10 00:40:02, willchan wrote:
> Make sure to include build_config.h for this definition.

OS_CHROMEOS is not defined by build_config.h (it defines OS_LINUX, OS_WINDOWS,
... but not OS_CHROMEOS).

I think the definition comes from the following line in
platform/cros/SConstruct.chromiumos:
env_test.Append(CCFLAGS=['-DOS_CHROMEOS'])

If you like, I can include build_config.h here. But as of now it does not make a
difference. With this in mind, should I include it?

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:454:
BrowserProcessImpl::chromeos_proxy_config_service_impl() {
On 2011/03/10 00:40:02, willchan wrote:
> DCHECK(CalledOnValidThread())
> 
> Perhaps you should have a created_foo_ bool like the others to make sure this
> isn't double initialized?

I have added the DCHECK. I think there is no need for a created_foo_ bool:
The chromeos_proxy_config_service_impl_ variable is initialized only once and !=
NULL afterwards. We need the created_foo_ booleans only for variables that are
manually reset in the destructor at times when consumers of BrowserProcessImpl
could still try to access them. In this case the following sequence would be
possible and justifies the created_foo_ booleans:
- destructor is called
  - variable foo_ is reset to zero
- consumer of BrowserProcessImpl calls BrowserBrocessImpl::foo()
--> foo_ is initialized a second time

This cannot happen for BrowserProcessImpl.

I can add the created_foo_ variable but I think there is no benefit.

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

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:516:
BrowserProcessSubThread::CleanUpAfterMessageLoopDestruction();
On 2011/03/10 00:40:02, willchan wrote:
> Bad merge?

wow, yes.

Done.

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:523:
base::debug::LeakTracker<net::URLRequest>::CheckForLeaks();
On 2011/03/10 00:40:02, willchan wrote:
> Bad berge? This already exists above.

Done.

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:525:
base::debug::LeakTracker<SystemURLRequestContextGetter>::CheckForLeaks();
On 2011/03/10 00:40:02, willchan wrote:
> This is good.

Done.

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/6292017/diff/47001/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:9: #include
"content/browser/browser_thread.h"
On 2011/03/10 00:40:02, willchan wrote:
> alphabetical order

Done.

[willchan no longer on Chromium, 2011-03-14 19:32:39]

LGTM

Be careful with the GoogleURLTracker, since if we start needing to hit https://
URLs, then it'll break since the system URLRequestContext doesn't support a real
SSLConfigService yet. But this is probably fine since I don't imagine us using
https:// URLs for Google search domains before we finish the system
SSLConfigService.

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

http://codereview.chromium.org/6292017/diff/54002/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:203: scoped_refptr<URLRequestContextGetter>
system_url_request_context_getter_;
Member variable ordering should be longest-lived objects first.

The system URLRequestContextGetter should live beyond the profile-specific
ChromeURLRequestContextGetters. So, this should move ahead of those.

And the objects that the URLRequestContextGetter transitively point to (these
next two variables) should live beyond the URLRequestContextGetter itself. They
should probably be after |globals_| and before |url_request_context_getters_|.

http://codereview.chromium.org/6292017/diff/54002/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.h (right):

http://codereview.chromium.org/6292017/diff/54002/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.h:19: }  // net
namespace net

[battre, 2011-03-15 18:14:40]

Thanks for the reviews.

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

http://codereview.chromium.org/6292017/diff/54002/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:203: scoped_refptr<URLRequestContextGetter>
system_url_request_context_getter_;
On 2011/03/14 19:32:39, willchan wrote:
> Member variable ordering should be longest-lived objects first.
> 
> The system URLRequestContextGetter should live beyond the profile-specific
> ChromeURLRequestContextGetters. So, this should move ahead of those.
> 
> And the objects that the URLRequestContextGetter transitively point to (these
> next two variables) should live beyond the URLRequestContextGetter itself.
They
> should probably be after |globals_| and before |url_request_context_getters_|.

Done.

http://codereview.chromium.org/6292017/diff/54002/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.h (right):

http://codereview.chromium.org/6292017/diff/54002/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.h:19: }  // net
On 2011/03/14 19:32:39, willchan wrote:
> namespace net

Done.