Integrate ManagePasswordsState into ManagePasswordsUIController.

Integrate ManagePasswordsState into ManagePasswordsUIController.

BUG=461801
Committed: https://crrev.com/d42cf56fd325b1b97ae7f9d181c9e0078c85c148
Cr-Commit-Position: refs/heads/master@{#320906}

[vasilii, 2015-03-16 13:13:58]

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

[vasilii, 2015-03-16 17:50:40]

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

[vasilii, 2015-03-16 17:53:46]

Hi guys,

please review
groby@: chrome/browser/ui/cocoa
vabr@: everything else

[groby-ooo-7-16, 2015-03-16 22:03:17]

c/b/ui/cocoa syntactically LGTM (unless the callback indicates resource issues)

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_account_chooser_view_controller_unittest.mm
(right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_account_chooser_view_controller_unittest.mm:132:
base::Callback<void(const password_manager::CredentialInfo&)>()));
Is there any need for the callback to be executed so that resources can be
freed, or is it purely informational?

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:98: const
ScopedVector<const autofill::PasswordForm>& local_credentials() const {
Please add a comment what "local_credentials" and "federated_credentials" are -
I'm not sure I follow why it's OK to fold best_matches_ into local_credentials_

[vabr (Chromium), 2015-03-17 09:25:58]

Hi Vasilii,

Looking good, but I have a couple of questions before the approval.

Thanks!
Vaclav

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_state.cc:171:
DCHECK(local_credentials_forms_.front()->blacklisted_by_user);
Why is this important?

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_state.cc:275:
password_manager::PasswordManagerClient* client =
Why the change from getting the client directly to getting it through the
WebContents instead?

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_test.cc (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_test.cc:73:
test_form()->username_value = kTestUsername;
I'm surprised the username_value needs to be set for blacklisted credentials.
This should be ignored by the production code, so why do we care about it in
tests?

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:247:
DCHECK(state() == password_manager::ui::BLACKLIST_STATE);
nit:
DCHECK_EQ(password_manager::ui::BLACKLIST_STATE, state());
for more information in case of failure (the value of state()).

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:285: state() ==
password_manager::ui::CONFIRMATION_STATE);
optional nit:
Because we cannot do DCHECK_EQ here, we could at least add
<< state()
at the end.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:145: // Called when
a passwordform is autofilled, when a new PasswordForm is
nit: One more all-lowercase "passwordform" still left on this line.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:174:
ManagePasswordsState passwords_data_;
optional nit: Personally I would prefer the name to match the type, e.g.,
|passwords_state_| instead of *data_. The "data" hints at structures like
PasswordFormData, and one has to look up this line to actually discover it is
the State object.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Have you though about merging this class into
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc, now that
it is only used there?
I actually support leaving it separately, because it is fairly big; but I just
wanted to check if you have considered it and what did you think about it.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h:63: //
Simulated the pending password state. |best_matches| can't be empty.
I'm confused about the past tense (simulated). Is it a typo (from simulate), or
does this mean that this should be called after the pending state was somehow
simulated? The latter case would need more explanation.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:36: //
This mock is used in tests to ensure that we're just testing the controller
The comment speaks about a mock, but the class below is not a mock (in the GMock
sense), and neither is named as a mock. And there is another class inheriting
from the same parent, which is called mock. This is a bit confusing.

[vasilii, 2015-03-17 10:49:49]

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_account_chooser_view_controller_unittest.mm
(right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_account_chooser_view_controller_unittest.mm:132:
base::Callback<void(const password_manager::CredentialInfo&)>()));
On 2015/03/16 22:03:17, groby wrote:
> Is there any need for the callback to be executed so that resources can be
> freed, or is it purely informational?

In general, it should be. But this is a feature of ManagePasswordsUIController
which is mocked here. One test below has
EXPECT_TRUE(ui_controller()->choose_credential()); It tests that the UI
controller was called. Normally it'd proceed and call the callback but not for
ManagePasswordsUIControllerMock.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:98: const
ScopedVector<const autofill::PasswordForm>& local_credentials() const {
On 2015/03/16 22:03:17, groby wrote:
> Please add a comment what "local_credentials" and "federated_credentials" are
-
> I'm not sure I follow why it's OK to fold best_matches_ into
local_credentials_

Done.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_state.cc:171:
DCHECK(local_credentials_forms_.front()->blacklisted_by_user);
On 2015/03/17 09:25:57, vabr (Chromium) wrote:
> Why is this important?

If you look at the current code in
ManagePasswordsUIController::UnblacklistSite() then you'll see that we always
remove the first form supposing that it's the blacklisted one. It'll be a pity
to remove something else instead.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_state.cc:275:
password_manager::PasswordManagerClient* client =
On 2015/03/17 09:25:57, vabr (Chromium) wrote:
> Why the change from getting the client directly to getting it through the
> WebContents instead?

I changed it back so the tests set it too.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_test.cc (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_test.cc:73:
test_form()->username_value = kTestUsername;
On 2015/03/17 09:25:57, vabr (Chromium) wrote:
> I'm surprised the username_value needs to be set for blacklisted credentials.
> This should be ignored by the production code, so why do we care about it in
> tests?

Done.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:247:
DCHECK(state() == password_manager::ui::BLACKLIST_STATE);
On 2015/03/17 09:25:57, vabr (Chromium) wrote:
> nit:
> DCHECK_EQ(password_manager::ui::BLACKLIST_STATE, state());
> for more information in case of failure (the value of state()).

Done.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:285: state() ==
password_manager::ui::CONFIRMATION_STATE);
On 2015/03/17 09:25:57, vabr (Chromium) wrote:
> optional nit:
> Because we cannot do DCHECK_EQ here, we could at least add
> << state()
> at the end.

Done.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:145: // Called when
a passwordform is autofilled, when a new PasswordForm is
On 2015/03/17 09:25:58, vabr (Chromium) wrote:
> nit: One more all-lowercase "passwordform" still left on this line.

Done.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:174:
ManagePasswordsState passwords_data_;
On 2015/03/17 09:25:57, vabr (Chromium) wrote:
> optional nit: Personally I would prefer the name to match the type, e.g.,
> |passwords_state_| instead of *data_. The "data" hints at structures like
> PasswordFormData, and one has to look up this line to actually discover it is
> the State object.

But then there is a confusion with state() which used very often in the code.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/03/17 09:25:58, vabr (Chromium) wrote:
> Have you though about merging this class into
> chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc, now
that
> it is only used there?
> I actually support leaving it separately, because it is fairly big; but I just
> wanted to check if you have considered it and what did you think about it.

It's widely used by the Mac code.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h:63: //
Simulated the pending password state. |best_matches| can't be empty.
On 2015/03/17 09:25:58, vabr (Chromium) wrote:
> I'm confused about the past tense (simulated). Is it a typo (from simulate),
or
> does this mean that this should be called after the pending state was somehow
> simulated? The latter case would need more explanation.

It's a typo of course.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:36: //
This mock is used in tests to ensure that we're just testing the controller
On 2015/03/17 09:25:58, vabr (Chromium) wrote:
> The comment speaks about a mock, but the class below is not a mock (in the
GMock
> sense), and neither is named as a mock. And there is another class inheriting
> from the same parent, which is called mock. This is a bit confusing.

Done.

[vabr (Chromium), 2015-03-17 11:55:13]

c/b/password_manager and c/b/ui/*passwords/ LGTM with some comments below.

Thanks!
Vaclav

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_state.cc:171:
DCHECK(local_credentials_forms_.front()->blacklisted_by_user);
On 2015/03/17 10:49:48, vasilii wrote:
> On 2015/03/17 09:25:57, vabr (Chromium) wrote:
> > Why is this important?
> 
> If you look at the current code in
> ManagePasswordsUIController::UnblacklistSite() then you'll see that we always
> remove the first form supposing that it's the blacklisted one. It'll be a pity
> to remove something else instead.

Thanks for explaining. I added some further comments on places where I believe
this should be mentioned.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:174:
ManagePasswordsState passwords_data_;
On 2015/03/17 10:49:48, vasilii wrote:
> On 2015/03/17 09:25:57, vabr (Chromium) wrote:
> > optional nit: Personally I would prefer the name to match the type, e.g.,
> > |passwords_state_| instead of *data_. The "data" hints at structures like
> > PasswordFormData, and one has to look up this line to actually discover it
is
> > the State object.
> 
> But then there is a confusion with state() which used very often in the code.

Fair enough. Then we should probably rename ManagePasswordsState to
ManagePasswordsData eventually. Not needed in this CL.

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h (right):

https://codereview.chromium.org/1009953002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/03/17 10:49:48, vasilii wrote:
> On 2015/03/17 09:25:58, vabr (Chromium) wrote:
> > Have you though about merging this class into
> > chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc, now
> that
> > it is only used there?
> > I actually support leaving it separately, because it is fairly big; but I
just
> > wanted to check if you have considered it and what did you think about it.
> 
> It's widely used by the Mac code.

Ah right, I always forget about the stuff not cross-referenced in CodeSearch.
Never mind, then.

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_state.cc:88: // otherwise.
nit: Could you please also mention here that this is because UnblacklistSite()
expects the first saved password to be the blacklisted credential?

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_state.h (right):

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_state.h:34: // The embedder if this
class has to set the client for logging.
typo: if -> of

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:252:
password_store->RemoveLogin(*passwords_data_.GetCurrentForms().front());
nit: Consider adding a DCHECK that the removed credential is blacklisted. Also,
it might be good to mention this requirement at the declaration comment for
UnblacklistSite.

[vasilii, 2015-03-17 12:11:33]

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_state.cc:88: // otherwise.
On 2015/03/17 11:55:13, vabr (Chromium) wrote:
> nit: Could you please also mention here that this is because UnblacklistSite()
> expects the first saved password to be the blacklisted credential?

Done.

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_state.h (right):

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_state.h:34: // The embedder if this
class has to set the client for logging.
On 2015/03/17 11:55:13, vabr (Chromium) wrote:
> typo: if -> of

Done.

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/1009953002/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:252:
password_store->RemoveLogin(*passwords_data_.GetCurrentForms().front());
On 2015/03/17 11:55:13, vabr (Chromium) wrote:
> nit: Consider adding a DCHECK that the removed credential is blacklisted.
Also,
> it might be good to mention this requirement at the declaration comment for
> UnblacklistSite.

Done.

[vasilii, 2015-03-17 12:13:57]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-03-17 12:13:57]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org,
vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1009953002/#ps100001
(title: "last comments")

[commit-bot: I haz the power, 2015-03-17 12:14:05]

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

[vabr (Chromium), 2015-03-17 12:14:49]

c/b/password_manager and c/b/ui/*passwords/ LGTM!

[commit-bot: I haz the power, 2015-03-17 13:05:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-17 13:15:52]

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