Using scoped_ptr for URLRequestJobFactoryImpl::SetProtocolHandler

Using scoped_ptr for URLRequestJobFactoryImpl::SetProtocolHandler

Adding scoped_ptr to call in order to take ownership of handler.

BUG=517161
Committed: https://crrev.com/5d58c9eb2baa203be1b84ac88cde82c59d72f143
Cr-Commit-Position: refs/heads/master@{#345185}

[svaldez, 2015-08-18 17:18:53]

svaldez@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2015-08-18 20:40:37]

Thanks!

https://codereview.chromium.org/1295523006/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1178:
make_scoped_ptr(extensions::CreateExtensionProtocolHandler(
This would probably be better having CreateExtensionProtocolHandler itself
return a scoped_ptr.

https://codereview.chromium.org/1295523006/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1183:
make_scoped_ptr(CreateExtensionResourceProtocolHandler()));
Ditto.

https://codereview.chromium.org/1295523006/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1201:
make_scoped_ptr(content::ContentProtocolHandler::Create(
Ditto.

https://codereview.chromium.org/1295523006/diff/40001/chromecast/browser/url_...
File chromecast/browser/url_request_context_factory.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/chromecast/browser/url_...
chromecast/browser/url_request_context_factory.cc:241: it->first,
make_scoped_ptr(it->second.release()));
[Hrmf. If it->second were a scoped_ptr rather than a linked_ptr because we lack
move, this could be it->second.Pass() without anything extra.]

https://codereview.chromium.org/1295523006/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_blob_to_disk_cache_unittest.cc
(right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_blob_to_disk_cache_unittest.cc:121:
make_scoped_ptr(CreateMockBlobProtocolHandler(blob_storage_context_)));
CreateMockBlobProtocolHandler could just return a scoped_ptr.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache_unittest.cc:262:
blob_storage_context->context())));
CreateMockBlobProtocolHandler could just return a scoped_ptr.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:167:
make_scoped_ptr(net::URLRequestTestJob::CreateProtocolHandler()));
CreateProtocolHandler could just return a scoped_ptr.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:562:
make_scoped_ptr(CreateProtocolHandler()));
CreateProtocolHandler could just return a scoped_ptr.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:191:
make_scoped_ptr(CreateMockBlobProtocolHandler(blob_storage_context)));
CreateMockBlobProtocolHandler could just return a scoped_ptr.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend_unittest.cc:46: "chrome",
make_scoped_ptr(URLDataManagerBackend::CreateProtocolHandler(
URLDataManagerBackend::CreateProtocolHandler coudl just return a scoped_ptr.

https://codereview.chromium.org/1295523006/diff/40001/net/url_request/url_req...
File net/url_request/url_request_job_factory_impl.h (right):

https://codereview.chromium.org/1295523006/diff/40001/net/url_request/url_req...
net/url_request/url_request_job_factory_impl.h:28: // URLRequestJobFactory takes
ownership of |protocol_handler|.
Remove "On success, [...] takes ownership [...]" since that's reflected in the
type (and no longer accurate).

[svaldez, 2015-08-18 22:12:44]

https://codereview.chromium.org/1295523006/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1178:
make_scoped_ptr(extensions::CreateExtensionProtocolHandler(
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> This would probably be better having CreateExtensionProtocolHandler itself
> return a scoped_ptr.

Done.

https://codereview.chromium.org/1295523006/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1183:
make_scoped_ptr(CreateExtensionResourceProtocolHandler()));
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> Ditto.

Done.

https://codereview.chromium.org/1295523006/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1201:
make_scoped_ptr(content::ContentProtocolHandler::Create(
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> Ditto.

Done.

https://codereview.chromium.org/1295523006/diff/40001/chromecast/browser/url_...
File chromecast/browser/url_request_context_factory.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/chromecast/browser/url_...
chromecast/browser/url_request_context_factory.cc:241: it->first,
make_scoped_ptr(it->second.release()));
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> [Hrmf. If it->second were a scoped_ptr rather than a linked_ptr because we
lack
> move, this could be it->second.Pass() without anything extra.]

Acknowledged.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_blob_to_disk_cache_unittest.cc
(right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_blob_to_disk_cache_unittest.cc:121:
make_scoped_ptr(CreateMockBlobProtocolHandler(blob_storage_context_)));
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> CreateMockBlobProtocolHandler could just return a scoped_ptr.

Done.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache_unittest.cc:262:
blob_storage_context->context())));
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> CreateMockBlobProtocolHandler could just return a scoped_ptr.

Done.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:167:
make_scoped_ptr(net::URLRequestTestJob::CreateProtocolHandler()));
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> CreateProtocolHandler could just return a scoped_ptr.

Done.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:562:
make_scoped_ptr(CreateProtocolHandler()));
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> CreateProtocolHandler could just return a scoped_ptr.

Done.

https://codereview.chromium.org/1295523006/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/1295523006/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:191:
make_scoped_ptr(CreateMockBlobProtocolHandler(blob_storage_context)));
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> CreateMockBlobProtocolHandler could just return a scoped_ptr.

Done.

https://codereview.chromium.org/1295523006/diff/40001/net/url_request/url_req...
File net/url_request/url_request_job_factory_impl.h (right):

https://codereview.chromium.org/1295523006/diff/40001/net/url_request/url_req...
net/url_request/url_request_job_factory_impl.h:28: // URLRequestJobFactory takes
ownership of |protocol_handler|.
On 2015/08/18 20:40:37, David Benjamin (slow) wrote:
> Remove "On success, [...] takes ownership [...]" since that's reflected in the
> type (and no longer accurate).

Done.

[davidben, 2015-08-18 22:19:37]

Did you forget to upload the new version? :-)

[davidben, 2015-08-19 19:53:22]

Sorry, basically all of the comments below were ones I should have noticed the
first time around. :-( lgtm with those comments, since this is all pretty
trivial and you'll get the corresponding OWNERS' review anyway.

You have //net and //content from me. You'll want to hunt down a few others.
Generally people on more top-level directories prefer you find lower-level
OWNERS where possible. (Particularly //chrome and //components which are kind of
dumping grounds.) Also you should mention when adding people which files you
want them to look at.

https://codereview.chromium.org/1295523006/diff/90001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.cc (right):

https://codereview.chromium.org/1295523006/diff/90001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:78:
make_scoped_ptr(protocol_handler));
Oh hrmf. I should have noticed this in the first pass. This probably should also
take scoped_ptr<ProtocolHandler>. I believe the only files you have to modify
for its callers are aw_url_request_context_getter.cc and
android_stream_reader_url_request_job_unittest.cc

https://codereview.chromium.org/1295523006/diff/90001/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols_unittest.cc (left):

https://codereview.chromium.org/1295523006/diff/90001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:124:
extension_info_map_.get()));
(This file ends up not changing now. Slightly smaller CL if you take it out.
*shrug*)

https://codereview.chromium.org/1295523006/diff/90001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1295523006/diff/90001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc:65:
url::kHttpScheme, make_scoped_ptr(test_job_interceptor_)));
Huh. Actually it doesn't look like test_job_interceptor_ even does anything
here. Perhaps just kill the comment and member and just make_scoped_ptr(new
Thingy).

https://codereview.chromium.org/1295523006/diff/90001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/90001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend_unittest.cc:46: "chrome",
make_scoped_ptr(URLDataManagerBackend::CreateProtocolHandler(
Return a scoped_ptr?

[svaldez, 2015-08-20 14:17:20]

svaldez@chromium.org changed reviewers:
+ asargent@chromium.org, joedow@chromium.org, lcwu@chromium.org,
sgurun@chromium.org, sky@chromium.org, stuartmorgan@chromium.org

[svaldez, 2015-08-20 14:17:20]

svaldez@chromium.org changed required reviewers:
+ asargent@chromium.org, davidben@chromium.org, joedow@chromium.org,
lcwu@chromium.org, sgurun@chromium.org, sky@chromium.org,
stuartmorgan@chromium.org

[svaldez, 2015-08-20 14:17:21]

Hi,

I'm currently doing a re-write of the SetProtocolHandler to used scoped_ptr
instead of the passing it is currently doing and its touching a number of files
across chromium. If you could take a look and comment on/approve changes to the
files you have ownership on, that would be helpful. The files I need approval on
from each person are:

sgurun:
  android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc
  android_webview/browser/net/aw_url_request_context_getter.cc
  android_webview/browser/net/aw_url_request_job_factory.cc
  android_webview/browser/net/aw_url_request_job_factory.h

sky:
  chrome/browser/io_thread.cc
 
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc

stuartmorgan:
  ios/crnet/crnet_environment.mm
  ios/net/protocol_handler_util_unittest.mm
  ios/web/net/request_tracker_impl_unittest.mm
  ios/web/shell/shell_url_request_context_getter.cc
  ios/web/webui/url_data_manager_ios_backend.cc
  ios/web/webui/url_data_manager_ios_backend.h

asargent:
  chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
  chrome/browser/extensions/extension_protocols_unittest.cc
  chrome/browser/extensions/extension_resource_protocols.cc
  chrome/browser/extensions/extension_resource_protocols.h
  extensions/browser/extension_protocols.cc
  extensions/browser/extension_protocols.h
  extensions/shell/browser/shell_content_browser_client.cc

lcwu:
  chromecast/browser/url_request_context_factory.cc

joedow:
  remoting/host/token_validator_factory_impl_unittest.cc

Thanks,
Steven

https://codereview.chromium.org/1295523006/diff/90001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.cc (right):

https://codereview.chromium.org/1295523006/diff/90001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:78:
make_scoped_ptr(protocol_handler));
On 2015/08/19 19:53:22, David Benjamin (slow) wrote:
> Oh hrmf. I should have noticed this in the first pass. This probably should
also
> take scoped_ptr<ProtocolHandler>. I believe the only files you have to modify
> for its callers are aw_url_request_context_getter.cc and
> android_stream_reader_url_request_job_unittest.cc

Done.

https://codereview.chromium.org/1295523006/diff/90001/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols_unittest.cc (left):

https://codereview.chromium.org/1295523006/diff/90001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:124:
extension_info_map_.get()));
On 2015/08/19 19:53:22, David Benjamin (slow) wrote:
> (This file ends up not changing now. Slightly smaller CL if you take it out.
> *shrug*)

Acknowledged.

https://codereview.chromium.org/1295523006/diff/90001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1295523006/diff/90001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc:65:
url::kHttpScheme, make_scoped_ptr(test_job_interceptor_)));
On 2015/08/19 19:53:22, David Benjamin (slow) wrote:
> Huh. Actually it doesn't look like test_job_interceptor_ even does anything
> here. Perhaps just kill the comment and member and just make_scoped_ptr(new
> Thingy).

Done.

https://codereview.chromium.org/1295523006/diff/90001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/90001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend_unittest.cc:46: "chrome",
make_scoped_ptr(URLDataManagerBackend::CreateProtocolHandler(
On 2015/08/19 19:53:22, David Benjamin (slow) wrote:
> Return a scoped_ptr?

Done.

[lcwu1, 2015-08-20 15:50:20]

chromecast/ lgtm

[sky, 2015-08-20 20:08:40]

LGTM

[stuartmorgan, 2015-08-20 21:02:51]

ios lgtm

[asargent_no_longer_on_chrome, 2015-08-20 21:57:01]

extensions files lgtm

https://codereview.chromium.org/1295523006/diff/150001/chrome/browser/extensi...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/150001/chrome/browser/extensi...
chrome/browser/extensions/extension_protocols_unittest.cc:123: is_incognito,
extension_info_map_.get()));
Looks like this is just a formatting change, not a functional one - did you
intend to keep this in your CL? Fine by me either way. 

(I'm guessing you might have had a functional change here at one point, and then
ran 'git cl format', but then ended up removing the functional change but not
the formatting change?)

[svaldez, 2015-08-20 21:59:12]

https://codereview.chromium.org/1295523006/diff/150001/chrome/browser/extensi...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/1295523006/diff/150001/chrome/browser/extensi...
chrome/browser/extensions/extension_protocols_unittest.cc:123: is_incognito,
extension_info_map_.get()));
On 2015/08/20 21:57:01, Antony Sargent wrote:
> Looks like this is just a formatting change, not a functional one - did you
> intend to keep this in your CL? Fine by me either way. 
> 
> (I'm guessing you might have had a functional change here at one point, and
then
> ran 'git cl format', but then ended up removing the functional change but not
> the formatting change?)
> 
> 
> 

Yeah, it was a functional change until we decided to have
CreateExtensionProtocolHandler return a scoped_ptr instead of making it here.
I'll remove the file from the CL.

[sgurun-gerrit only, 2015-08-20 22:18:55]

lgtm

[svaldez, 2015-08-20 22:23:40]

svaldez@chromium.org changed reviewers:
+ jeremyim@chromium.org
- asargent@chromium.org, davidben@chromium.org, joedow@chromium.org,
lcwu@chromium.org, sgurun@chromium.org, sky@chromium.org,
stuartmorgan@chromium.org

[svaldez, 2015-08-20 22:23:40]

svaldez@chromium.org changed required reviewers:
+ jeremyim@chromium.org
- asargent@chromium.org, davidben@chromium.org, joedow@chromium.org,
lcwu@chromium.org, sgurun@chromium.org, sky@chromium.org,
stuartmorgan@chromium.org

[svaldez, 2015-08-20 22:23:41]

Hi,

I'm currently doing a re-write of the SetProtocolHandler to used scoped_ptr
instead of the passing it is currently doing and its touching a number of files
across chromium. If you could take a look and comment on/approve changes to the
files you have ownership on, that would be helpful. The files I need approval on
are:


   
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc
   
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc
   
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc

Thanks,
Steven

[jeremyim, 2015-08-20 22:31:12]

data_reduction_proxy lgtm

[svaldez, 2015-08-21 13:45:40]

svaldez@chromium.org changed reviewers:
+ jamiewalch@chromium.org
- jeremyim@chromium.org

[svaldez, 2015-08-21 13:45:40]

svaldez@chromium.org changed required reviewers:
+ jamiewalch@chromium.org
- jeremyim@chromium.org

[svaldez, 2015-08-21 13:45:41]

Hi,

I'm currently doing a re-write of the SetProtocolHandler to used scoped_ptr
instead of the passing it is currently doing and its touching a number of files
across chromium. If you could take a look and comment on/approve changes to
remoting/host/token_validator_factory_impl_unittest.cc, I think that's the last
file I need a LGTM on.

Thanks,
Steven

[Jamie, 2015-08-24 20:45:50]

remoting/ LGTM

[svaldez, 2015-08-24 20:46:32]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-08-24 20:46:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, sky@chromium.org, stuartmorgan@chromium.org,
asargent@chromium.org, lcwu@chromium.org
Link to the patchset: https://codereview.chromium.org/1295523006/#ps170001
(title: "Removing un-modified file")

[commit-bot: I haz the power, 2015-08-24 20:47:26]

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

[commit-bot: I haz the power, 2015-08-24 21:36:27]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-08-24 21:39:46]

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

[stgao, 2015-08-25 05:17:47]

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

[stgao, 2015-08-25 05:17:48]

FYI: this CL seems to cause the memory test in
https://build.chromium.org/p/chromium.memory.fyi/builders/Webkit%20Linux%20%2...

https://findit-for-me.appspot.com/build-failure?url=https://build.chromium.or...

[svaldez, 2015-08-25 15:02:06]

On 2015/08/25 05:17:48, Shuotao wrote:
> FYI: this CL seems to cause the memory test in
>
https://build.chromium.org/p/chromium.memory.fyi/builders/Webkit%20Linux%20%2...
> 
>
https://findit-for-me.appspot.com/build-failure?url=https://build.chromium.or...

It looks like this is a result of the type signature changing and the
suppression from crbug.com/380575 no longer applying:

https://code.google.com/p/chromium/codesearch#chromium/src/tools/valgrind/mem...