Remove most ScopedVector usage from c/b/extensions.

Remove most ScopedVector usage from c/b/extensions.

Replace them with vector<unique_ptr<T>>.

After this CL there will be only 3 places where ScopedVector
would still be around. Since those (language_settings_private_delegate.cc,
web_navigation_apitest.cc and chrome_content_browser_client_extensions_part.cc)
are non-trivial, I will try to remove ScopedVector from them in subsequent CLs.

BUG=556939
Test=None, internal only change.

Committed: https://crrev.com/4aeef2005e69892c90c3adec9607f4ef378fd533
Cr-Commit-Position: refs/heads/master@{#417042}

[lazyboy, 2016-09-02 23:14:17]

lazyboy@chromium.org changed reviewers:
+ asargent@chromium.org

[lazyboy, 2016-09-02 23:14:18]

Antony, can you take a look? I'll add other OWNERS once you're happy with the
CL. Thanks.

[lazyboy, 2016-09-02 23:59:44]

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

[commit-bot: I haz the power, 2016-09-03 00:00:13]

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-09-03 00:32:49]

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

[commit-bot: I haz the power, 2016-09-03 00:32:50]

Dry run: Exceeded global retry quota

[asargent_no_longer_on_chrome, 2016-09-06 22:12:40]

CL description nit: the 3 paths you mention in items 1-3 of the description do
not appear at all in this CL, so I was initially a bit confused. Did you reuse
the title from a different CL? Or did you mean to say that those are the 3
places where ScopedVector is used, and to eliminate those you're having to touch
a bunch of files and breaking the changes up into batches of files?

Otherwise things mostly look fine with a couple of suggestions and one question.

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/gcd_private/privet_v3_session.cc (right):

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/gcd_private/privet_v3_session.cc:508: }));
optional: The following equivalent code seems a little simpler to me:

for (auto iter : fetchers_) {
  if (iter->get() == fetcher) {
    fetchers_.erase(iter);
    break;
  }    
}

Feel free to leave as you have it above if you prefer your version though.

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/signed_in_devices/id_mapping_helper.cc
(right):

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/signed_in_devices/id_mapping_helper.cc:95: it !=
devices.end(); ++it) {
consider shortening this to:

for (auto it : devices) {

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right):

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:77: tabs_.erase(it);
optional: a simpler way to write this method may be:

for (auto it : tabs_) {
  if (it->get() == tab) {
    tabs_.erase(it);
    break;
  }
}

https://codereview.chromium.org/2310683002/diff/20001/extensions/browser/mana...
File extensions/browser/management_policy.cc (right):

https://codereview.chromium.org/2310683002/diff/20001/extensions/browser/mana...
extensions/browser/management_policy.cc:67: providers_.insert(provider.get());
Hmm, this is a little unusual - when I see unique_ptr's as arguments, I usually
expect ownership to be transferred (that's typically what happens with
single-argument unique_ptr's not wrapped in a vector). 

Doesn't it seem a little unsafe that we just hold on to raw ptr's here? Could
ownership be transferred? Or could we use weak_ptr or RefCounted? At a minimum
we should probably have some comment about the lifetime expectations.

[lazyboy, 2016-09-06 23:47:51]

Description was changed from

==========
Remove most ScopedVector usage from c/b/extensions.

Replace them with vector<unique_ptr<T>>.

There are 3 places where ScopedVector is still used:
1) api/language_settings_private/language_settings_private_delegate.cc
2) api/web_navigation/web_navigation_apitest.cc
3) chrome_content_browser_client_extensions_part.cc
These spread out too much, so probably a better idea to separately
fix them.

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

to

==========
Remove most ScopedVector usage from c/b/extensions.

Replace them with vector<unique_ptr<T>>.

After this CL there will be only 3 places where ScopedVector
would still be around. Since those (language_settings_private_delegate.cc,
web_navigation_apitest.cc and chrome_content_browser_client_extensions_part.cc)
are non-trivial, I will try to remove ScopedVector from them in subsequent CLs.


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

[lazyboy, 2016-09-06 23:49:38]

re: 3 places mentioned in CL: those are places this CL is not touching, I've
made the description a bit compact to avoid confusion. Hope that helps, thanks!

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/gcd_private/privet_v3_session.cc (right):

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/gcd_private/privet_v3_session.cc:508: }));
On 2016/09/06 22:12:40, Antony Sargent wrote:
> optional: The following equivalent code seems a little simpler to me:
> 
> for (auto iter : fetchers_) {
>   if (iter->get() == fetcher) {
>     fetchers_.erase(iter);
>     break;
>   }    
> }
> 
> Feel free to leave as you have it above if you prefer your version though. 

Done.

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/signed_in_devices/id_mapping_helper.cc
(right):

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/signed_in_devices/id_mapping_helper.cc:95: it !=
devices.end(); ++it) {
On 2016/09/06 22:12:40, Antony Sargent wrote:
> consider shortening this to:
> 
> for (auto it : devices) {
> 
> 

Done.

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right):

https://codereview.chromium.org/2310683002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:77: tabs_.erase(it);
On 2016/09/06 22:12:40, Antony Sargent wrote:
> optional: a simpler way to write this method may be:
> 
> for (auto it : tabs_) {
>   if (it->get() == tab) {
>     tabs_.erase(it);
>     break;
>   }
> }

Done.

https://codereview.chromium.org/2310683002/diff/20001/extensions/browser/mana...
File extensions/browser/management_policy.cc (right):

https://codereview.chromium.org/2310683002/diff/20001/extensions/browser/mana...
extensions/browser/management_policy.cc:67: providers_.insert(provider.get());
On 2016/09/06 22:12:40, Antony Sargent wrote:
> Hmm, this is a little unusual - when I see unique_ptr's as arguments, I
usually
> expect ownership to be transferred (that's typically what happens with
> single-argument unique_ptr's not wrapped in a vector). 

Ya.
I think this is what ScopedVector was kind of hiding:
ScopedVector<T>::get() returns raw pointers: vector<T*>, that what
RegisterProviders is called with.
> 
> Doesn't it seem a little unsafe that we just hold on to raw ptr's here? 
Think so. I don't see an easy way to transfer ownership as multiple classes are
adding to |providers_| in different ways. SupervisedUserService would be one
hard one to fix.
I've added TODO in the header file beside |providers_|.
Could
> ownership be transferred? Or could we use weak_ptr or RefCounted? At a minimum
> we should probably have some comment about the lifetime expectations. 
>

[asargent_no_longer_on_chrome, 2016-09-06 23:55:22]

lgtm

[lazyboy, 2016-09-06 23:57:14]

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

[commit-bot: I haz the power, 2016-09-06 23:57:52]

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-09-07 00:38:40]

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

[commit-bot: I haz the power, 2016-09-07 00:38:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lazyboy, 2016-09-07 04:09:53]

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

[commit-bot: I haz the power, 2016-09-07 04:10:02]

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

[lazyboy, 2016-09-07 04:14:35]

lazyboy@chromium.org changed reviewers:
+ stanisc@chromium.org, xiyuan@chromium.org

[lazyboy, 2016-09-07 04:14:35]

+stanisc@chromium.org for components/sync/*
+xiyuan@chromium.org for customization_document_unittest.cc

[commit-bot: I haz the power, 2016-09-07 05:10:03]

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

[commit-bot: I haz the power, 2016-09-07 05:10:04]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-09-07 16:11:24]

customization_document_unittest.cc lgtm

[pavely, 2016-09-07 17:03:58]

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

[pavely, 2016-09-07 17:03:59]

components/sync/* lgtm % couple of comments.

https://codereview.chromium.org/2310683002/diff/60001/components/sync/device_...
File components/sync/device_info/device_info_sync_service.h (right):

https://codereview.chromium.org/2310683002/diff/60001/components/sync/device_...
components/sync/device_info/device_info_sync_service.h:15: #include
"base/memory/scoped_vector.h"
Could you remove scoped_vector.h?

https://codereview.chromium.org/2310683002/diff/60001/components/sync/device_...
File components/sync/device_info/device_info_tracker.h (right):

https://codereview.chromium.org/2310683002/diff/60001/components/sync/device_...
components/sync/device_info/device_info_tracker.h:11: #include
"base/memory/scoped_vector.h"
Could you remove scoped_vector.h?

[lazyboy, 2016-09-07 19:45:03]

https://codereview.chromium.org/2310683002/diff/60001/components/sync/device_...
File components/sync/device_info/device_info_sync_service.h (right):

https://codereview.chromium.org/2310683002/diff/60001/components/sync/device_...
components/sync/device_info/device_info_sync_service.h:15: #include
"base/memory/scoped_vector.h"
On 2016/09/07 17:03:59, pavely wrote:
> Could you remove scoped_vector.h?

Done.

https://codereview.chromium.org/2310683002/diff/60001/components/sync/device_...
File components/sync/device_info/device_info_tracker.h (right):

https://codereview.chromium.org/2310683002/diff/60001/components/sync/device_...
components/sync/device_info/device_info_tracker.h:11: #include
"base/memory/scoped_vector.h"
On 2016/09/07 17:03:59, pavely wrote:
> Could you remove scoped_vector.h?

Done.

[lazyboy, 2016-09-07 19:50:13]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-09-07 19:50:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, pavely@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2310683002/#ps80001
(title: "remove scoped_vector includes")

[commit-bot: I haz the power, 2016-09-07 19:50:41]

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-09-07 21:29:58]

Description was changed from

==========
Remove most ScopedVector usage from c/b/extensions.

Replace them with vector<unique_ptr<T>>.

After this CL there will be only 3 places where ScopedVector
would still be around. Since those (language_settings_private_delegate.cc,
web_navigation_apitest.cc and chrome_content_browser_client_extensions_part.cc)
are non-trivial, I will try to remove ScopedVector from them in subsequent CLs.


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

to

==========
Remove most ScopedVector usage from c/b/extensions.

Replace them with vector<unique_ptr<T>>.

After this CL there will be only 3 places where ScopedVector
would still be around. Since those (language_settings_private_delegate.cc,
web_navigation_apitest.cc and chrome_content_browser_client_extensions_part.cc)
are non-trivial, I will try to remove ScopedVector from them in subsequent CLs.


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

[commit-bot: I haz the power, 2016-09-07 21:30:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-07 21:31:46]

Description was changed from

==========
Remove most ScopedVector usage from c/b/extensions.

Replace them with vector<unique_ptr<T>>.

After this CL there will be only 3 places where ScopedVector
would still be around. Since those (language_settings_private_delegate.cc,
web_navigation_apitest.cc and chrome_content_browser_client_extensions_part.cc)
are non-trivial, I will try to remove ScopedVector from them in subsequent CLs.


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

to

==========
Remove most ScopedVector usage from c/b/extensions.

Replace them with vector<unique_ptr<T>>.

After this CL there will be only 3 places where ScopedVector
would still be around. Since those (language_settings_private_delegate.cc,
web_navigation_apitest.cc and chrome_content_browser_client_extensions_part.cc)
are non-trivial, I will try to remove ScopedVector from them in subsequent CLs.

BUG=556939
Test=None, internal only change.

Committed: https://crrev.com/4aeef2005e69892c90c3adec9607f4ef378fd533
Cr-Commit-Position: refs/heads/master@{#417042}
==========

[commit-bot: I haz the power, 2016-09-07 21:31:48]

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