make ProxyService::CreateSystemProxyConfigService return scoped_ptrs NOT FOR REVIEW

This revision has been superseded by https://codereview.chromium.org/1356933002/
BUG=523075

[Charlie Harrison, 2015-08-20 22:17:32]

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

[Randy Smith (Not in Mondays), 2015-08-24 21:58:19]

Charles: Answering your IM question in the form of a drive-by review :-}.

https://codereview.chromium.org/1301333002/diff/40001/content/shell/browser/s...
File content/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/1301333002/diff/40001/content/shell/browser/s...
content/shell/browser/shell_url_request_context_getter.cc:98: .get();
I think this is broken.  What'll happen is the
net::ProxyService::CreateSystemProxyConfigService will return a scoped_ptr<>,
which this routine will get the value of, and then the scoped_ptr will go out of
scope and the underlying object will be deleted.  

.release() will avoid the double free and appropriately gasket between the two
interfaces (since this function passes ownership back, just without the
scoped_ptr<>) but the ideal would be to base your CL on my CL so that you could
just do the return and the scoped_ptr<> would go back up the stack.  

In case you don't know how to do that: Create a clean branch off of master
(Randy_<cl#> or something--name's arbitrary).  Do a "git cl patch 1290243007". 
Then shift over to your branch and do a "git merge Randy_<cl#>".  There'll be
conflicts (at least here, I think :-}), resolve them and commit the merge.  Then
do "git branch --set-upstream-to=Randy_<cl#>".  Then "git cl upload" with some
descriptive text for the patchset.

The result will be a diff in Rietveld which is the diff from my patch, but when
you submit try jobs it'll automatically pull in my patch before running the try
job.  Which is convenient for this sort of change.

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

https://codereview.chromium.org/1301333002/diff/40001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:988: make_scoped_ptr(new
ProxyConfigServiceDirect).Pass(),
As you can see from the line underneath, you don't actually need the .Pass() if
the scoped_ptr<> is about to go out of scope (as is generally true for a
temporary)--the compiler actually figures that out and accepts the un-decorated
scoped pointer.  I don't understand the magic behind it, but I know it works
:-}.  The effect is that you only need .Pass() on a named variable that isn't
being returned from a function.

https://codereview.chromium.org/1301333002/diff/40001/net/proxy/proxy_service...
File net/proxy/proxy_service_mojo_unittest.cc (right):

https://codereview.chromium.org/1301333002/diff/40001/net/proxy/proxy_service...
net/proxy/proxy_service_mojo_unittest.cc:129: proxy_service_ =
make_scoped_ptr(CreateProxyServiceUsingMojoFactory(
Why not just leave it as .reset() if it's getting a raw pointer?  I think that's
the more common idiom.

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

https://codereview.chromium.org/1301333002/diff/40001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:285:
proxy_config_service_.release());
I think that .Pass() would be more idiomatic here than .release().

https://codereview.chromium.org/1301333002/diff/40001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:289:
proxy_config_service.reset(proxy_config_service_.release());
Why not "proxy_config_service = proxy_config_service_.Pass();"?

[Charlie Harrison, 2015-08-24 22:00:35]

On 2015/08/24 21:58:19, rdsmith wrote:
> Charles: Answering your IM question in the form of a drive-by review :-}.
> 
>
https://codereview.chromium.org/1301333002/diff/40001/content/shell/browser/s...
> File content/shell/browser/shell_url_request_context_getter.cc (right):
> 
>
https://codereview.chromium.org/1301333002/diff/40001/content/shell/browser/s...
> content/shell/browser/shell_url_request_context_getter.cc:98: .get();
> I think this is broken.  What'll happen is the
> net::ProxyService::CreateSystemProxyConfigService will return a scoped_ptr<>,
> which this routine will get the value of, and then the scoped_ptr will go out
of
> scope and the underlying object will be deleted.  
> 
> .release() will avoid the double free and appropriately gasket between the two
> interfaces (since this function passes ownership back, just without the
> scoped_ptr<>) but the ideal would be to base your CL on my CL so that you
could
> just do the return and the scoped_ptr<> would go back up the stack.  
> 
> In case you don't know how to do that: Create a clean branch off of master
> (Randy_<cl#> or something--name's arbitrary).  Do a "git cl patch 1290243007".

> Then shift over to your branch and do a "git merge Randy_<cl#>".  There'll be
> conflicts (at least here, I think :-}), resolve them and commit the merge. 
Then
> do "git branch --set-upstream-to=Randy_<cl#>".  Then "git cl upload" with some
> descriptive text for the patchset.
> 
> The result will be a diff in Rietveld which is the diff from my patch, but
when
> you submit try jobs it'll automatically pull in my patch before running the
try
> job.  Which is convenient for this sort of change.
> 
>
https://codereview.chromium.org/1301333002/diff/40001/net/proxy/proxy_service.cc
> File net/proxy/proxy_service.cc (right):
> 
>
https://codereview.chromium.org/1301333002/diff/40001/net/proxy/proxy_service...
> net/proxy/proxy_service.cc:988: make_scoped_ptr(new
> ProxyConfigServiceDirect).Pass(),
> As you can see from the line underneath, you don't actually need the .Pass()
if
> the scoped_ptr<> is about to go out of scope (as is generally true for a
> temporary)--the compiler actually figures that out and accepts the
un-decorated
> scoped pointer.  I don't understand the magic behind it, but I know it works
> :-}.  The effect is that you only need .Pass() on a named variable that isn't
> being returned from a function.
> 
>
https://codereview.chromium.org/1301333002/diff/40001/net/proxy/proxy_service...
> File net/proxy/proxy_service_mojo_unittest.cc (right):
> 
>
https://codereview.chromium.org/1301333002/diff/40001/net/proxy/proxy_service...
> net/proxy/proxy_service_mojo_unittest.cc:129: proxy_service_ =
> make_scoped_ptr(CreateProxyServiceUsingMojoFactory(
> Why not just leave it as .reset() if it's getting a raw pointer?  I think
that's
> the more common idiom.
> 
>
https://codereview.chromium.org/1301333002/diff/40001/net/url_request/url_req...
> File net/url_request/url_request_context_builder.cc (right):
> 
>
https://codereview.chromium.org/1301333002/diff/40001/net/url_request/url_req...
> net/url_request/url_request_context_builder.cc:285:
> proxy_config_service_.release());
> I think that .Pass() would be more idiomatic here than .release().
> 
>
https://codereview.chromium.org/1301333002/diff/40001/net/url_request/url_req...
> net/url_request/url_request_context_builder.cc:289:
> proxy_config_service.reset(proxy_config_service_.release());
> Why not "proxy_config_service = proxy_config_service_.Pass();"?

Thanks for the comments! I'll work on these changes tomorrow.

[mmenke, 2015-09-11 17:04:29]

csharrison:  Are you planning on picking this up any time soon, or can I remove
myself as a reviewer?  Not a big deal, either way, I'm just trying to trim down
my pending review list.

[Charlie Harrison, 2015-09-11 17:05:26]

On 2015/09/11 17:04:29, mmenke wrote:
> csharrison:  Are you planning on picking this up any time soon, or can I
remove
> myself as a reviewer?  Not a big deal, either way, I'm just trying to trim
down
> my pending review list.

Yeah I'm waiting on Randy's change to land, which should be any day now. Feel
free to remove yourself if you're bogged down, we can find others :)

[mmenke, 2015-09-11 17:09:38]

On 2015/09/11 17:05:26, csharrison wrote:
> On 2015/09/11 17:04:29, mmenke wrote:
> > csharrison:  Are you planning on picking this up any time soon, or can I
> remove
> > myself as a reviewer?  Not a big deal, either way, I'm just trying to trim
> down
> > my pending review list.
> 
> Yeah I'm waiting on Randy's change to land, which should be any day now. Feel
> free to remove yourself if you're bogged down, we can find others :)

Not so much bogged down, as I have 20 reviews on my list, most of which require
no action on my part.  It's difficult to figure out which ones actually do
require action when there are that many that don't.  I'm going to go ahead and
remove myself from this CL, but feel free to add me back when it's ready for
review.

[mmenke, 2015-09-11 17:09:47]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[Charlie Harrison, 2015-09-18 15:47:55]

csharrison@chromium.org changed reviewers:
- rdsmith@chromium.org