Adjust URLFetcher::Create API so that object is returned as scoped_ptr.

Adjust URLFetcher::Create API so that object is returned as scoped_ptr.

Most interfaces were storing the object in a scoped_ptr already. This
adjusts the API so that it is a little clearer of the ownership
transfer.

A number of clients put the URLFetcher in a table and do memory
management on it themselves; this is likely templatable code
for a future CL. The scope of this CL was to change the API but
no control flow changes. Making this change found one memory leak;
(http://crbug.com/482459) has been addressed separately.

BUG=371201
TESTS=net_unittests google_api_unittests
TBR=jochen@chromium.org, thakis@chromium.org, oshima@chromium.org, armansito@chromium.org, reillyg@chromium.org, rogerta@chromium.org, stuartmorgan@chromium.org, wez@chromium.org, pavely@chromium.org, rouslan@chromium.org

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

[dtapuska, 2015-04-29 21:13:31]

dtapuska@chromium.org changed reviewers:
+ jam@chromium.org, rsleevi@chromium.org

[dtapuska, 2015-04-29 21:13:32]

Adding Ryan and jam as reviewers. Since Ryan opened the bug; and since this is
largely a grep and replace issue; it touches a lot of files so need a top level
owner.

[vabr (Chromium), 2015-04-30 04:44:39]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-04-30 04:44:40]

Drive by: components/password_manager/core/browser/affiliation_fetcher.cc LGTM
Cheers,
Vaclav

[vabr (Chromium), 2015-04-30 04:45:46]

On 2015/04/30 04:44:40, vabr (Chromium) wrote:
> Drive by: components/password_manager/core/browser/affiliation_fetcher.cc LGTM
> Cheers,
> Vaclav

Also chrome/browser/profile_resetter/brandcode_config_fetcher.cc LGTM.

[jam, 2015-04-30 15:42:35]

On 2015/04/29 21:13:32, Dave Tapuska wrote:
> Adding Ryan and jam as reviewers. Since Ryan opened the bug; and since this is
> largely a grep and replace issue; it touches a lot of files so need a top
level
> owner.

per
https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...,
you just need review from the net owner (rsleevi in this case) and then you can
TBR the others.

[Ryan Sleevi, 2015-04-30 18:23:45]

Thanks for doing this Dave; I started reviewing this and then noticed your
remark about the mechnical nature of this. I'm a little nervous about the Foo* =
CreateFoo().release() pattern, but to some extent, that's "not your fault", so
you can ignore most of my grumbles about .release()

One nit below re: expectations, otherwise LGTM

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/attestation/attestation_ca_client.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/attestation_ca_client.cc:82: .release();
Ideally, you'd move the .release() to line 88 and have this be a scoped_ptr<>

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc
(right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:98:
EXPECT_TRUE(fetcher.get());
EXPECT_TRUE(fetcher)

should be sufficient (with the bool converter helper)

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:99:
return fetcher;
This should be fetcher.Pass(), right?

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_ui_bindings.cc:593:
net::URLFetcher::Create(gurl, net::URLFetcher::GET, this).release();
maybe move this .release() to 594

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/blacklist_state_fetcher.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/blacklist_state_fetcher.cc:144: net::URLFetcher::POST,
this).release();
ditto line 145

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/intranet...
File chrome/browser/intranet_redirect_detector.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/intranet...
chrome/browser/intranet_redirect_detector.cc:85: .release();
Ditto moving this to line 92

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc_log_uploader.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_log_uploader.cc:453: this).release();
line 457

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_service.cc:332:
net::URLFetcher::POST, this).release();
line 338

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_service.cc:373:
net::URLFetcher::POST, this).release();
line 379

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ping_manager.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ping_manager.cc:102: this).release();
line 107

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ping_manager.cc:117: .release();
line 124

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:188: net::URLFetcher::POST,
this).release();
line 189

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/srt_fetcher_win.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_win.cc:79: this).Pass()) {
Is the .Pass() needed here for these emplacements? I didn't think so, but memory
fails me :)

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/spellche...
File chrome/browser/spellchecker/feedback_sender.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/spellche...
chrome/browser/spellchecker/feedback_sender.cc:412: net::URLFetcher::POST,
this).release();
line 416

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/spellche...
File chrome/browser/spellchecker/spelling_service_client.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/spellche...
chrome/browser/spellchecker/spelling_service_client.cc:93: net::URLFetcher*
fetcher = CreateURLFetcher(url).release();
line 98

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/devtools_ui.cc:211: net::URLFetcher::Create(url,
net::URLFetcher::GET, this).release();
line 212

[dtapuska, 2015-04-30 19:05:57]

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_service.cc:373:
net::URLFetcher::POST, this).release();
On 2015/04/30 18:23:44, Ryan Sleevi wrote:
> line 379

That is the way I started doing this CL; changing to a scoped_ptr and putting a
release on line 379.. but the problem is once the scoped_ptr is released; the
members call the ptr... ie; on like 381 calls fetcher-> and if you added it at
379; then it would be a SEGFAULT. So you could move the code around; putting it
in the table last (but I couldn't convince myself that was correct because it
probably should be in the table *before* Start() is called in-case it fails
synchronously and calls the callback and removes it from the table...

Anyways; I ended up with doing it this way to be the least invasive and
bug-free...

I don't agree with all these classes putting the URLFetcher as the key to the
map. I think the id used to create the fetcher should be used; and then the data
can be a struct of two scoped_ptrs... but then again we don't support std::move
in chromium yet..

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/srt_fetcher_win.cc (right):

https://codereview.chromium.org/1117703002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_win.cc:79: this).Pass()) {
On 2015/04/30 18:23:44, Ryan Sleevi wrote:
> Is the .Pass() needed here for these emplacements? I didn't think so, but
memory
> fails me :)

Pass() is the safe bet.. I was concerned about compilers which I didn't have
available to me.. ie; MSVC

[dtapuska, 2015-05-01 13:51:44]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2015-05-01 13:51:45]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1117703002/#ps80001
(title: "Remove unneeded Pass() calls")

[commit-bot: I haz the power, 2015-05-01 13:52:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1117703002/80001

[commit-bot: I haz the power, 2015-05-01 13:58:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-01 14:27:06]

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