GetLoginsRequest: Use ScopedVector to express ownership of forms

GetLoginsRequest: Use ScopedVector to express ownership of forms

This is a clean-up, it follows up on https://codereview.chromium.org/825773003/.

BUG=451018
Committed: https://crrev.com/64e9247a37f5b2c1cbf287d1a5c89d7fdb3571fe
Cr-Commit-Position: refs/heads/master@{#315341}

[vabr (Chromium), 2015-02-02 16:51:33]

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

[vabr (Chromium), 2015-02-02 17:23:37]

Hi Vasilii,

Please have a look. Take your time, this is not urgent.

Thanks!

Vaclav

https://codereview.chromium.org/866983003/diff/200001/components/password_man...
File components/password_manager/core/browser/password_store_consumer.cc
(right):

https://codereview.chromium.org/866983003/diff/200001/components/password_man...
components/password_manager/core/browser/password_store_consumer.cc:6: #include
"components/autofill/core/common/password_form.h"
This is used to instantiate the constructor (which has to construct the vector
of PasswordForms).

Alternatively, I could move this header to password_store_consumer.h, replacing
the forward declaration. The style guide still seems to be OK with forward
declarations, so I chose keeping it, but don't feel strongly about it.

[vasilii, 2015-02-03 19:22:17]

https://codereview.chromium.org/866983003/diff/200001/components/password_man...
File components/password_manager/core/browser/password_store_consumer.cc
(right):

https://codereview.chromium.org/866983003/diff/200001/components/password_man...
components/password_manager/core/browser/password_store_consumer.cc:6: #include
"components/autofill/core/common/password_form.h"
On 2015/02/02 17:23:37, vabr (Chromium) wrote:
> This is used to instantiate the constructor (which has to construct the vector
> of PasswordForms).
> 
> Alternatively, I could move this header to password_store_consumer.h,
replacing
> the forward declaration. The style guide still seems to be OK with forward
> declarations, so I chose keeping it, but don't feel strongly about it.

I like your approach.

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
File chrome/browser/password_manager/password_store_win_unittest.cc (right):

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:242:
CreatePasswordFormFromData(expected_form_data));
Did you consider changing this function to return scoped_ptr?

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

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:166:
request->result()->swap(obtained_forms.get());
It's better to do whatever we want with |obtained_forms| and swap in the last
line.

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:185:
SortLoginsByOrigin(&request->result()->get());
See above.

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

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:399:
EXPECT_TRUE(password_manager::ContainsSamePasswordFormsPtr(
This place definitely suffered from the changed OnGetPasswordStoreResults()
interface.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:461: if
(const_login.type == PasswordForm::TYPE_GENERATED)
what about "else if" here so we don't save |login| in |credentials_to_keep|
twice?

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:250:
EXPECT_CALL(*store_.get(), GetLogins(_, _, _))
*store_?

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:34:
consumer->results()->clear();
Do you need it?

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:58:
DCHECK(thread_checker_.CalledOnValidThread());
Looks like this is the only place where it's used. What about removing it?

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:82:
base::Passed(result_.Pass())));
base::Passed(&result_)?

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:151:
base::Bind(&CopyAndForwardLoginsResult, base::Passed(request.Pass()));
base::Passed(&request)?

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:251: void
PasswordStore::Schedule(BackendFunc func,
Optional: you can get rid of the template because BackendFunc is just one
concrete, quite simple type.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:256: base::Bind(func,
this, base::Passed(request.Pass())));
base::Passed(&request);

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

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.h:72: class
GetLoginsRequest {
Should we move it to "protected"?

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.h:97: // TODO(dubroy):
Remove this, and instead pass the vector directly to the
Obsolete?

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_store_consumer.h (right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store_consumer.h:30: virtual
void OnGetPasswordStoreResults() = 0;
This is my deepest concern in the CL. I didn't see places except some tests
where the consumer's benefited from having the member variable. On the other
hand, I did see places where it can bite us:
PasswordFormManager once calls OnGetPasswordStoreResults() hoping that results_
is empty.
PasswordFormManagerTest entirely relies on the fact that
OnGetPasswordStoreResults() will clean result().
CredentialManagerDispatcher may leave some trash in result_.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:33:
EXPECT_FALSE(expected_results_.size());
EXPECT_TRUE(expected_results_.empty());

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:37:
ASSERT_TRUE(expected_results_.size());
empty()

[vabr (Chromium), 2015-02-04 16:13:45]

Thanks, Vasilii!

I addressed your comments. Could you please review again_

Thanks,
Vaclav

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
File chrome/browser/password_manager/password_store_win_unittest.cc (right):

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:242:
CreatePasswordFormFromData(expected_form_data));
On 2015/02/03 19:22:16, vasilii wrote:
> Did you consider changing this function to return scoped_ptr?

Good idea! Done.

I updated both copies of the function, and uploaded a separate CL to merge those
copies later. https://codereview.chromium.org/896903003/

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

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:166:
request->result()->swap(obtained_forms.get());
On 2015/02/03 19:22:16, vasilii wrote:
> It's better to do whatever we want with |obtained_forms| and swap in the last
> line.

I share that sentiment. :)
Done.

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:185:
SortLoginsByOrigin(&request->result()->get());
On 2015/02/03 19:22:16, vasilii wrote:
> See above.

Done.

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

https://codereview.chromium.org/866983003/diff/240001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:399:
EXPECT_TRUE(password_manager::ContainsSamePasswordFormsPtr(
On 2015/02/03 19:22:16, vasilii wrote:
> This place definitely suffered from the changed OnGetPasswordStoreResults()
> interface.

Acknowledged.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:461: if
(const_login.type == PasswordForm::TYPE_GENERATED)
On 2015/02/03 19:22:16, vasilii wrote:
> what about "else if" here so we don't save |login| in |credentials_to_keep|
> twice?

Good catch! Done.

Amusing though, that this double-saving did neither cause use-after free before
(due to the copy-by-value) nor after (due to |login| being already a scoped_ptr
and giving out nulls on subsequent releases).

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:250:
EXPECT_CALL(*store_.get(), GetLogins(_, _, _))
On 2015/02/03 19:22:16, vasilii wrote:
> *store_?

Done.

(I tend not to add independent clean-ups to huge CLs, but if you request it, I'm
happy to add it.)

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:34:
consumer->results()->clear();
On 2015/02/03 19:22:16, vasilii wrote:
> Do you need it?

I don't, but I left it to make it clear that the current results are dropped. On
the other hand, it's quite clear what is happening, so I'm dropping this line.
Done.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:58:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/02/03 19:22:17, vasilii wrote:
> Looks like this is the only place where it's used. What about removing it?

It is explained in the TODO I added above. We might need it in the tests to make
sure threads are actually present at all. As the TODO states, this is not ideal,
but I prefer to separate that removal from this CL, which is independent of it.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:82:
base::Passed(result_.Pass())));
On 2015/02/03 19:22:16, vasilii wrote:
> base::Passed(&result_)?

Done.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:151:
base::Bind(&CopyAndForwardLoginsResult, base::Passed(request.Pass()));
On 2015/02/03 19:22:16, vasilii wrote:
> base::Passed(&request)?

Done.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:251: void
PasswordStore::Schedule(BackendFunc func,
On 2015/02/03 19:22:17, vasilii wrote:
> Optional: you can get rid of the template because BackendFunc is just one
> concrete, quite simple type.

Sounds reasonable, the function signature was restricted by the PostTask call
anyway.
Done.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.cc:256: base::Bind(func,
this, base::Passed(request.Pass())));
On 2015/02/03 19:22:17, vasilii wrote:
> base::Passed(&request);

Done.

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

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.h:72: class
GetLoginsRequest {
On 2015/02/03 19:22:17, vasilii wrote:
> Should we move it to "protected"?

Done.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store.h:97: // TODO(dubroy):
Remove this, and instead pass the vector directly to the
On 2015/02/03 19:22:17, vasilii wrote:
> Obsolete?

I'm not sure, because I don't know what "this" in the comment meant. I chatted
with engedy@, who plans some interface refactoring in these parts. Because the
comment is not clear, and we'll have to plan the changes thoroughly anyway, I'm
dropping it, as it's no longer helpful.

Done.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_store_consumer.h (right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store_consumer.h:30: virtual
void OnGetPasswordStoreResults() = 0;
On 2015/02/03 19:22:17, vasilii wrote:
> This is my deepest concern in the CL. I didn't see places except some tests
> where the consumer's benefited from having the member variable. On the other
> hand, I did see places where it can bite us:
> PasswordFormManager once calls OnGetPasswordStoreResults() hoping that
results_
> is empty.
> PasswordFormManagerTest entirely relies on the fact that
> OnGetPasswordStoreResults() will clean result().
> CredentialManagerDispatcher may leave some trash in result_.

Thanks for pointing that out. I agree, and I'm actually not sure why I separated
result from the On* method.
Joined them again.
Done.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:33:
EXPECT_FALSE(expected_results_.size());
On 2015/02/03 19:22:17, vasilii wrote:
> EXPECT_TRUE(expected_results_.empty());

Done.

https://codereview.chromium.org/866983003/diff/240001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:37:
ASSERT_TRUE(expected_results_.size());
On 2015/02/03 19:22:17, vasilii wrote:
> empty()

Done.

[vasilii, 2015-02-05 19:23:27]

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

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:58: void
SaveACopyOfFirstForm(const std::vector<PasswordForm*>& result) {
Turn it to the action taking an argument (PasswordForm* or
std::vector<PasswordForm>*) and copying the input to this argument. Then you can
drop |first_form|. Example:

PasswordForm returned_form;
EXPECT_CALL(consumer, OnGetPasswordStoreResultsConstRef(SizeIs(1u)))
    .WillOnce(Invoke(SaveACopyOfFirstForm(&returned_form)));
// Do something with returned_form

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:225: static
scoped_ptr<PasswordForm> CreatePasswordFormFromData(
I'd put this method and it's neighbors to the namespace {} above. I don't like
'static' keyword.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1271:
PasswordForm returned_form;
Unused now, but to be used in your next PatchSet :-)

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1552:
EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(ElementsAre()))
IsEmpty()

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1563:
.WillOnce(QuitUIMessageLoop());
This is repeated many times here. Sounds like a candidate for
ON_CALL().WillByDefault

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
File chrome/browser/password_manager/password_store_win_unittest.cc (right):

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:389:
EXPECT_CALL(consumer, OnGetPasswordStoreResultsConstRef(ElementsAre()))
Here and below: IsEmpty()

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:390:
.WillOnce(QuitUIMessageLoop());
Seems unnecessary.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:407:
.WillOnce(QuitUIMessageLoop());
Unnecessary?

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:424:
.WillOnce(QuitUIMessageLoop());
Unnecessary?

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

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:416:
EXPECT_CALL(ld_return, OnLoginDatabaseQueryDone(ElementsAre()));
IsEmpty() here and below.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/passwords_helper.cc (right):

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/passwords_helper.cc:29: const char*
kIndexedFakeOrigin = "http://fake-signon-realm.google.com/%d";
I'd suggest to change both variables to const char[] and move them to the
namespace.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/passwords_helper.cc:44:
PasswordStoreConsumerHelper() {}
Probably you don't need to declare it?

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/passwords_helper.h (right):

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/passwords_helper.h:34: //
LivePasswordsSyncTest.
LivePasswordsSyncTest exists only in this comment :-)

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/content/browser/credential_manager_dispatcher.cc:82:
// TODO(mkwst): Use the `one_time_disable_zero_click` flag on the result
The flag is 'skip_zero_click' and it's already implemented.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:250:
.WillOnce(DoAll(WithArg<2>(InvokeConsumer(&result)), Return()));
Do you really need Return() here and below?

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:919:
.WillOnce(DoAll(WithArg<2>(InvokeConsumer(&result)), Return()));
The file is full of these calls. It seems that it's not the condition we want to
test. I'd move this to a test initialiser as
ON_CALL(*store_, GetLogins(_, _,
_)).WillByDefault(WithArg<2>(InvokeEmptyConsumer));

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_store.cc:31: void
MaybeCallConsumerCallback(base::WeakPtr<PasswordStoreConsumer> consumer,
Shouldn't Bind be smart enough to do what you are doing in this method. I mean:

base::Bind(&PasswordStoreConsumer::OnGetPasswordStoreResults, consumer_weak_,
base::Passed(&result_));

If yes, we don't need this method.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_store.cc:55:
DCHECK(thread_checker_.CalledOnValidThread());
After discussing I still think that |thread_checker_| is to be deleted.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/password_store_default_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_store_default_unittest.cc:227:
EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(ElementsAre()));
What about IsEmpty()?

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/test_password_store.h (right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/test_password_store.h:64: void
GetBlacklistLoginsImpl(scoped_ptr<GetLoginsRequest> request) override {}
If we follow the guide line then the method isn't so simple because it destroys
|request|. Regardless, I'd prefer to see both of them together in .cc.

[vabr (Chromium), 2015-02-06 14:16:06]

Thanks, Vasilii.

Please have a look at how I addressed your further comments.

Cheers,
Vaclav

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

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:58: void
SaveACopyOfFirstForm(const std::vector<PasswordForm*>& result) {
On 2015/02/05 19:23:27, vasilii wrote:
> Turn it to the action taking an argument (PasswordForm* or
> std::vector<PasswordForm>*) and copying the input to this argument. Then you
can
> drop |first_form|. Example:
> 
> PasswordForm returned_form;
> EXPECT_CALL(consumer, OnGetPasswordStoreResultsConstRef(SizeIs(1u)))
>     .WillOnce(Invoke(SaveACopyOfFirstForm(&returned_form)));
> // Do something with returned_form

Done.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:225: static
scoped_ptr<PasswordForm> CreatePasswordFormFromData(
On 2015/02/05 19:23:26, vasilii wrote:
> I'd put this method and it's neighbors to the namespace {} above. I don't like
> 'static' keyword.

Done.
BTW., this method and the struct will be removed in
https://codereview.chromium.org/896903003/.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1271:
PasswordForm returned_form;
On 2015/02/05 19:23:26, vasilii wrote:
> Unused now, but to be used in your next PatchSet :-)

Acknowledged. :)

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1552:
EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(ElementsAre()))
On 2015/02/05 19:23:26, vasilii wrote:
> IsEmpty()

Done.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:1563:
.WillOnce(QuitUIMessageLoop());
On 2015/02/05 19:23:26, vasilii wrote:
> This is repeated many times here. Sounds like a candidate for
> ON_CALL().WillByDefault

Done.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
File chrome/browser/password_manager/password_store_win_unittest.cc (right):

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:389:
EXPECT_CALL(consumer, OnGetPasswordStoreResultsConstRef(ElementsAre()))
On 2015/02/05 19:23:27, vasilii wrote:
> Here and below: IsEmpty()

Done.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:390:
.WillOnce(QuitUIMessageLoop());
On 2015/02/05 19:23:27, vasilii wrote:
> Seems unnecessary.

Done.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:407:
.WillOnce(QuitUIMessageLoop());
On 2015/02/05 19:23:27, vasilii wrote:
> Unnecessary?

Done.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:424:
.WillOnce(QuitUIMessageLoop());
On 2015/02/05 19:23:27, vasilii wrote:
> Unnecessary?

Done.

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

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/password...
chrome/browser/password_manager/password_store_x_unittest.cc:416:
EXPECT_CALL(ld_return, OnLoginDatabaseQueryDone(ElementsAre()));
On 2015/02/05 19:23:27, vasilii wrote:
> IsEmpty() here and below.

Done.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/passwords_helper.cc (right):

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/passwords_helper.cc:29: const char*
kIndexedFakeOrigin = "http://fake-signon-realm.google.com/%d";
On 2015/02/05 19:23:27, vasilii wrote:
> I'd suggest to change both variables to const char[] and move them to the
> namespace.

Done.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/passwords_helper.cc:44:
PasswordStoreConsumerHelper() {}
On 2015/02/05 19:23:27, vasilii wrote:
> Probably you don't need to declare it?

Due to the DISALLOW_COPY_AND_ASSIGN macro below, the compiler won't generate it
automatically, so I need to keep it.

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/passwords_helper.h (right):

https://codereview.chromium.org/866983003/diff/380001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/passwords_helper.h:34: //
LivePasswordsSyncTest.
On 2015/02/05 19:23:27, vasilii wrote:
> LivePasswordsSyncTest exists only in this comment :-)

You are right. Seems like it has been renamed to PasswordsSyncPerfTest.
Done.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/content/browser/credential_manager_dispatcher.cc:82:
// TODO(mkwst): Use the `one_time_disable_zero_click` flag on the result
On 2015/02/05 19:23:27, vasilii wrote:
> The flag is 'skip_zero_click' and it's already implemented.

I checked with Mike, and corrected the comment based on that.
Done.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:250:
.WillOnce(DoAll(WithArg<2>(InvokeConsumer(&result)), Return()));
On 2015/02/05 19:23:27, vasilii wrote:
> Do you really need Return() here and below?
No. :) Done.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:919:
.WillOnce(DoAll(WithArg<2>(InvokeConsumer(&result)), Return()));
On 2015/02/05 19:23:27, vasilii wrote:
> The file is full of these calls. It seems that it's not the condition we want
to
> test. I'd move this to a test initialiser as
> ON_CALL(*store_, GetLogins(_, _,
> _)).WillByDefault(WithArg<2>(InvokeEmptyConsumer));

Great idea!
Done.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_store.cc:31: void
MaybeCallConsumerCallback(base::WeakPtr<PasswordStoreConsumer> consumer,
On 2015/02/05 19:23:27, vasilii wrote:
> Shouldn't Bind be smart enough to do what you are doing in this method. I
mean:
> 
> base::Bind(&PasswordStoreConsumer::OnGetPasswordStoreResults, consumer_weak_,
> base::Passed(&result_));
> 
> If yes, we don't need this method.

Good call. WeakPtr should indeed just safely give up when attempting to run
this, and passing the ownership now takes care of the clean-up (which was
apparently the only purpose of this function before).
Done.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_store.cc:55:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/02/05 19:23:27, vasilii wrote:
> After discussing I still think that |thread_checker_| is to be deleted.

Done.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/password_store_default_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_store_default_unittest.cc:227:
EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(ElementsAre()));
On 2015/02/05 19:23:27, vasilii wrote:
> What about IsEmpty()?

Done.

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/test_password_store.h (right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/test_password_store.h:64: void
GetBlacklistLoginsImpl(scoped_ptr<GetLoginsRequest> request) override {}
On 2015/02/05 19:23:27, vasilii wrote:
> If we follow the guide line then the method isn't so simple because it
destroys
> |request|. Regardless, I'd prefer to see both of them together in .cc.

Agreed. Also done for ReportMetricsImpl for consistency.

Done.

[vabr (Chromium), 2015-02-06 18:07:40]

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

[vabr (Chromium), 2015-02-06 18:07:42]

Hi Nicolas.

While Vasilii is reviewing the whole CL, could you please do an OWNERS review
of:

chrome/browser/profiles/profile_manager_browsertest.cc
chrome/browser/sync/test/integration/passwords_helper.{cc|h}
chrome/browser/sync/test/integration/performance/passwords_sync_perf_test.cc

Cheers,
Vaclav

[vabr (Chromium), 2015-02-06 18:11:04]

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

[vabr (Chromium), 2015-02-06 18:11:05]

Monica,

Could you please do an OWNERS review of
chrome/browser/profiles/profile_manager_browsertest.cc ?

(Nicolas, sorry for mistakenly including the above in your review list, I only
need your review for chrome/browser/sync/test/integration/*).

Thanks!
Vaclav

[Nicolas Zea, 2015-02-06 18:31:24]

sync LGTM, thanks for cleaning that up!

[vasilii, 2015-02-09 10:08:35]

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:919:
.WillOnce(DoAll(WithArg<2>(InvokeConsumer(&result)), Return()));
On 2015/02/06 14:16:05, vabr (Chromium) wrote:
> On 2015/02/05 19:23:27, vasilii wrote:
> > The file is full of these calls. It seems that it's not the condition we
want
> to
> > test. I'd move this to a test initialiser as
> > ON_CALL(*store_, GetLogins(_, _,
> > _)).WillByDefault(WithArg<2>(InvokeEmptyConsumer));
> 
> Great idea!
> Done.

I don't see it's done.

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

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1092:
ScopedVector<autofill::PasswordForm> obtained_forms;
In PasswordStoreDefault you have DCHECK(request->result().empty()); here and
below.

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1093:
FillBlacklistLogins(&obtained_forms);
FillBlacklistLogins(request->result())?

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

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:51:
DCHECK(!arg0.empty());
As it's a test then ASSERT_FALSE(arg0.empty()) is more appropriate.

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
File chrome/browser/password_manager/password_store_win_unittest.cc (right):

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:255:
.WillOnce(QuitUIMessageLoop());
Not needed given the line #219

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:346:
expected_forms.get()))).WillOnce(QuitUIMessageLoop());
QuitUIMessageLoop() not needed

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:478:
preferred_match_ = const_login.preferred ? &const_login : preferred_match_;
What about DCHECK(!login) above because you save the permanent pointer to the
form.

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

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:128:
FillAutofillableLogins(&logins);
Why not FillAutofillableLogins(request->result()); here and below?

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:174:
.RetiresOnSaturation();
I'm not sure if you need it here and below. The expected forms seem different.

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
File components/password_manager/core/browser/test_password_store.cc (right):

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
components/password_manager/core/browser/test_password_store.cc:50: for (auto&
forms_for_realm : stored_passwords_) {
const auto&

[vabr (Chromium), 2015-02-09 11:03:06]

New patchsets have been uploaded after l-g-t-m from zea@chromium.org

[vabr (Chromium), 2015-02-09 11:36:06]

Thanks, Vasilii!

Please have yet another look, I addressed your further comments.

Cheers,
Vaclav

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/380001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:919:
.WillOnce(DoAll(WithArg<2>(InvokeConsumer(&result)), Return()));
On 2015/02/09 10:08:34, vasilii wrote:
> On 2015/02/06 14:16:05, vabr (Chromium) wrote:
> > On 2015/02/05 19:23:27, vasilii wrote:
> > > The file is full of these calls. It seems that it's not the condition we
> want
> > to
> > > test. I'd move this to a test initialiser as
> > > ON_CALL(*store_, GetLogins(_, _,
> > > _)).WillByDefault(WithArg<2>(InvokeEmptyConsumer));
> > 
> > Great idea!
> > Done.
> 
> I don't see it's done.

Sorry, I focussed on adding the empty consumer action so much that I forgot the
other part of your comment. Remedied now.

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

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1092:
ScopedVector<autofill::PasswordForm> obtained_forms;
On 2015/02/09 10:08:34, vasilii wrote:
> In PasswordStoreDefault you have DCHECK(request->result().empty()); here and
> below.

Done.

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1093:
FillBlacklistLogins(&obtained_forms);
On 2015/02/09 10:08:35, vasilii wrote:
> FillBlacklistLogins(request->result())?

Done.

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

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac_unittest.cc:51:
DCHECK(!arg0.empty());
On 2015/02/09 10:08:35, vasilii wrote:
> As it's a test then ASSERT_FALSE(arg0.empty()) is more appropriate.

Done.

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
File chrome/browser/password_manager/password_store_win_unittest.cc (right):

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:255:
.WillOnce(QuitUIMessageLoop());
On 2015/02/09 10:08:35, vasilii wrote:
> Not needed given the line #219

Done.

https://codereview.chromium.org/866983003/diff/460001/chrome/browser/password...
chrome/browser/password_manager/password_store_win_unittest.cc:346:
expected_forms.get()))).WillOnce(QuitUIMessageLoop());
On 2015/02/09 10:08:35, vasilii wrote:
> QuitUIMessageLoop() not needed

Done.

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:478:
preferred_match_ = const_login.preferred ? &const_login : preferred_match_;
On 2015/02/09 10:08:35, vasilii wrote:
> What about DCHECK(!login) above because you save the permanent pointer to the
> form.

Done.

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

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:128:
FillAutofillableLogins(&logins);
On 2015/02/09 10:08:35, vasilii wrote:
> Why not FillAutofillableLogins(request->result()); here and below?

Done.

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:174:
.RetiresOnSaturation();
On 2015/02/09 10:08:35, vasilii wrote:
> I'm not sure if you need it here and below. The expected forms seem different.

I do need it. Without the saturation the expectations spew logs about unexpected
calls.

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
File components/password_manager/core/browser/test_password_store.cc (right):

https://codereview.chromium.org/866983003/diff/460001/components/password_man...
components/password_manager/core/browser/test_password_store.cc:50: for (auto&
forms_for_realm : stored_passwords_) {
On 2015/02/09 10:08:35, vasilii wrote:
> const auto&

Done.

[vasilii, 2015-02-09 11:48:41]

lgtm

[vabr (Chromium), 2015-02-09 11:51:29]

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

[vabr (Chromium), 2015-02-09 11:51:31]

Jochen,

Would you mind rubber-stamping
chrome/browser/profiles/profile_manager_browsertest.cc specifically instead of
noms@, so that I can land the patch today?

The rest of the patch is already reviewed.

Cheers,
Vaclav

[vabr (Chromium), 2015-02-09 11:53:52]

New patchsets have been uploaded after l-g-t-m from vasilii@chromium.org

[jochen (gone - plz use gerrit), 2015-02-09 14:24:38]

chrome/browser/profiles/profile_manager_browsertest.cc lgtm

[vabr (Chromium), 2015-02-09 14:36:18]

New patchsets have been uploaded after l-g-t-m from jochen@chromium.org

[vabr (Chromium), 2015-02-09 14:57:07]

Thanks all for your approvals.

The post-approval patch sets were just rebasing. In particular, 27 was a failed
and 28 a successful attempt to rebase onto
https://codereview.chromium.org/896803003/.

Given the approvals, I'm sending this to the CQ now.

Cheers,
Vaclav

[vabr (Chromium), 2015-02-09 14:57:17]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2015-02-09 14:58:44]

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

[noms (inactive), 2015-02-09 15:14:00]

tardy profiles lgtm

[vabr (Chromium), 2015-02-09 15:32:34]

On 2015/02/09 15:14:00, Monica Dinculescu wrote:
> tardy profiles lgtm

Thank you!
Vaclav

[commit-bot: I haz the power, 2015-02-09 15:39:56]

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

[commit-bot: I haz the power, 2015-02-09 15:39:57]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[vabr (Chromium), 2015-02-09 17:07:41]

New patchsets have been uploaded after l-g-t-m from noms@chromium.org

[vabr (Chromium), 2015-02-09 17:09:59]

I just added patch set 29.
The diff of patch set 29 against 28 is a correction of the rebase mentioned
previously.
Because it is a straightforward and very small fix, I'll keep CQing this CL.

Cheers,
Vaclav

[vabr (Chromium), 2015-02-09 17:20:10]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2015-02-09 17:20:48]

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

[commit-bot: I haz the power, 2015-02-09 18:12:43]

Committed patchset #29 (id:550001)

[commit-bot: I haz the power, 2015-02-09 18:13:27]

Patchset 29 (id:??) landed as
https://crrev.com/64e9247a37f5b2c1cbf287d1a5c89d7fdb3571fe
Cr-Commit-Position: refs/heads/master@{#315341}