Credential Manager API: pop up the new "Manage accounts" bubble.

Credential Manager API: pop up the new "Manage accounts" bubble.

BUG=400674
Committed: https://crrev.com/78ef603ac76924f74b3831f228d25bce0e6113ad
Cr-Commit-Position: refs/heads/master@{#321101}

[vasilii, 2015-02-24 19:59:56]

vasilii@chromium.org changed reviewers:
+ isherman@chromium.org, mkwst@chromium.org

[vasilii, 2015-02-24 19:59:57]

isherman@chromium.org: Please review changes in 
histograms.xml and password_form.*

mkwst@chromium.org: Please review changes in 
aforementioned files + not mentioned files.

[Ilya Sherman, 2015-02-24 21:45:48]

https://codereview.chromium.org/952023002/diff/1/components/autofill/core/com...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/952023002/diff/1/components/autofill/core/com...
components/autofill/core/common/password_form.h:257: // True if the unique key
for the forms is the same. The unique key is
nit: "key for the forms is the same" -> "keys for the forms are the same"
("keys", "are")

https://codereview.chromium.org/952023002/diff/1/components/autofill/core/com...
components/autofill/core/common/password_form.h:259: // It's used by Sync and
LoginDatabase.
I don't see these uses in the diff.  In fact, I only see one use, within
//chrome/browser/ui/passwords/manage_passwords_ui_controller.cc.  Does this
method need to be defined in the password_form header and implementation at all?

[vasilii, 2015-02-25 09:39:50]

https://codereview.chromium.org/952023002/diff/1/components/autofill/core/com...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/952023002/diff/1/components/autofill/core/com...
components/autofill/core/common/password_form.h:257: // True if the unique key
for the forms is the same. The unique key is
On 2015/02/24 21:45:48, Ilya Sherman wrote:
> nit: "key for the forms is the same" -> "keys for the forms are the same"
> ("keys", "are")

Done.

https://codereview.chromium.org/952023002/diff/1/components/autofill/core/com...
components/autofill/core/common/password_form.h:259: // It's used by Sync and
LoginDatabase.
On 2015/02/24 21:45:48, Ilya Sherman wrote:
> I don't see these uses in the diff.  In fact, I only see one use, within
> //chrome/browser/ui/passwords/manage_passwords_ui_controller.cc.  Does this
> method need to be defined in the password_form header and implementation at
all?

I move this function temporarily out of here. It will be used by
native_backend_kwallet_x.cc in the future.

[Mike West, 2015-02-25 10:11:13]

This patch generally looks fine, but we end up in a complicated state where we
have two manage modes.

After talking things through with Sabine, I think it makes sense to merge them
now rather than later.

[Ilya Sherman, 2015-02-25 19:23:24]

histograms.xml lgtm, thanks

[vasilii, 2015-03-17 16:22:48]

Mike, please review the patch.

[Mike West, 2015-03-17 16:35:25]

On 2015/03/17 at 16:22:48, vasilii wrote:
> Mike, please review the patch.

So we're keeping both states? :(

I'll take a closer look at this tonight or tomorrow morning.

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

On 2015/03/17 16:35:25, Mike West wrote:
> On 2015/03/17 at 16:22:48, vasilii wrote:
> > Mike, please review the patch.
> 
> So we're keeping both states? :(
> 
> I'll take a closer look at this tonight or tomorrow morning.

No, just MANAGE_STATE!

[Mike West, 2015-03-18 08:33:51]

LGTM % failing test.

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:117: if
(IsNewUIActive()) {
Optional nit: I'd vaguely prefer `title_ = IsNewUIActive() ? ... : ...;` here,
but up to you.

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:124: // Returns true
iff the new YOLO UI should be presented to user for managing
Nit: s/YOLO//.

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:321:
passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
This is clearer now, thanks!

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:326:
EXPECT_CALL(url_callback, OnRequestDone(avatar_url));
This expectation is failing. Perhaps you need to pump the event loop?

[Mike West, 2015-03-18 08:34:20]

On 2015/03/17 at 16:37:50, vasilii wrote:
> On 2015/03/17 16:35:25, Mike West wrote:
> > On 2015/03/17 at 16:22:48, vasilii wrote:
> > > Mike, please review the patch.
> > 
> > So we're keeping both states? :(
> > 
> > I'll take a closer look at this tonight or tomorrow morning.
> 
> No, just MANAGE_STATE!

Sorry, that was poor word choice. I like that you got rid of the additional
state. I'm sad that we have to keep both the old and new UIs.

[vasilii, 2015-03-18 09:25:09]

Both UIs is a temporary solution. It's a way better than 2 UI states.

Do you want to have a look at the test?

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:117: if
(IsNewUIActive()) {
On 2015/03/18 08:33:51, Mike West wrote:
> Optional nit: I'd vaguely prefer `title_ = IsNewUIActive() ? ... : ...;` here,
> but up to you.

Done.

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:124: // Returns true
iff the new YOLO UI should be presented to user for managing
On 2015/03/18 08:33:51, Mike West wrote:
> Nit: s/YOLO//.

Done.

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:321:
passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
On 2015/03/18 08:33:51, Mike West wrote:
> This is clearer now, thanks!

Acknowledged.

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/952023002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:326:
EXPECT_CALL(url_callback, OnRequestDone(avatar_url));
On 2015/03/18 08:33:51, Mike West wrote:
> This expectation is failing. Perhaps you need to pump the event loop?

No, we were opening the old UI without avatars.

[Mike West, 2015-03-18 09:29:51]

LGTM, thanks!

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

The CQ bit was checked by vasilii@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/952023002/#ps60001 (title:
"fix the test")

[commit-bot: I haz the power, 2015-03-18 09:31:13]

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

[commit-bot: I haz the power, 2015-03-18 10:42:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-18 10:43:31]

Patchset 4 (id:??) landed as
https://crrev.com/78ef603ac76924f74b3831f228d25bce0e6113ad
Cr-Commit-Position: refs/heads/master@{#321101}