make ProxyService::CreateSystemProxyConfigService return scoped_ptrs

make ProxyService::CreateSystemProxyConfigService return scoped_ptrs

This change is fairly straightforward. I ended up refactoring ProxyServiceFactory too because it makes the resulting code much easier to reason about, so we are only making scoped pointers out of raw pointers in as few places as possible

new CL to reflect Randy and Pauls changes in master

BUG=523075
TBR=brettw@chromium.org

Committed: https://crrev.com/b7e3a08f4c62c69093ecb3a258e18f2cdd340b08
Cr-Commit-Position: refs/heads/master@{#350207}

[Charlie Harrison, 2015-09-18 18:30:12]

csharrison@chromium.org changed reviewers:
+ mmenke@chromium.org, rdsmith@chromium.org

[Charlie Harrison, 2015-09-18 18:30:13]

Could you take a look before I rope in other reviewers? Thanks :)

[mmenke, 2015-09-18 18:58:54]

Looks good, thanks for taking this on!

https://codereview.chromium.org/1356933002/diff/50001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/1356933002/diff/50001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:66: net::ProxyConfigService*
CreateProxyConfigService() {
Should probably modify this to return a scoped_ptr in this CL as well, if there
isn't too much additional fallout.

https://codereview.chromium.org/1356933002/diff/50001/chrome/browser/net/prox...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1356933002/diff/50001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:80: base_service =
(net::ProxyService::CreateSystemProxyConfigService(
nit:  Extra parens not needed.

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1482:
config_service_.reset(new_proxy_config_service.release());
Prefer Pass() to release().  Pass ensures things never escapes from being
wrapped in a scoped_ptr.

config_service_ = new_proxy_config_service.Pass().

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
net/proxy/proxy_service.h:207: // |new_proxy_config_service|.
nit:  No longer need the last sentence.

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
File net/proxy/proxy_service_mojo.h (right):

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
net/proxy/proxy_service_mojo.h:28: // ownership of |proxy_config_service|.
No need for last sentence anymore

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
File net/proxy/proxy_service_v8.h (right):

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
net/proxy/proxy_service_v8.h:23: // the proxy settings change. We take ownership
of |proxy_config_service|.
Remove last sentence.

https://codereview.chromium.org/1356933002/diff/50001/net/url_request/url_req...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1356933002/diff/50001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:300:
proxy_config_service_.release());
= *.Pass()

https://codereview.chromium.org/1356933002/diff/50001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:304:
proxy_config_service.reset(proxy_config_service_.release());
= *.Pass()

[mmenke, 2015-09-18 19:01:18]

Oh, and since this is just a minor API change, you can TBR OWNERs once you get
signoff from some net/ folks.

[Charlie Harrison, 2015-09-18 19:56:06]

https://codereview.chromium.org/1356933002/diff/50001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/1356933002/diff/50001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:66: net::ProxyConfigService*
CreateProxyConfigService() {
On 2015/09/18 18:58:53, mmenke wrote:
> Should probably modify this to return a scoped_ptr in this CL as well, if
there
> isn't too much additional fallout.

Done.

https://codereview.chromium.org/1356933002/diff/50001/chrome/browser/net/prox...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1356933002/diff/50001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:80: base_service =
(net::ProxyService::CreateSystemProxyConfigService(
On 2015/09/18 18:58:53, mmenke wrote:
> nit:  Extra parens not needed.

Done.

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1482:
config_service_.reset(new_proxy_config_service.release());
On 2015/09/18 18:58:53, mmenke wrote:
> Prefer Pass() to release().  Pass ensures things never escapes from being
> wrapped in a scoped_ptr.
> 
> config_service_ = new_proxy_config_service.Pass().

Done.

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
net/proxy/proxy_service.h:207: // |new_proxy_config_service|.
On 2015/09/18 18:58:54, mmenke wrote:
> nit:  No longer need the last sentence.

Done.

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
File net/proxy/proxy_service_mojo.h (right):

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
net/proxy/proxy_service_mojo.h:28: // ownership of |proxy_config_service|.
On 2015/09/18 18:58:54, mmenke wrote:
> No need for last sentence anymore

Done.

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
File net/proxy/proxy_service_v8.h (right):

https://codereview.chromium.org/1356933002/diff/50001/net/proxy/proxy_service...
net/proxy/proxy_service_v8.h:23: // the proxy settings change. We take ownership
of |proxy_config_service|.
On 2015/09/18 18:58:54, mmenke wrote:
> Remove last sentence.

Done.

https://codereview.chromium.org/1356933002/diff/50001/net/url_request/url_req...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1356933002/diff/50001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:300:
proxy_config_service_.release());
On 2015/09/18 18:58:54, mmenke wrote:
> = *.Pass()

Done.

https://codereview.chromium.org/1356933002/diff/50001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:304:
proxy_config_service.reset(proxy_config_service_.release());
On 2015/09/18 18:58:54, mmenke wrote:
> = *.Pass()

Done.

[Randy Smith (Not in Mondays), 2015-09-18 20:14:55]

I'm going to be on vacation next week, so I'll leave this review in Matt's
hands.  You can consider all my comments below suggestions except for the
concern about the .get()->.Pass() semantic change; that's worrisome.

https://codereview.chromium.org/1356933002/diff/70001/chrome/browser/net/prox...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1356933002/diff/70001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:85: return
tracker->CreateTrackingProxyConfigService(base_service.Pass()).Pass();
If the return type of CreateTrackingProxyConfigService is ProxyConfigService (as
opposed to some subclass), you don't need the .Pass(); the compiler figures out
that it's going out of scope and hence that a straight assignment is legal.

https://codereview.chromium.org/1356933002/diff/70001/ios/chrome/browser/net/...
File ios/chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1356933002/diff/70001/ios/chrome/browser/net/...
ios/chrome/browser/net/proxy_service_factory.cc:48: net::ProxyConfigService*
proxy_config_service,
Did you look at the call tree leading into this argument?  How hard would it be
to make this argument a scoped_ptr<>?

https://codereview.chromium.org/1356933002/diff/70001/ios/crnet/crnet_environ...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1356933002/diff/70001/ios/crnet/crnet_environ...
ios/crnet/crnet_environment.mm:411: proxy_config_service_.Pass(), 0, nullptr)
This is a semantic change; did you trace code to make sure it's safe?  (i.e. no
one else touching proxy_config_service_ again, it never being deleted)

https://codereview.chromium.org/1356933002/diff/70001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1356933002/diff/70001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:934: ResetConfigService(config_service.Pass());
nit, suggestion: You might not need the .Pass() here, since config_service is
going out of scope.  The compiler's the ultimate arbiter, obviously.

https://codereview.chromium.org/1356933002/diff/70001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:946: return
CreateWithoutProxyResolver(proxy_config_service.Pass(), net_log);
same comment here and below (but I'm speculating).

https://codereview.chromium.org/1356933002/diff/70001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1356933002/diff/70001/net/proxy/proxy_service...
net/proxy/proxy_service.h:95: // The instance takes ownership of
|config_service| and |resolver_factory|.
I think this comment is no longer needed; the same info's contained in the
signature.  (Apologies--I suspect this is partially cleanup from my CL.)

[mmenke, 2015-09-18 20:31:11]

https://codereview.chromium.org/1356933002/diff/70001/ios/crnet/crnet_environ...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1356933002/diff/70001/ios/crnet/crnet_environ...
ios/crnet/crnet_environment.mm:411: proxy_config_service_.Pass(), 0, nullptr)
On 2015/09/18 20:14:55, rdsmith wrote:
> This is a semantic change; did you trace code to make sure it's safe?  (i.e.
no
> one else touching proxy_config_service_ again, it never being deleted)

This change actually fixes a double delete, I believe.

[Randy Smith (Not in Mondays), 2015-09-18 20:35:03]

LGTM; Matt can take it from here.  See you in a week.

https://codereview.chromium.org/1356933002/diff/70001/ios/crnet/crnet_environ...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1356933002/diff/70001/ios/crnet/crnet_environ...
ios/crnet/crnet_environment.mm:411: proxy_config_service_.Pass(), 0, nullptr)
On 2015/09/18 20:31:10, mmenke wrote:
> On 2015/09/18 20:14:55, rdsmith wrote:
> > This is a semantic change; did you trace code to make sure it's safe?  (i.e.
> no
> > one else touching proxy_config_service_ again, it never being deleted)
> 
> This change actually fixes a double delete, I believe.

Best kind of semantic change :-}.

[Charlie Harrison, 2015-09-21 16:47:05]

https://codereview.chromium.org/1356933002/diff/70001/chrome/browser/net/prox...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1356933002/diff/70001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:85: return
tracker->CreateTrackingProxyConfigService(base_service.Pass()).Pass();
On 2015/09/18 20:14:55, rdsmith wrote:
> If the return type of CreateTrackingProxyConfigService is ProxyConfigService
(as
> opposed to some subclass), you don't need the .Pass(); the compiler figures
out
> that it's going out of scope and hence that a straight assignment is legal.

Done.

https://codereview.chromium.org/1356933002/diff/70001/ios/chrome/browser/net/...
File ios/chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1356933002/diff/70001/ios/chrome/browser/net/...
ios/chrome/browser/net/proxy_service_factory.cc:48: net::ProxyConfigService*
proxy_config_service,
On 2015/09/18 20:14:55, rdsmith wrote:
> Did you look at the call tree leading into this argument?  How hard would it
be
> to make this argument a scoped_ptr<>?

Actually I think this won't be hard. Will do this.

https://codereview.chromium.org/1356933002/diff/70001/ios/crnet/crnet_environ...
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1356933002/diff/70001/ios/crnet/crnet_environ...
ios/crnet/crnet_environment.mm:411: proxy_config_service_.Pass(), 0, nullptr)
On 2015/09/18 20:35:03, rdsmith wrote:
> On 2015/09/18 20:31:10, mmenke wrote:
> > On 2015/09/18 20:14:55, rdsmith wrote:
> > > This is a semantic change; did you trace code to make sure it's safe? 
(i.e.
> > no
> > > one else touching proxy_config_service_ again, it never being deleted)
> > 
> > This change actually fixes a double delete, I believe.
> 
> Best kind of semantic change :-}.

Yeah I believe this is the only time proxy_config_service_ is needed by the
class. There's only that brief window of ownership.

https://codereview.chromium.org/1356933002/diff/70001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1356933002/diff/70001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:934: ResetConfigService(config_service.Pass());
On 2015/09/18 20:14:55, rdsmith wrote:
> nit, suggestion: You might not need the .Pass() here, since config_service is
> going out of scope.  The compiler's the ultimate arbiter, obviously.

Compiler complains about these when Pass() is omitted. Unlike in the case where
you .Pass() the whole `return` expression, in this case maybe the compiler is
extra cautious, because you could be using config_service again e.g. as another
argument, etc.

https://codereview.chromium.org/1356933002/diff/70001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1356933002/diff/70001/net/proxy/proxy_service...
net/proxy/proxy_service.h:95: // The instance takes ownership of
|config_service| and |resolver_factory|.
On 2015/09/18 20:14:55, rdsmith wrote:
> I think this comment is no longer needed; the same info's contained in the
> signature.  (Apologies--I suspect this is partially cleanup from my CL.)

Done.

[mmenke, 2015-09-21 21:34:51]

LGTM, just a couple optional suggestions of the "while you're here..." variety.

https://codereview.chromium.org/1356933002/diff/110001/net/url_request/url_re...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1356933002/diff/110001/net/url_request/url_re...
net/url_request/url_request_context_builder.cc:310: #endif  // defined(OS_LINUX)
|| defined(OS_ANDROID)
Mind doing a small cleanup while you're here?  This could just be:

#if !defined(OS_LINUX) && !defined(OS_ANDROID)
  if (!proxy_config_service_) {
    proxy_config_service_ = ProxyService::CreateSystemProxyConfigService(
        ...);
  }
#endif  //  !defined(OS_LINUX) && !defined(OS_ANDROID)

And below use proxy_config_service_.Pass(), and get rid of the
proxy_config_service local completely.

https://codereview.chromium.org/1356933002/diff/110001/remoting/base/url_requ...
File remoting/base/url_request_context_getter.cc (right):

https://codereview.chromium.org/1356933002/diff/110001/remoting/base/url_requ...
remoting/base/url_request_context_getter.cc:21: network_task_runner_,
file_task_runner);
nit:  Can you just put this in an initializer list?

https://codereview.chromium.org/1356933002/diff/110001/remoting/base/url_requ...
remoting/base/url_request_context_getter.cc:21: network_task_runner_,
file_task_runner);
nit:  While you're here, remove the underscore at the end of
network_task_runner_?  (Weird that we use an argument for one, and a member
variable for the other).

[mmenke, 2015-09-21 21:38:07]

Oh, and those Android build failures look like real failures, to me.

[Charlie Harrison, 2015-09-21 21:51:40]

On 2015/09/21 21:38:07, mmenke wrote:
> Oh, and those Android build failures look like real failures, to me.

Yeah they are, problem with static_cast-ing a scoped ptr I think. I'm building
now for android to debug. It's related to the change in
android_webview/browser/aw_browser_context.cc if you have any ideas.

[mmenke, 2015-09-21 21:58:09]

On 2015/09/21 21:51:40, csharrison wrote:
> On 2015/09/21 21:38:07, mmenke wrote:
> > Oh, and those Android build failures look like real failures, to me.
> 
> Yeah they are, problem with static_cast-ing a scoped ptr I think. I'm building
> now for android to debug. It's related to the change in
> android_webview/browser/aw_browser_context.cc if you have any ideas.

You may have to store it in a scoped_ptr<net::ProxyConfigService>, and cast the
results of its get() method, and call set_exclude_pac_url on that, and then pass
the scoped_ptr.

Or you could just call "new ProxyConfigServiceAndroid" directly, as we aren't
getting anything from calling the wrapper.

You could even make ProxyConfigServiceAndroid take a bool in its constructor,
get rid of the setter, and just call the constructor.  There'll be a lot more
fallout if you go this route, though, as you'll probably have to muck with
tests, so may make sense in another CL, to be landed either before or after this
one.

[mmenke, 2015-09-21 22:22:25]

Just to save you a trybot round, you'll need to merge with
https://codereview.chromium.org/1318573006 and update your CL.

[Charlie Harrison, 2015-09-21 22:31:07]

On 2015/09/21 22:22:25, mmenke wrote:
> Just to save you a trybot round, you'll need to merge with
> https://codereview.chromium.org/1318573006 and update your CL.

Thanks for the heads up. I'm going to go with option 1 for now, as it's most
explicit about what needs to be happening. I don't want to remove the call to
the wrapper.

[mmenke, 2015-09-21 22:55:50]

On 2015/09/21 22:31:07, csharrison wrote:
> On 2015/09/21 22:22:25, mmenke wrote:
> > Just to save you a trybot round, you'll need to merge with
> > https://codereview.chromium.org/1318573006 and update your CL.
> 
> Thanks for the heads up. I'm going to go with option 1 for now, as it's most
> explicit about what needs to be happening. I don't want to remove the call to
> the wrapper.

Works for me (Though my feeling is that if you're explicitly casting, 9 times
out of 9.01, you're doing the wrong thing).

[Charlie Harrison, 2015-09-21 22:58:28]

On 2015/09/21 22:55:50, mmenke wrote:
> On 2015/09/21 22:31:07, csharrison wrote:
> > On 2015/09/21 22:22:25, mmenke wrote:
> > > Just to save you a trybot round, you'll need to merge with
> > > https://codereview.chromium.org/1318573006 and update your CL.
> > 
> > Thanks for the heads up. I'm going to go with option 1 for now, as it's most
> > explicit about what needs to be happening. I don't want to remove the call
to
> > the wrapper.
> 
> Works for me (Though my feeling is that if you're explicitly casting, 9 times
> out of 9.01, you're doing the wrong thing).

Yes I agree, but in lieu of a more dramatic refactoring. I think the code needs
some architectural love. I'll add a todo to make the wrapper more generic so
this isn't needed.

[Charlie Harrison, 2015-09-22 18:00:27]

csharrison@chromium.org changed reviewers:
+ brettw@chromium.org

[Charlie Harrison, 2015-09-22 18:00:29]

TBRing for trivial api change.

https://codereview.chromium.org/1356933002/diff/110001/net/url_request/url_re...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1356933002/diff/110001/net/url_request/url_re...
net/url_request/url_request_context_builder.cc:310: #endif  // defined(OS_LINUX)
|| defined(OS_ANDROID)
On 2015/09/21 21:34:51, mmenke wrote:
> Mind doing a small cleanup while you're here?  This could just be:
> 
> #if !defined(OS_LINUX) && !defined(OS_ANDROID)
>   if (!proxy_config_service_) {
>     proxy_config_service_ = ProxyService::CreateSystemProxyConfigService(
>         ...);
>   }
> #endif  //  !defined(OS_LINUX) && !defined(OS_ANDROID)
> 
> And below use proxy_config_service_.Pass(), and get rid of the
> proxy_config_service local completely.

Done. much nicer.

https://codereview.chromium.org/1356933002/diff/110001/remoting/base/url_requ...
File remoting/base/url_request_context_getter.cc (right):

https://codereview.chromium.org/1356933002/diff/110001/remoting/base/url_requ...
remoting/base/url_request_context_getter.cc:21: network_task_runner_,
file_task_runner);
On 2015/09/21 21:34:51, mmenke wrote:
> nit:  While you're here, remove the underscore at the end of
> network_task_runner_?  (Weird that we use an argument for one, and a member
> variable for the other).

Done.

[Charlie Harrison, 2015-09-22 18:02:14]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2015-09-22 18:02:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1356933002/#ps190001
(title: "proper namespacing")

[commit-bot: I haz the power, 2015-09-22 18:03:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356933002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356933002/190001

[commit-bot: I haz the power, 2015-09-22 18:19:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 18:19:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2015-09-22 19:03:51]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-09-22 19:05:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356933002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356933002/190001

[commit-bot: I haz the power, 2015-09-22 19:13:13]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2015-09-22 19:14:10]

Patchset 11 (id:??) landed as
https://crrev.com/b7e3a08f4c62c69093ecb3a258e18f2cdd340b08
Cr-Commit-Position: refs/heads/master@{#350207}