Remove ScopedVector from chrome/browser/.

Remove ScopedVector from chrome/browser/.

base::ScopedVector is deprecated, see bug.

BUG=554289
Review-Url: https://codereview.chromium.org/2767893002
Cr-Commit-Position: refs/heads/master@{#460000}
Committed: https://chromium.googlesource.com/chromium/src/+/4ea301f06b3c94bc2cf9a8be6ca151e3c01987ba

[leonhsl(Using Gerrit), 2017-03-22 09:19:44]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 09:20:02]

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, 2017-03-22 09:33:34]

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

[commit-bot: I haz the power, 2017-03-22 09:33:35]

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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2017-03-22 23:45:13]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 23:45: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, 2017-03-23 01:05:45]

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

[commit-bot: I haz the power, 2017-03-23 01:05:46]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-03-23 02:02:11]

leon.han@intel.com changed reviewers:
+ avi@chromium.org, thestig@chromium.org

[leonhsl(Using Gerrit), 2017-03-23 02:02:12]

PTAL, Thanks!

avi@: Overall and content/ OWNER review

thestig@: Overall and chrome/browser/ OWNER review

[Avi (use Gerrit), 2017-03-23 02:44:48]

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:595:
clients_[index].release();
Is this right? I know that this preserves the code's behavior, but making it
explicit like this exposes the wat.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:640:
invalidation_forwarders_[index].release();
ditto

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:699:
sync_refreshers_[i].release();
ditto

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.h (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.h:159: // object and manages its
lifetime.
This note about lifetime seems wrong now that a vector is returned rather than a
reference. Probably drop this sentence.

[leonhsl(Using Gerrit), 2017-03-23 02:47:55]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2017-03-23 02:59:26]

Mostly nits.

In the future, maybe split large CLs into smaller pieces. e.g. c/b/{sync,ui}
could have been their own CLs.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_device.h (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.h:163: typedef
std::vector<std::unique_ptr<AdbMessage>> PendingMessages;
Convert to using PendingMessages = std::vector<std::unique_ptr<AdbMessage>>;
while we are here?

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/policy/p...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/policy/p...
chrome/browser/policy/policy_prefs_browsertest.cc:138:
indicator_test_cases_.push_back(std::move(test_case));
#include <utility>

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/policy/p...
chrome/browser/policy/policy_prefs_browsertest.cc:562: for (auto pref_mapping =
pref_mappings.begin();
Can this be a range-based for-loop instead? Ditto below 2X.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/status_i...
File chrome/browser/status_icons/status_tray.h (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/status_tray.h:49: typedef
std::vector<std::unique_ptr<StatusIcon>> StatusIcons;
using

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/status_i...
File chrome/browser/status_icons/status_tray_unittest.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/status_tray_unittest.cc:5: #include
"chrome/browser/status_icons/status_tray.h"
Blank line after.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:302:
std::vector<ProfileSyncServiceHarness*> clients) {
Should this remain a const-ref?

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/status_icons/status_tray_mac.mm (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/status_icons/status_tray_mac.mm:24: return
std::move(icon);
<utility>

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.cc:57: if
(status_icon)
braces

[leonhsl(Using Gerrit), 2017-03-23 14:26:47]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 14:27:09]

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

[leonhsl(Using Gerrit), 2017-03-23 15:03:17]

Thank you very much for kindly review! Uploaded ps#2 to address comments, PTAnL.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_device.h (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.h:163: typedef
std::vector<std::unique_ptr<AdbMessage>> PendingMessages;
On 2017/03/23 02:59:26, Lei Zhang wrote:
> Convert to using PendingMessages = std::vector<std::unique_ptr<AdbMessage>>;
> while we are here?

Done.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/policy/p...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/policy/p...
chrome/browser/policy/policy_prefs_browsertest.cc:138:
indicator_test_cases_.push_back(std::move(test_case));
On 2017/03/23 02:59:26, Lei Zhang wrote:
> #include <utility>

Done.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/policy/p...
chrome/browser/policy/policy_prefs_browsertest.cc:562: for (auto pref_mapping =
pref_mappings.begin();
On 2017/03/23 02:59:26, Lei Zhang wrote:
> Can this be a range-based for-loop instead? Ditto below 2X.

Done. And also for below 2X.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/status_i...
File chrome/browser/status_icons/status_tray.h (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/status_tray.h:49: typedef
std::vector<std::unique_ptr<StatusIcon>> StatusIcons;
On 2017/03/23 02:59:26, Lei Zhang wrote:
> using

Done.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/status_i...
File chrome/browser/status_icons/status_tray_unittest.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/status_tray_unittest.cc:5: #include
"chrome/browser/status_icons/status_tray.h"
On 2017/03/23 02:59:26, Lei Zhang wrote:
> Blank line after.

Done.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:302:
std::vector<ProfileSyncServiceHarness*> clients) {
On 2017/03/23 02:59:26, Lei Zhang wrote:
> Should this remain a const-ref?

This function is called by SyncTest::AwaitQuiescence(), which was calling
ScopedVector::get() to obtain the const-ref of vector and then passing it as
parameter to this function. 

But now for SyncTest::AwaitQuiescence(), std::vector<std::unique_ptr<>> has no
such convenient function get() to obtain the const-ref of vector, so I did this
change.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:595:
clients_[index].release();
On 2017/03/23 02:44:48, Avi wrote:
> Is this right? I know that this preserves the code's behavior, but making it
> explicit like this exposes the wat.

Yeah I also felt a little weak here.. After checking around I think we can use a
DCHECK(!clients_[index]) here, original code is not intentionally releasing
ownership of a non-null pointer. WDYT? Thanks.

Also changed to use DCHECK for below 2X.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.h (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.h:159: // object and manages its
lifetime.
On 2017/03/23 02:44:48, Avi wrote:
> This note about lifetime seems wrong now that a vector is returned rather than
a
> reference. Probably drop this sentence.

Done.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/status_icons/status_tray_mac.mm (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/status_icons/status_tray_mac.mm:24: return
std::move(icon);
On 2017/03/23 02:59:26, Lei Zhang wrote:
> <utility>

Done.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_icon_linux_wrapper.cc:57: if
(status_icon)
On 2017/03/23 02:59:26, Lei Zhang wrote:
> braces

Done.

[leonhsl(Using Gerrit), 2017-03-23 15:04:29]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 15:04:35]

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, 2017-03-23 16:15:23]

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

[commit-bot: I haz the power, 2017-03-23 16:15:24]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-03-23 18:32:55]

avi@chromium.org changed reviewers:
+ zea@chromium.org

[Avi (use Gerrit), 2017-03-23 18:32:56]

Nicolas,

Can you take a look at
https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
? It looks like sync code is intentionally leaking. Should we be fixing this?

[skym, 2017-03-23 19:28:03]

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

[skym, 2017-03-23 19:28:05]

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:595:
clients_[index].release();
On 2017/03/23 15:03:17, leonhsl wrote:
> On 2017/03/23 02:44:48, Avi wrote:
> > Is this right? I know that this preserves the code's behavior, but making it
> > explicit like this exposes the wat.
> 
> Yeah I also felt a little weak here.. After checking around I think we can use
a
> DCHECK(!clients_[index]) here, original code is not intentionally releasing
> ownership of a non-null pointer. WDYT? Thanks.
> 
> Also changed to use DCHECK for below 2X.

I agree that the intention is that each index is written to exactly once.

I don't quite follow why the old code was leaking, was this a quirk of
ScopedVector? I definitely don't think we want to leak. If there was for some
reason already an entry at |index|, I think it should be destructed when
overriden. In this new std::vector<std::unique_ptr<X>> world, will assigning to
|index| correctly destruct the existing object? I don't really think adding a
DCHECK above this really adds anything.

[Avi (use Gerrit), 2017-03-23 19:36:33]

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:595:
clients_[index].release();
On 2017/03/23 19:28:05, skym wrote:
> I don't quite follow why the old code was leaking, was this a quirk of
> ScopedVector?

Yes. If you assigned through operator[] of ScopedVector it would not delete the
old value. If this is not intentional, and the old code was *accidentally*
leaking, we can fix it.

We just wanted to check to see if for some reason it was intentional.

> In this new std::vector<std::unique_ptr<X>> world, will assigning to
> |index| correctly destruct the existing object?

Yes. If that's the desired behavior (and it seems like it was the behavior you
were expecting and not getting) we can switch to it.

[skym, 2017-03-23 20:05:50]

On 2017/03/23 19:36:33, Avi wrote:
>
https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
> File chrome/browser/sync/test/integration/sync_test.cc (right):
> 
>
https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/sync_test.cc:595:
> clients_[index].release();
> On 2017/03/23 19:28:05, skym wrote:
> > I don't quite follow why the old code was leaking, was this a quirk of
> > ScopedVector?
> 
> Yes. If you assigned through operator[] of ScopedVector it would not delete
the
> old value. If this is not intentional, and the old code was *accidentally*
> leaking, we can fix it.
> 
> We just wanted to check to see if for some reason it was intentional.
> 
> > In this new std::vector<std::unique_ptr<X>> world, will assigning to
> > |index| correctly destruct the existing object?
> 
> Yes. If that's the desired behavior (and it seems like it was the behavior you
> were expecting and not getting) we can switch to it.

Well, I'm glad we're moving away from ScopedVector, what a trap. Removing this
leak sounds great for me, thanks for finding this and the explanation!

[Nicolas Zea, 2017-03-23 20:06:47]

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:595:
clients_[index].release();
On 2017/03/23 19:36:33, Avi wrote:
> On 2017/03/23 19:28:05, skym wrote:
> > I don't quite follow why the old code was leaking, was this a quirk of
> > ScopedVector?
> 
> Yes. If you assigned through operator[] of ScopedVector it would not delete
the
> old value. If this is not intentional, and the old code was *accidentally*
> leaking, we can fix it.
> 
> We just wanted to check to see if for some reason it was intentional.
> 
> > In this new std::vector<std::unique_ptr<X>> world, will assigning to
> > |index| correctly destruct the existing object?
> 
> Yes. If that's the desired behavior (and it seems like it was the behavior you
> were expecting and not getting) we can switch to it.
> 

Agreed with Sky. I'm fairly sure this wasn't intentionally leaking, and was just
an oversight.

[Avi (use Gerrit), 2017-03-23 20:12:16]

On 2017/03/23 20:06:47, Nicolas Zea wrote:
>
https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
> File chrome/browser/sync/test/integration/sync_test.cc (right):
> 
>
https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/sync_test.cc:595:
> clients_[index].release();
> On 2017/03/23 19:36:33, Avi wrote:
> > On 2017/03/23 19:28:05, skym wrote:
> > > I don't quite follow why the old code was leaking, was this a quirk of
> > > ScopedVector?
> > 
> > Yes. If you assigned through operator[] of ScopedVector it would not delete
> the
> > old value. If this is not intentional, and the old code was *accidentally*
> > leaking, we can fix it.
> > 
> > We just wanted to check to see if for some reason it was intentional.
> > 
> > > In this new std::vector<std::unique_ptr<X>> world, will assigning to
> > > |index| correctly destruct the existing object?
> > 
> > Yes. If that's the desired behavior (and it seems like it was the behavior
you
> > were expecting and not getting) we can switch to it.
> > 
> 
> Agreed with Sky. I'm fairly sure this wasn't intentionally leaking, and was
just
> an oversight.

Woot! Thank you, leonhsl!

[Lei Zhang, 2017-03-23 20:26:27]

lgtm, but I didn't check c/b/sync very carefully since there are others looking
at it.

[mark a. foltz, 2017-03-23 21:34:53]

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

[mark a. foltz, 2017-03-23 21:34:54]

Non-reviewer LGTM for */media/*

[Nicolas Zea, 2017-03-23 23:10:07]

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:302:
std::vector<ProfileSyncServiceHarness*> clients) {
On 2017/03/23 15:03:17, leonhsl wrote:
> On 2017/03/23 02:59:26, Lei Zhang wrote:
> > Should this remain a const-ref?
> 
> This function is called by SyncTest::AwaitQuiescence(), which was calling
> ScopedVector::get() to obtain the const-ref of vector and then passing it as
> parameter to this function. 
> 
> But now for SyncTest::AwaitQuiescence(), std::vector<std::unique_ptr<>> has no
> such convenient function get() to obtain the const-ref of vector, so I did
this
> change.

Isn't GetSyncClients good enough to access a vector from AwaitQuiescence (which
you're already using)? It doesn't seem like this needs std::move logic around it
anywhere, unless I'm missing something?

[leonhsl(Using Gerrit), 2017-03-24 02:23:12]

Thanks all for the discussions and review!

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:302:
std::vector<ProfileSyncServiceHarness*> clients) {
On 2017/03/23 23:10:07, Nicolas Zea wrote:
> On 2017/03/23 15:03:17, leonhsl wrote:
> > On 2017/03/23 02:59:26, Lei Zhang wrote:
> > > Should this remain a const-ref?
> > 
> > This function is called by SyncTest::AwaitQuiescence(), which was calling
> > ScopedVector::get() to obtain the const-ref of vector and then passing it as
> > parameter to this function. 
> > 
> > But now for SyncTest::AwaitQuiescence(), std::vector<std::unique_ptr<>> has
no
> > such convenient function get() to obtain the const-ref of vector, so I did
> this
> > change.
> 
> Isn't GetSyncClients good enough to access a vector from AwaitQuiescence
(which
> you're already using)? It doesn't seem like this needs std::move logic around
it
> anywhere, unless I'm missing something?

Sorry if I'm misunderstanding the question, do you mean that we'd better still
make GetSyncClients() to return a const-ref of vector? 
If so, we'd have to keep a vector member in SyncTest class, in corresponding
with that std::vector<std::unique_ptr<X>> member. Personally I'd prefer to
return a vector rather than returning a const-ref of a member vector..

[Avi (use Gerrit), 2017-03-24 05:50:01]

BTW with the sync leak fix, LGTM.

[Nicolas Zea, 2017-03-24 16:48:03]

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:302:
std::vector<ProfileSyncServiceHarness*> clients) {
On 2017/03/24 02:23:12, leonhsl wrote:
> On 2017/03/23 23:10:07, Nicolas Zea wrote:
> > On 2017/03/23 15:03:17, leonhsl wrote:
> > > On 2017/03/23 02:59:26, Lei Zhang wrote:
> > > > Should this remain a const-ref?
> > > 
> > > This function is called by SyncTest::AwaitQuiescence(), which was calling
> > > ScopedVector::get() to obtain the const-ref of vector and then passing it
as
> > > parameter to this function. 
> > > 
> > > But now for SyncTest::AwaitQuiescence(), std::vector<std::unique_ptr<>>
has
> no
> > > such convenient function get() to obtain the const-ref of vector, so I did
> > this
> > > change.
> > 
> > Isn't GetSyncClients good enough to access a vector from AwaitQuiescence
> (which
> > you're already using)? It doesn't seem like this needs std::move logic
around
> it
> > anywhere, unless I'm missing something?
> 
> Sorry if I'm misunderstanding the question, do you mean that we'd better still
> make GetSyncClients() to return a const-ref of vector? 
> If so, we'd have to keep a vector member in SyncTest class, in corresponding
> with that std::vector<std::unique_ptr<X>> member. Personally I'd prefer to
> return a vector rather than returning a const-ref of a member vector..

Why do you need to keep a vector member in SyncTest? GetSyncClients returns a
std::vector<ProfileSyncServiceHarness*> already. You can simply pass the return
value of GetSyncClients by const ref all the way down to AwaitQuiescence right?
Nothing is keeping a reference to that vector within the callstack from what I
can tell.

[leonhsl(Using Gerrit), 2017-03-25 13:40:13]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-25 13:40:22]

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

[leonhsl(Using Gerrit), 2017-03-25 14:41:13]

Hi, Nicolas, uploaded ps#3, PTAnL, Thanks.

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/2767893002/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:302:
std::vector<ProfileSyncServiceHarness*> clients) {
On 2017/03/24 16:48:03, Nicolas Zea wrote:
> On 2017/03/24 02:23:12, leonhsl wrote:
> > On 2017/03/23 23:10:07, Nicolas Zea wrote:
> > > On 2017/03/23 15:03:17, leonhsl wrote:
> > > > On 2017/03/23 02:59:26, Lei Zhang wrote:
> > > > > Should this remain a const-ref?
> > > > 
> > > > This function is called by SyncTest::AwaitQuiescence(), which was
calling
> > > > ScopedVector::get() to obtain the const-ref of vector and then passing
it
> as
> > > > parameter to this function. 
> > > > 
> > > > But now for SyncTest::AwaitQuiescence(), std::vector<std::unique_ptr<>>
> has
> > no
> > > > such convenient function get() to obtain the const-ref of vector, so I
did
> > > this
> > > > change.
> > > 
> > > Isn't GetSyncClients good enough to access a vector from AwaitQuiescence
> > (which
> > > you're already using)? It doesn't seem like this needs std::move logic
> around
> > it
> > > anywhere, unless I'm missing something?
> > 
> > Sorry if I'm misunderstanding the question, do you mean that we'd better
still
> > make GetSyncClients() to return a const-ref of vector? 
> > If so, we'd have to keep a vector member in SyncTest class, in corresponding
> > with that std::vector<std::unique_ptr<X>> member. Personally I'd prefer to
> > return a vector rather than returning a const-ref of a member vector..
> 
> Why do you need to keep a vector member in SyncTest? GetSyncClients returns a
> std::vector<ProfileSyncServiceHarness*> already. You can simply pass the
return
> value of GetSyncClients by const ref all the way down to AwaitQuiescence
right?
> Nothing is keeping a reference to that vector within the callstack from what I
> can tell.

Ah,, I see, Thank you! Done.

[commit-bot: I haz the power, 2017-03-25 14:45:34]

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

[commit-bot: I haz the power, 2017-03-25 14:45:35]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2017-03-27 17:28:49]

sync LGTM thanks!

[leonhsl(Using Gerrit), 2017-03-28 02:04:22]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2017-03-28 02:04:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, mfoltz@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2767893002/#ps60001
(title: "Address comments from zea@")

[commit-bot: I haz the power, 2017-03-28 02:04:44]

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, 2017-03-28 03:36:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490666662853350,
"parent_rev": "ff3b9813827f6a237045257e87ad4f9711caf9e0", "commit_rev":
"4ea301f06b3c94bc2cf9a8be6ca151e3c01987ba"}

[commit-bot: I haz the power, 2017-03-28 03:37:52]

Description was changed from

==========
Remove ScopedVector from chrome/browser/.

base::ScopedVector is deprecated, see bug.

BUG=554289
==========

to

==========
Remove ScopedVector from chrome/browser/.

base::ScopedVector is deprecated, see bug.

BUG=554289

Review-Url: https://codereview.chromium.org/2767893002
Cr-Commit-Position: refs/heads/master@{#460000}
Committed:
https://chromium.googlesource.com/chromium/src/+/4ea301f06b3c94bc2cf9a8be6ca1...
==========

[commit-bot: I haz the power, 2017-03-28 03:37:54]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4ea301f06b3c94bc2cf9a8be6ca1...