PasswordStore: Use ScopedVector to express ownership of forms

PasswordStore: Use ScopedVector to express ownership of forms

This is a clean-up, getting rid of many manual deletes and comments about who owns forms passed around in vectors.

BUG=451018
Committed: https://crrev.com/155761f5775bf744f1600ce7a518767d24853117
Cr-Commit-Position: refs/heads/master@{#313539}

[vabr (Chromium), 2015-01-22 11:13:42]

Patchset #1 (id:1) has been deleted

[vabr (Chromium), 2015-01-22 15:18:18]

Patchset #1 (id:20001) has been deleted

[vabr (Chromium), 2015-01-26 09:34:23]

Patchset #1 (id:40001) has been deleted

[vabr (Chromium), 2015-01-26 10:49:08]

Patchset #1 (id:60001) has been deleted

[vabr (Chromium), 2015-01-26 15:58:45]

Patchset #1 (id:80001) has been deleted

[vabr (Chromium), 2015-01-26 15:59:01]

Patchset #1 (id:100001) has been deleted

[vabr (Chromium), 2015-01-26 15:59:17]

Patchset #1 (id:110001) has been deleted

[vabr (Chromium), 2015-01-26 15:59:31]

Patchset #1 (id:130001) has been deleted

[vabr (Chromium), 2015-01-26 15:59:46]

Patchset #1 (id:150001) has been deleted

[vabr (Chromium), 2015-01-27 11:45:15]

Patchset #1 (id:170001) has been deleted

[vabr (Chromium), 2015-01-27 11:45:23]

Patchset #1 (id:190001) has been deleted

[vabr (Chromium), 2015-01-27 11:45:31]

Patchset #1 (id:210001) has been deleted

[vabr (Chromium), 2015-01-27 11:45:40]

Patchset #1 (id:230001) has been deleted

[vabr (Chromium), 2015-01-27 14:23:24]

Patchset #1 (id:250001) has been deleted

[vabr (Chromium), 2015-01-27 14:23:32]

Patchset #1 (id:270001) has been deleted

[vabr (Chromium), 2015-01-27 14:25:22]

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

[vabr (Chromium), 2015-01-27 14:28:14]

Hi Vasilii,

Please have a look.

One change which is still pending is removing std::vector from GetLoginsRequest.
I'm looking into that class, and maybe will create a separate CL for that,
because it requires more work than just the switch to ScopedVector. If I do
that, I will change the TODO(VABR) to TODO(vabr) here, otherwise I will add the
changes to the request in a further patch set. The rest of this CL should not be
affected.

Thanks!

Vaclav

[vabr (Chromium), 2015-01-27 16:02:56]

I'm fixing the ASAN failures now, will update this CL shortly.
The GetLoginsRequest changes will become a separate CL, this is too big already.
Cheers,
Vaclav

[vabr (Chromium), 2015-01-27 16:07:43]

Hi Vasilii,

The CL is not ready for review again. This is how I propose to land it, please
have a look and let me know your comments.

Cheers,
Vaclav

[vasilii, 2015-01-27 20:45:31]

On 2015/01/27 16:07:43, vabr (Chromium) wrote:
> Hi Vasilii,
> 
> The CL is not ready for review again. This is how I propose to land it, please
> have a look and let me know your comments.
> 
> Cheers,
> Vaclav

Again NOT ready?? :-)

[vasilii, 2015-01-27 20:45:52]

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x.cc:713: for (const
auto& form : forms) {
autofill::PasswordForm* form

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:430: form = nullptr;
you could weak_clear |forms| instead.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:462: db_form = nullptr;
again I'd prefer weak_clear.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:471: using std::swap;
excessive here.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:473: swap(keychain_form,
unused_keychain_forms.back());
It's easier to understand

unused_keychain_forms.push_back(keychain_form);
keychain_form = nullptr;

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:526: db_form = nullptr;
I think weak_clear is better here.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:536: for (auto& item :
keychain_items) {
Here I'd prefer "SecKeychainItemRef item" but not a reference to the pointer.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:588:
ScopedVector<autofill::PasswordForm>* matches) {
returning this object would simplify the caller.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:742:
ScopedVector<autofill::PasswordForm>* forms) {
returning this would simplify the code.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:743: for (const auto& item
: *items) {
You are iterating over pointers. I'd prefer "SecKeychainItemRef item"

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1025: for (const auto&
db_form : database_forms) {
const reference to pointer seems to be an overhead.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1068:
obtained_forms.weak_clear();  // TODO(vabr): Change request to have a
Seems to be a leak.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1077:
obtained_forms.weak_clear();  // TODO(vabr): Change request to have a
Same here.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1122: for (const auto&
db_form : database_forms) {
a reference to the pointer

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_win.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_win.cc:100: void
PasswordStoreWin::DBHandler::GetIE7Results(
I think it's cleaner to return ScopedVector<autofill::PasswordForm>.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_win.cc:195:
ScopedVector<autofill::PasswordForm>* matched_forms) {
If you change the type of ConsumerCallbackRunner then it'll be much cleaner.
GetLoginImpl transfers the ownership to GetIE7LoginIfNecessary which transfers
it to callback_runner.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:166:
obtained_forms.weak_clear();  // TODO(vabr): Change request to have a
Why? request->result() was probably empty. However, if not then we leak here.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:185:
obtained_forms.weak_clear();  // TODO(vabr): Change request to have a
Same as above.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:369: for (const
auto& form : expected_autofillable) {
const auto*

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:372: for (const
auto& form : expected_blacklisted) {
see above

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:393:
.WillOnce(WithArg<0>(STLDeleteElements0()));
This causes crashes in the tests. You don't need it anymore.

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store.h:227: typedef
base::Callback<void(ScopedVector<autofill::PasswordForm>*)>
Why not void(ScopedVector<autofill::PasswordForm>) here and in
CopyAndForwardLoginsResult?

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
File components/password_manager/core/browser/password_store_default.cc (right):

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:86: for
(const auto& form : forms) {
Here you wanted "const auto* form" or just autofill::PasswordForm*.

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:104: for
(const auto& form : forms) {
The same as above.

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:131:
logins.get().swap(*request->result());
logins.swap(*request->result());

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:141:
logins.get().swap(*request->result());
logins.swap(*request->result());

[vabr (Chromium), 2015-01-28 13:27:37]

Thanks a lot, Vasilii, your review has been very fast and helpful.

Please have another look.

Cheers,
Vaclav

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x.cc:713: for (const
auto& form : forms) {
On 2015/01/27 20:45:51, vasilii wrote:
> autofill::PasswordForm* form

Done.
I guess I was a bit confused about the new for loop syntax, should have taken
look before. At least I finally did take a look, sorry for those silly mistakes.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:430: form = nullptr;
On 2015/01/27 20:45:51, vasilii wrote:
> you could weak_clear |forms| instead.

I could, but that extends the period of time when two ScopedVectors think they
own the same object. Suppose somebody added an early return in the for-loop
later, then the weak_clear() would not be called, and instead some forms would
be double-freed.

What is the reason against nullptr and for weak_clear()?

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:462: db_form = nullptr;
On 2015/01/27 20:45:51, vasilii wrote:
> again I'd prefer weak_clear.

Please see my response above.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:471: using std::swap;
On 2015/01/27 20:45:51, vasilii wrote:
> excessive here.

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:473: swap(keychain_form,
unused_keychain_forms.back());
On 2015/01/27 20:45:51, vasilii wrote:
> It's easier to understand
> 
> unused_keychain_forms.push_back(keychain_form);
> keychain_form = nullptr;

Agreed. This was an overlooked relic from a past patch set.
Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:526: db_form = nullptr;
On 2015/01/27 20:45:51, vasilii wrote:
> I think weak_clear is better here.

Please see my answer above.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:536: for (auto& item :
keychain_items) {
On 2015/01/27 20:45:51, vasilii wrote:
> Here I'd prefer "SecKeychainItemRef item" but not a reference to the pointer.

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:588:
ScopedVector<autofill::PasswordForm>* matches) {
On 2015/01/27 20:45:51, vasilii wrote:
> returning this object would simplify the caller.

Done.

(I misread the definition of ScopedVector and thought it does not have the
scoped_ptr's Pass() and friends to make it work as a return value. Now I read
base/move.h and repent my mistake. :))

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:742:
ScopedVector<autofill::PasswordForm>* forms) {
On 2015/01/27 20:45:51, vasilii wrote:
> returning this would simplify the code.

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:743: for (const auto& item
: *items) {
On 2015/01/27 20:45:51, vasilii wrote:
> You are iterating over pointers. I'd prefer "SecKeychainItemRef item"

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1025: for (const auto&
db_form : database_forms) {
On 2015/01/27 20:45:51, vasilii wrote:
> const reference to pointer seems to be an overhead.

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1068:
obtained_forms.weak_clear();  // TODO(vabr): Change request to have a
On 2015/01/27 20:45:51, vasilii wrote:
> Seems to be a leak.

You are right. I honestly don't know what I was thinking.
Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1077:
obtained_forms.weak_clear();  // TODO(vabr): Change request to have a
On 2015/01/27 20:45:51, vasilii wrote:
> Same here.

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1122: for (const auto&
db_form : database_forms) {
On 2015/01/27 20:45:51, vasilii wrote:
> a reference to the pointer

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_win.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_win.cc:100: void
PasswordStoreWin::DBHandler::GetIE7Results(
On 2015/01/27 20:45:51, vasilii wrote:
> I think it's cleaner to return ScopedVector<autofill::PasswordForm>.

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_win.cc:195:
ScopedVector<autofill::PasswordForm>* matched_forms) {
On 2015/01/27 20:45:51, vasilii wrote:
> If you change the type of ConsumerCallbackRunner then it'll be much cleaner.
> GetLoginImpl transfers the ownership to GetIE7LoginIfNecessary which transfers
> it to callback_runner.

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:166:
obtained_forms.weak_clear();  // TODO(vabr): Change request to have a
On 2015/01/27 20:45:51, vasilii wrote:
> Why? request->result() was probably empty. However, if not then we leak here.

You are correct.
Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:185:
obtained_forms.weak_clear();  // TODO(vabr): Change request to have a
On 2015/01/27 20:45:51, vasilii wrote:
> Same as above.

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:369: for (const
auto& form : expected_autofillable) {
On 2015/01/27 20:45:52, vasilii wrote:
> const auto*

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:372: for (const
auto& form : expected_blacklisted) {
On 2015/01/27 20:45:51, vasilii wrote:
> see above

Done.

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:393:
.WillOnce(WithArg<0>(STLDeleteElements0()));
On 2015/01/27 20:45:52, vasilii wrote:
> This causes crashes in the tests. You don't need it anymore.

Actually, the STLDeleteElements still needs to be here, because
OnGetPasswordStoreResults is still responsible for deleting its argument (see
MaybeCallConsumerCallback in password_store.cc).

The failures in the ASAN tests were due to line 417 below, which I'll now
remove.

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store.h:227: typedef
base::Callback<void(ScopedVector<autofill::PasswordForm>*)>
On 2015/01/27 20:45:52, vasilii wrote:
> Why not void(ScopedVector<autofill::PasswordForm>) here and in
> CopyAndForwardLoginsResult?

Done.

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
File components/password_manager/core/browser/password_store_default.cc (right):

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:86: for
(const auto& form : forms) {
On 2015/01/27 20:45:52, vasilii wrote:
> Here you wanted "const auto* form" or just autofill::PasswordForm*.

Done.

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:104: for
(const auto& form : forms) {
On 2015/01/27 20:45:52, vasilii wrote:
> The same as above.

Done.

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:131:
logins.get().swap(*request->result());
On 2015/01/27 20:45:52, vasilii wrote:
> logins.swap(*request->result());

Done.

https://codereview.chromium.org/825773003/diff/310001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:141:
logins.get().swap(*request->result());
On 2015/01/27 20:45:52, vasilii wrote:
> logins.swap(*request->result());

Done.

[vasilii, 2015-01-28 15:09:03]

lgtm

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:430: form = nullptr;
On 2015/01/28 13:27:36, vabr (Chromium) wrote:
> On 2015/01/27 20:45:51, vasilii wrote:
> > you could weak_clear |forms| instead.
> 
> I could, but that extends the period of time when two ScopedVectors think they
> own the same object. Suppose somebody added an early return in the for-loop
> later, then the weak_clear() would not be called, and instead some forms would
> be double-freed.
> 
> What is the reason against nullptr and for weak_clear()?

It's performance related. You know that |forms| contains just zeros but still
iterate over it and release the pointers.
Optional: add weak_clear() before swap().

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:526:
ScopedVector<autofill::PasswordForm> keychain_matches(
nit: ScopedVector<autofill::PasswordForm> keychain_matches = ... would be easier
to read while being semantically identical.

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac_unittest.cc (right):

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1077:
ScopedVector<autofill::PasswordForm> all_passwords(
nit: here and below I'd prefer ScopedVector<autofill::PasswordForm> var = ...

[vabr (Chromium), 2015-01-28 16:36:27]

Thanks, Vasilii!

Comments addressed below. I'll wait for the trybots and repetition of the
benchmark on my Mac, and if there are no surprises, I'll land it.

Cheers,
Vaclav

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/825773003/diff/310001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:430: form = nullptr;
On 2015/01/28 15:09:03, vasilii wrote:
> On 2015/01/28 13:27:36, vabr (Chromium) wrote:
> > On 2015/01/27 20:45:51, vasilii wrote:
> > > you could weak_clear |forms| instead.
> > 
> > I could, but that extends the period of time when two ScopedVectors think
they
> > own the same object. Suppose somebody added an early return in the for-loop
> > later, then the weak_clear() would not be called, and instead some forms
would
> > be double-freed.
> > 
> > What is the reason against nullptr and for weak_clear()?
> 
> It's performance related. You know that |forms| contains just zeros but still
> iterate over it and release the pointers.
> Optional: add weak_clear() before swap().

To record our offline discussion: a benchmark run in release mode showed that
even for vectors of size of tens of thousands, the time to destroy them is in
the order of tens of microseconds (on my machine) with or without weak_clear().
What takes time in the weak_clear() case could be the deallocation of the space
previously taken by the vector, but that's just a hypothesis.

(Just for comparison, the same benchmark run in debug mode did show the expected
asymptotic difference between the O(1) destruction with weak_clear() and the
linear-time destruction in the absence of weak_clear().)

The performance gain thus seems rather negligible, and would come on the cost of
code readability and robustness -- this whole refactoring is to get rid of
low-level memory management. So with vasilii@'s permission, I'm not addressing
this comment.


Attachment -- the benchmark, for the record:
  for (unsigned size_shift = 0; size_shift < 17; ++size_shift) {
    const size_t size = 1 << size_shift;
    std::vector<autofill::PasswordForm*> v(size, nullptr);
    base::TimeTicks t;
    {
      ScopedVector<autofill::PasswordForm> sv;
      sv.swap(v);
      t = base::TimeTicks::Now();
      // sv.weak_clear();
    }
    base::TimeDelta elapsed = base::TimeTicks::Now() - t;
    LOG(INFO) << "For size=" << size << ", no weak_clear, elapsed "
               << elapsed.InMicroseconds() << "us";
  }
  for (unsigned size_shift = 0; size_shift < 17; ++size_shift) {
    const size_t size = 1 << size_shift;
    std::vector<autofill::PasswordForm*> v(size, nullptr);
    base::TimeTicks t;
    {
      ScopedVector<autofill::PasswordForm> sv;
      sv.swap(v);
      t = base::TimeTicks::Now();
      sv.weak_clear();
    }
    base::TimeDelta elapsed = base::TimeTicks::Now() - t;
    LOG(INFO) << "For size=" << size << ", + weak_clear, elapsed "
               << elapsed.InMicroseconds() << "us";
  }

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:526:
ScopedVector<autofill::PasswordForm> keychain_matches(
On 2015/01/28 15:09:03, vasilii wrote:
> nit: ScopedVector<autofill::PasswordForm> keychain_matches = ... would be
easier
> to read while being semantically identical.

Done.

You are right: Both the r-value constructor and the operator= of ScopedVector do
the same swap(). Generally, I prefer initialising variables through constructors
rather than assignments, because assignment always means -- first assign default
values to the data members, then replace them -- whereas constructors could
potentially skip the default-value step. However, here there is no distinction,
so I'll rewrite it to assignment.

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac_unittest.cc (right):

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1077:
ScopedVector<autofill::PasswordForm> all_passwords(
On 2015/01/28 15:09:03, vasilii wrote:
> nit: here and below I'd prefer ScopedVector<autofill::PasswordForm> var = ...

Done.

[vasilii, 2015-01-28 17:27:23]

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/825773003/diff/370001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:526:
ScopedVector<autofill::PasswordForm> keychain_matches(
On 2015/01/28 16:36:27, vabr (Chromium) wrote:
> On 2015/01/28 15:09:03, vasilii wrote:
> > nit: ScopedVector<autofill::PasswordForm> keychain_matches = ... would be
> easier
> > to read while being semantically identical.
> 
> Done.
> 
> You are right: Both the r-value constructor and the operator= of ScopedVector
do
> the same swap(). Generally, I prefer initialising variables through
constructors
> rather than assignments, because assignment always means -- first assign
default
> values to the data members, then replace them -- whereas constructors could
> potentially skip the default-value step. However, here there is no
distinction,
> so I'll rewrite it to assignment.

There is no operator= here. "MyObject o = something" invokes the copy
constructor. This is just a difference in syntax.

[vabr (Chromium), 2015-01-28 17:28:09]

The benchmark on Mac is still compiling. I will land this now, and if the
benchmark shows something interesting tomorrow, will prepare a follow-up CL.

Cheers,
Vaclav

[vabr (Chromium), 2015-01-28 17:30:07]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2015-01-28 17:30:20]

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

[commit-bot: I haz the power, 2015-01-28 17:44:06]

Committed patchset #6 (id:390001)

[commit-bot: I haz the power, 2015-01-28 17:45:00]

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