Some linked_ptr -> unique_ptr conversion in extensions/browser.

Some linked_ptr -> unique_ptr conversion in extensions/browser.

Change ExtensionService::AddProviderForTesting() to take
unique_ptr instead of raw pointer.

BUG=556939
Test=None, internal only change.

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

[lazyboy, 2016-08-30 03:19:00]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 03:46:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 04:13:21]

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

[commit-bot: I haz the power, 2016-08-30 04:13:22]

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

[lazyboy, 2016-08-30 18:07:14]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 18:07:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2016-08-30 18:09:52]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-08-30 19:09:32]

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

[commit-bot: I haz the power, 2016-08-30 19:09:33]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-08-30 19:21:54]

A bunch of nits, but overall, looks good.  Thanks for doing this!

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc
(right):

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:1484:
const std::string extension_id = listener->extension_id();
can this be const&?

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/mdns/mdns_api_unittest.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:49: .release()));
EventListener::ForExtension() returns a unique_ptr, right?  So we can remove
WrapUnique and release()

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

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:177: ExternalProviderInterface*
test_provider) {
Can we add a TODO to make this take a unique_ptr (or, it looks like it's only
called in three places, if you wanted to do it in this cl)?

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

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:533:
std::unique_ptr<ExternalProviderInterface>(new ExternalProviderImpl(
prefer base::MakeUnique

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:583:
std::unique_ptr<ExternalProviderInterface>(new ExternalProviderImpl(
ditto

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:620:
std::unique_ptr<ExternalProviderInterface>(new ExternalProviderImpl(
ditto (and for the rest of this file)

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
File extensions/browser/api/api_resource_manager.h (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
extensions/browser/api/api_resource_manager.h:189: int Add(T* api_resource) {
here, too, add a TODO to make this (and Add() on line 114) take unique ptrs. 
(Wouldn't recommend doing that change in this CL - it's a little bit more
far-reaching.)

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_linux.cc
(right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_linux.cc:946:
std::unique_ptr<base::DictionaryValue>& existing_access_point =
std::unique_ptr&s are kind of weird.  Sometimes we need them, but here, I think
it might be more clear to just use a raw ptr.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_linux.h
(right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_linux.h:31: typedef
std::map<base::string16, std::unique_ptr<base::DictionaryValue>>
update to using syntax?

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
File extensions/browser/api/system_storage/system_storage_apitest.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
extensions/browser/api/system_storage/system_storage_apitest.cc:120:
device_ids_listeners.push_back(std::move(listener));
optional: inline listener with a base::MakeUnique

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/cont...
File extensions/browser/content_verifier_io_data.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/cont...
extensions/browser/content_verifier_io_data.cc:40: if (found != data_map_.end())
?? Why not do data_map_.erase(extension_id);?  (Not your code, obviously, but we
should fix it! ;))

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/decl...
File extensions/browser/declarative_user_script_manager.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/decl...
extensions/browser/declarative_user_script_manager.cc:48:
declarative_user_script_masters_[host_id] = base::WrapUnique(master);
MakeUnique inlined?

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/gues...
File extensions/browser/guest_view/app_view/app_view_guest.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/gues...
extensions/browser/guest_view/app_view/app_view_guest.cc:82: const
std::unique_ptr<ResponseInfo>& response_info = it->second;
ditto re maybe using a raw ptr

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/lazy...
File extensions/browser/lazy_background_task_queue.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/lazy...
extensions/browser/lazy_background_task_queue.cc:81: pending_tasks_[key] =
base::WrapUnique(tasks_list);
MakeUnique + insert()?

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/valu...
File extensions/browser/value_store/value_store_unittest.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/valu...
extensions/browser/value_store/value_store_unittest.cc:95:
expected_as_map[it->key()] = std::move(change);
base::MakeUnique

[lazyboy, 2016-08-30 20:30:57]

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc
(right):

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:1484:
const std::string extension_id = listener->extension_id();
On 2016/08/30 19:21:53, Devlin wrote:
> can this be const&?

Done.

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/mdns/mdns_api_unittest.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/mdns/mdns_api_unittest.cc:49: .release()));
On 2016/08/30 19:21:53, Devlin wrote:
> EventListener::ForExtension() returns a unique_ptr, right?  So we can remove
> WrapUnique and release()

Done.

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

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:177: ExternalProviderInterface*
test_provider) {
On 2016/08/30 19:21:53, Devlin wrote:
> Can we add a TODO to make this take a unique_ptr (or, it looks like it's only
> called in three places, if you wanted to do it in this cl)?

Done.
Changed those places.

I also tried changing AddMockExternalProvider's signature to take unique_ptr in
extensoin_service_unittest.cc, gave up as I thought changing that to just wrap a
raw ptr with WrapUnique isn't doing much. The pattern currently looks like:

MockExtensionProvider* provider = new ..;
AddMockExternalProvider(provider);
..
keep accessing |provider| raw ptr.

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

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:533:
std::unique_ptr<ExternalProviderInterface>(new ExternalProviderImpl(
On 2016/08/30 19:21:53, Devlin wrote:
> prefer base::MakeUnique

Done.

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:583:
std::unique_ptr<ExternalProviderInterface>(new ExternalProviderImpl(
On 2016/08/30 19:21:53, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:620:
std::unique_ptr<ExternalProviderInterface>(new ExternalProviderImpl(
On 2016/08/30 19:21:53, Devlin wrote:
> ditto (and for the rest of this file)

Done.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
File extensions/browser/api/api_resource_manager.h (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
extensions/browser/api/api_resource_manager.h:189: int Add(T* api_resource) {
On 2016/08/30 19:21:53, Devlin wrote:
> here, too, add a TODO to make this (and Add() on line 114) take unique ptrs. 
> (Wouldn't recommend doing that change in this CL - it's a little bit more
> far-reaching.)

Done.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_linux.cc
(right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_linux.cc:946:
std::unique_ptr<base::DictionaryValue>& existing_access_point =
On 2016/08/30 19:21:53, Devlin wrote:
> std::unique_ptr&s are kind of weird.  Sometimes we need them, but here, I
think
> it might be more clear to just use a raw ptr.

Done.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_linux.h
(right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_linux.h:31: typedef
std::map<base::string16, std::unique_ptr<base::DictionaryValue>>
On 2016/08/30 19:21:53, Devlin wrote:
> update to using syntax?

Done.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
File extensions/browser/api/system_storage/system_storage_apitest.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/api/...
extensions/browser/api/system_storage/system_storage_apitest.cc:120:
device_ids_listeners.push_back(std::move(listener));
On 2016/08/30 19:21:53, Devlin wrote:
> optional: inline listener with a base::MakeUnique

Done.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/cont...
File extensions/browser/content_verifier_io_data.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/cont...
extensions/browser/content_verifier_io_data.cc:40: if (found != data_map_.end())
On 2016/08/30 19:21:53, Devlin wrote:
> ?? Why not do data_map_.erase(extension_id);?  (Not your code, obviously, but
we
> should fix it! ;))

Thanks for noticing, done.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/decl...
File extensions/browser/declarative_user_script_manager.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/decl...
extensions/browser/declarative_user_script_manager.cc:48:
declarative_user_script_masters_[host_id] = base::WrapUnique(master);
On 2016/08/30 19:21:54, Devlin wrote:
> MakeUnique inlined?

Need raw ptr on next line. Currently this is done without a second lookup.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/gues...
File extensions/browser/guest_view/app_view/app_view_guest.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/gues...
extensions/browser/guest_view/app_view/app_view_guest.cc:82: const
std::unique_ptr<ResponseInfo>& response_info = it->second;
On 2016/08/30 19:21:54, Devlin wrote:
> ditto re maybe using a raw ptr

Done.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/lazy...
File extensions/browser/lazy_background_task_queue.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/lazy...
extensions/browser/lazy_background_task_queue.cc:81: pending_tasks_[key] =
base::WrapUnique(tasks_list);
On 2016/08/30 19:21:54, Devlin wrote:
> MakeUnique + insert()?

|task_list| raw ptr is used on line 103 below... Not sure MakeUnique will help
much here.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/valu...
File extensions/browser/value_store/value_store_unittest.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/valu...
extensions/browser/value_store/value_store_unittest.cc:95:
expected_as_map[it->key()] = std::move(change);
On 2016/08/30 19:21:54, Devlin wrote:
> base::MakeUnique

Done.

[Avi (use Gerrit), 2016-08-30 20:33:00]

FYI, linked_ptr conversion is http://crbug.com/556939 .

[lazyboy, 2016-08-30 20:34:28]

Description was changed from

==========
Some linked_ptr -> unique_ptr conversion in extensions/browser.

BUG=None
Test=None, internal only change.
==========

to

==========
Some linked_ptr -> unique_ptr conversion in extensions/browser.

BUG=556939
Test=None, internal only change.
==========

[lazyboy, 2016-08-30 20:34:46]

On 2016/08/30 20:33:00, Avi wrote:
> FYI, linked_ptr conversion is http://crbug.com/556939 .

Added Bug#, thanks!

[Devlin, 2016-08-30 22:48:28]

lgtm.  Might be worth updating description to include the MakeUnique
conversions, too?

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

https://codereview.chromium.org/2294653002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:177: ExternalProviderInterface*
test_provider) {
On 2016/08/30 20:30:56, lazyboy wrote:
> On 2016/08/30 19:21:53, Devlin wrote:
> > Can we add a TODO to make this take a unique_ptr (or, it looks like it's
only
> > called in three places, if you wanted to do it in this cl)?
> 
> Done.
> Changed those places.
> 
> I also tried changing AddMockExternalProvider's signature to take unique_ptr
in
> extensoin_service_unittest.cc, gave up as I thought changing that to just wrap
a
> raw ptr with WrapUnique isn't doing much. The pattern currently looks like:
> 
> MockExtensionProvider* provider = new ..;
> AddMockExternalProvider(provider);
> ..
> keep accessing |provider| raw ptr.

Ack; no need to change it in this CL.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/decl...
File extensions/browser/declarative_user_script_manager.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/decl...
extensions/browser/declarative_user_script_manager.cc:48:
declarative_user_script_masters_[host_id] = base::WrapUnique(master);
On 2016/08/30 20:30:56, lazyboy wrote:
> On 2016/08/30 19:21:54, Devlin wrote:
> > MakeUnique inlined?
> 
> Need raw ptr on next line. Currently this is done without a second lookup.

// Insert a new DeclarativeUserScriptMaster and return a ptr to it.
return declarative_user_script_masters_.insert(std::make_pair(
    host_id, base::MakeUnique<DeclarativeUserScriptMaster>(browser_context_,
host_id))).second->second.get();

I'd prefer that, because it avoids any "bare news" and references after wrapping
something (and I think it's clear enough with the comment).  But I can
understand if you'd prefer this for the legibility.

[lazyboy, 2016-08-30 23:26:02]

Description was changed from

==========
Some linked_ptr -> unique_ptr conversion in extensions/browser.

BUG=556939
Test=None, internal only change.
==========

to

==========
Some linked_ptr -> unique_ptr conversion in extensions/browser.

Change ExtensionService::AddProviderForTesting() to take
unique_ptr instead of raw pointer.

BUG=556939
Test=None, internal only change.
==========

[lazyboy, 2016-08-30 23:27:48]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[lazyboy, 2016-08-30 23:28:29]

Updated CL description.

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/decl...
File extensions/browser/declarative_user_script_manager.cc (right):

https://codereview.chromium.org/2294653002/diff/40001/extensions/browser/decl...
extensions/browser/declarative_user_script_manager.cc:48:
declarative_user_script_masters_[host_id] = base::WrapUnique(master);
On 2016/08/30 22:48:28, Devlin wrote:
> On 2016/08/30 20:30:56, lazyboy wrote:
> > On 2016/08/30 19:21:54, Devlin wrote:
> > > MakeUnique inlined?
> > 
> > Need raw ptr on next line. Currently this is done without a second lookup.
> 
> // Insert a new DeclarativeUserScriptMaster and return a ptr to it.
> return declarative_user_script_masters_.insert(std::make_pair(
>     host_id, base::MakeUnique<DeclarativeUserScriptMaster>(browser_context_,
> host_id))).second->second.get();
> 
> I'd prefer that, because it avoids any "bare news" and references after
wrapping
> something (and I think it's clear enough with the comment).  But I can
> understand if you'd prefer this for the legibility.

Done.

[commit-bot: I haz the power, 2016-08-30 23:28:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 00:20:00]

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

[commit-bot: I haz the power, 2016-08-31 00:20:01]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-08-31 00:31:25]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-08-31 00:31:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2294653002/#ps90001
(title: "address comments")

[commit-bot: I haz the power, 2016-08-31 00:31:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 00:37:45]

Description was changed from

==========
Some linked_ptr -> unique_ptr conversion in extensions/browser.

Change ExtensionService::AddProviderForTesting() to take
unique_ptr instead of raw pointer.

BUG=556939
Test=None, internal only change.
==========

to

==========
Some linked_ptr -> unique_ptr conversion in extensions/browser.

Change ExtensionService::AddProviderForTesting() to take
unique_ptr instead of raw pointer.

BUG=556939
Test=None, internal only change.
==========

[commit-bot: I haz the power, 2016-08-31 00:37:46]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-08-31 00:40:46]

Description was changed from

==========
Some linked_ptr -> unique_ptr conversion in extensions/browser.

Change ExtensionService::AddProviderForTesting() to take
unique_ptr instead of raw pointer.

BUG=556939
Test=None, internal only change.
==========

to

==========
Some linked_ptr -> unique_ptr conversion in extensions/browser.

Change ExtensionService::AddProviderForTesting() to take
unique_ptr instead of raw pointer.

BUG=556939
Test=None, internal only change.

Committed: https://crrev.com/f33109d5e1ca94a1e12ed34eb2f55cbb1d767cc4
Cr-Commit-Position: refs/heads/master@{#415518}
==========

[commit-bot: I haz the power, 2016-08-31 00:40:47]

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