Introduce ManagePasswordsState class.

Introduce ManagePasswordsState class.

It will simplify ManagePasswordsUIController. The tests are the same as in ManagePasswordsUIControllerTest.

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

[vasilii, 2015-03-09 18:41:30]

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

[vasilii, 2015-03-09 18:41:30]

Hi Vaclav,

please review. The CL is big but it copies the logic from
ManagePasswordsUIController. The same about the tests.

[vabr (Chromium), 2015-03-10 10:39:57]

LGTM with comments.

Could you perhaps also place remove-TODOs in the ManagePasswordsUIController
parts which are to be replaced with the new class eventually?

Cheers,
Vaclav

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.cc:57: void
UpdateFormInVector(const autofill::PasswordForm& updated_form,
This function seems to assume that there is at most one form in |forms| which
matches the unique key of |updated_forms|. Could you please mention that in a
comment?

This also applies to RemoveFormFromVector.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.cc:166: void
ManagePasswordsState::TransitionToState(
The declaration comment lists the allowed transitions, but the code does not
seem to check that.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.cc:187: if
(changed_form.blacklisted_by_user &&
The style guide weakly suggests braces for this if and the one on lines 196+.
"conditional or loop statements with complex conditions or statements may be
more readable with curly braces"
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.cc:214:
local_credentials_forms_.push_back(new autofill::PasswordForm(form));
Should we check that a form with the same unique key is not already in
local_credentials_forms_?

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_state.h (right):

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.h:35: void
SetClient(password_manager::PasswordManagerClient* client) {
nit: The style guide seems to indicate set_client for this kind of inline
setter.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.h:39: // The methods below
reset the object.
At this point of reading through the class definition, it is not entirely clear
what the meaning and consequences of a reset are. Could you describe the
consequences, to help the potential caller decide if the resetting methods are a
good choice or a bad one?

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_state_unittest.cc (right):

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state_unittest.cc:66: NULL,
&client_,
nit: nullptr

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state_unittest.cc:175:
EXPECT_THAT(passwords_data().federated_credentials_forms(), IsEmpty());
nit: On other lines (e.g., 119) the alternative formulation of
EXPECT_TRUE(...empty());
is used. Is this random, or are there special reasons to use EXPECT_THAT here?

[vasilii, 2015-03-10 11:34:48]

I'll touch the whole ManagePasswordsUIController but I added a TODO.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.cc:57: void
UpdateFormInVector(const autofill::PasswordForm& updated_form,
On 2015/03/10 10:39:57, vabr (Chromium) wrote:
> This function seems to assume that there is at most one form in |forms| which
> matches the unique key of |updated_forms|. Could you please mention that in a
> comment?
> 
> This also applies to RemoveFormFromVector.

Done.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.cc:166: void
ManagePasswordsState::TransitionToState(
On 2015/03/10 10:39:56, vabr (Chromium) wrote:
> The declaration comment lists the allowed transitions, but the code does not
> seem to check that.

Done.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.cc:187: if
(changed_form.blacklisted_by_user &&
On 2015/03/10 10:39:57, vabr (Chromium) wrote:
> The style guide weakly suggests braces for this if and the one on lines 196+.
> "conditional or loop statements with complex conditions or statements may be
> more readable with curly braces"
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals

Done.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.cc:214:
local_credentials_forms_.push_back(new autofill::PasswordForm(form));
On 2015/03/10 10:39:56, vabr (Chromium) wrote:
> Should we check that a form with the same unique key is not already in
> local_credentials_forms_?

OK. Let's be on the safe side.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_state.h (right):

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.h:35: void
SetClient(password_manager::PasswordManagerClient* client) {
On 2015/03/10 10:39:57, vabr (Chromium) wrote:
> nit: The style guide seems to indicate set_client for this kind of inline
> setter.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

Done.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state.h:39: // The methods below
reset the object.
On 2015/03/10 10:39:57, vabr (Chromium) wrote:
> At this point of reading through the class definition, it is not entirely
clear
> what the meaning and consequences of a reset are. Could you describe the
> consequences, to help the potential caller decide if the resetting methods are
a
> good choice or a bad one?

Done.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_state_unittest.cc (right):

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state_unittest.cc:66: NULL,
&client_,
On 2015/03/10 10:39:57, vabr (Chromium) wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/993513006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_state_unittest.cc:175:
EXPECT_THAT(passwords_data().federated_credentials_forms(), IsEmpty());
On 2015/03/10 10:39:57, vabr (Chromium) wrote:
> nit: On other lines (e.g., 119) the alternative formulation of
> EXPECT_TRUE(...empty());
> is used. Is this random, or are there special reasons to use EXPECT_THAT here?

I randomly changed my mind :-)
Now I'm consistent.

[vasilii, 2015-03-10 11:35:42]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-03-10 11:35:43]

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

[commit-bot: I haz the power, 2015-03-10 11:35:55]

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

[vabr (Chromium), 2015-03-10 12:26:22]

Still LGTM, although with some minor comments.
Cheers,
Vaclav

https://codereview.chromium.org/993513006/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/993513006/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_state.cc:57: // Updates a form in
|forms| that has the same unique key as |updated_form|.
Maybe there was a misunderstanding -- the point I was hoping to be mentioned was
that this function assumes that not more than one form in |forms| needs to be
updated.
Also, it would be good to comment on the meaning of the return value.

https://codereview.chromium.org/993513006/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_state_unittest.cc (right):

https://codereview.chromium.org/993513006/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_state_unittest.cc:305:
blacklisted.username_value = base::string16();
optional nit: Calling clear() is shorter and might be easier to read.

[commit-bot: I haz the power, 2015-03-10 12:31:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-10 12:32:15]

Patchset 2 (id:??) landed as
https://crrev.com/910f1ec6f2cdb93dc6278873bd52d23069cb5a1f
Cr-Commit-Position: refs/heads/master@{#319864}