Returning scoped_ptr<> instead of raw pointer in DhcpProxyScriptFetcherFactory::Create

Returning scoped_ptr<> instead of raw pointer in DhcpProxyScriptFetcherFactory::Create

BUG=472381
Committed: https://crrev.com/1dfe37962765a6a7b829085f127b638939ec3778
Cr-Commit-Position: refs/heads/master@{#345376}

[Paritosh Kumar, 2015-08-14 13:12:44]

The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-14 13:12:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273013004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273013004/60001

[commit-bot: I haz the power, 2015-08-14 13:38:14]

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

[commit-bot: I haz the power, 2015-08-14 13:38:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Paritosh Kumar, 2015-08-19 06:04:10]

paritosh.in@samsung.com changed reviewers:
+ eroman@chromium.org

[Paritosh Kumar, 2015-08-19 06:04:10]

Please have a look.

[eroman, 2015-08-19 22:08:40]

looks good after these comments

https://codereview.chromium.org/1273013004/diff/100001/chrome/browser/net/pro...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1273013004/diff/100001/chrome/browser/net/pro...
chrome/browser/net/proxy_service_factory.cc:155:
scoped_ptr<net::DhcpProxyScriptFetcher> dhcp_proxy_script_fetcher = nullptr;
leave off "=nullptr".

(Default initialization sets to nullptr and code doesn't commonly specify it)

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/dhcp_proxy_s...
File net/proxy/dhcp_proxy_script_fetcher_factory.h (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/dhcp_proxy_s...
net/proxy/dhcp_proxy_script_fetcher_factory.h:37: // Ownership is transferred to
the caller. url_request_context must be valid
Delete the first sentence  "Ownership is transferred to the caller. " as this is
not explicit in the API

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service.cc:1447:
dhcp_proxy_script_fetcher_.reset(dhcp_proxy_script_fetcher.release());
dhcp_proxy_script_fetcher_ = dhcp_proxy_script_fetcher.Pass();

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

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service.h:196: // ProxyService takes ownership of both objects.
Can you update this comment to "ProxyService takes ownership of
proxy_script_fetcher".

Since the other one is now implied by the API

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
File net/proxy/proxy_service_mojo.h (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_mojo.h:35: // resulting ProxyService will take ownership
of it.
Delete this last sentence since it is now redundant with the API

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:946: scoped_ptr<DhcpProxyScriptFetcher>
dhcp_fetcher(
Please remove this variable and inline it into the call below (will prevent
accidental use of "dhcp_fetcher" since it is going to be nulled by the call
below.

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:1864: scoped_ptr<DhcpProxyScriptFetcher>(
optional: You can probably use:
    make_scoped_ptr(new DoNothingDhcProxyScriptFetcher())

To make these shorter (since it infers the template parameter to scoped_ptr<>).

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:2003: scoped_ptr<DhcpProxyScriptFetcher>(
See comment above, and throughout this file.

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
File net/proxy/proxy_service_v8.h (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_v8.h:30: // resulting ProxyService will take ownership
of it.
Please delete this last sentence.

[Paritosh Kumar, 2015-08-20 15:23:35]

Thanks eroman

Addressed your comments in updated cl.

Please have a look.

https://codereview.chromium.org/1273013004/diff/100001/chrome/browser/net/pro...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1273013004/diff/100001/chrome/browser/net/pro...
chrome/browser/net/proxy_service_factory.cc:155:
scoped_ptr<net::DhcpProxyScriptFetcher> dhcp_proxy_script_fetcher = nullptr;
On 2015/08/19 22:08:39, eroman wrote:
> leave off "=nullptr".
> 
> (Default initialization sets to nullptr and code doesn't commonly specify it)

Done.

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/dhcp_proxy_s...
File net/proxy/dhcp_proxy_script_fetcher_factory.h (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/dhcp_proxy_s...
net/proxy/dhcp_proxy_script_fetcher_factory.h:37: // Ownership is transferred to
the caller. url_request_context must be valid
On 2015/08/19 22:08:39, eroman wrote:
> Delete the first sentence  "Ownership is transferred to the caller. " as this
is
> not explicit in the API

Done.

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service.cc:1447:
dhcp_proxy_script_fetcher_.reset(dhcp_proxy_script_fetcher.release());
On 2015/08/19 22:08:39, eroman wrote:
> dhcp_proxy_script_fetcher_ = dhcp_proxy_script_fetcher.Pass();

Done.

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

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service.h:196: // ProxyService takes ownership of both objects.
On 2015/08/19 22:08:39, eroman wrote:
> Can you update this comment to "ProxyService takes ownership of
> proxy_script_fetcher".
> 
> Since the other one is now implied by the API

Done.

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
File net/proxy/proxy_service_mojo.h (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_mojo.h:35: // resulting ProxyService will take ownership
of it.
On 2015/08/19 22:08:39, eroman wrote:
> Delete this last sentence since it is now redundant with the API

Done.

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:946: scoped_ptr<DhcpProxyScriptFetcher>
dhcp_fetcher(
On 2015/08/19 22:08:39, eroman wrote:
> Please remove this variable and inline it into the call below (will prevent
> accidental use of "dhcp_fetcher" since it is going to be nulled by the call
> below.

Done.

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:1864: scoped_ptr<DhcpProxyScriptFetcher>(
On 2015/08/19 22:08:39, eroman wrote:
> optional: You can probably use:
>     make_scoped_ptr(new DoNothingDhcProxyScriptFetcher())
> 
> To make these shorter (since it infers the template parameter to
scoped_ptr<>).

Done.

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:2003: scoped_ptr<DhcpProxyScriptFetcher>(
On 2015/08/19 22:08:39, eroman wrote:
> See comment above, and throughout this file.

Done.

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
File net/proxy/proxy_service_v8.h (right):

https://codereview.chromium.org/1273013004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service_v8.h:30: // resulting ProxyService will take ownership
of it.
On 2015/08/19 22:08:39, eroman wrote:
> Please delete this last sentence.

Done.

[eroman, 2015-08-21 20:03:12]

lgtm

https://codereview.chromium.org/1273013004/diff/120001/net/proxy/dhcp_proxy_s...
File net/proxy/dhcp_proxy_script_fetcher_factory.cc (right):

https://codereview.chromium.org/1273013004/diff/120001/net/proxy/dhcp_proxy_s...
net/proxy/dhcp_proxy_script_fetcher_factory.cc:24: return
scoped_ptr<DhcpProxyScriptFetcher>(
I suggest using "make_scoped_ptr(new DoNothingDhcpProxyScriptFetcher)" for these
instances (saves a bit of typing), but up to you.

https://codereview.chromium.org/1273013004/diff/120001/net/proxy/dhcp_proxy_s...
net/proxy/dhcp_proxy_script_fetcher_factory.cc:28:
scoped_ptr<DhcpProxyScriptFetcher> ret(nullptr);
remove "nullptr". It certainly isn't wrong, however chromium code tends to omit
it since the default constructor will have the same effect.

https://codereview.chromium.org/1273013004/diff/120001/net/proxy/proxy_servic...
File net/proxy/proxy_service_mojo_unittest.cc (right):

https://codereview.chromium.org/1273013004/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service_mojo_unittest.cc:132: fetcher_,
scoped_ptr<DhcpProxyScriptFetcher>(
optional: use make_scoped_ptr(new DoNothing...);

[Paritosh Kumar, 2015-08-25 12:13:08]

On 2015/08/21 20:03:12, eroman wrote:
> lgtm
> 
>
https://codereview.chromium.org/1273013004/diff/120001/net/proxy/dhcp_proxy_s...
> File net/proxy/dhcp_proxy_script_fetcher_factory.cc (right):
> 
>
https://codereview.chromium.org/1273013004/diff/120001/net/proxy/dhcp_proxy_s...
> net/proxy/dhcp_proxy_script_fetcher_factory.cc:24: return
> scoped_ptr<DhcpProxyScriptFetcher>(
> I suggest using "make_scoped_ptr(new DoNothingDhcpProxyScriptFetcher)" for
these
> instances (saves a bit of typing), but up to you.
> 
>
https://codereview.chromium.org/1273013004/diff/120001/net/proxy/dhcp_proxy_s...
> net/proxy/dhcp_proxy_script_fetcher_factory.cc:28:
> scoped_ptr<DhcpProxyScriptFetcher> ret(nullptr);
> remove "nullptr". It certainly isn't wrong, however chromium code tends to
omit
> it since the default constructor will have the same effect.
> 
>
https://codereview.chromium.org/1273013004/diff/120001/net/proxy/proxy_servic...
> File net/proxy/proxy_service_mojo_unittest.cc (right):
> 
>
https://codereview.chromium.org/1273013004/diff/120001/net/proxy/proxy_servic...
> net/proxy/proxy_service_mojo_unittest.cc:132: fetcher_,
> scoped_ptr<DhcpProxyScriptFetcher>(
> optional: use make_scoped_ptr(new DoNothing...);

Thanks. Updated.

[Paritosh Kumar, 2015-08-25 12:13:16]

The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-25 12:13:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273013004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273013004/140001

[commit-bot: I haz the power, 2015-08-25 15:30:48]

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

[commit-bot: I haz the power, 2015-08-25 15:30:49]

Dry run: This issue passed the CQ dry run.

[Paritosh Kumar, 2015-08-25 17:50:16]

The CQ bit was checked by paritosh.in@samsung.com

[Paritosh Kumar, 2015-08-25 17:50:17]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1273013004/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-08-25 17:50:35]

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

[commit-bot: I haz the power, 2015-08-25 17:57:55]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-25 17:58:32]

Patchset 8 (id:??) landed as
https://crrev.com/1dfe37962765a6a7b829085f127b638939ec3778
Cr-Commit-Position: refs/heads/master@{#345376}