Credential Manager API: Showing both local and federated logins.

Credential Manager API: Showing both local and federated logins.

Showing both local and federated logins and passing information about
type of credentials choosing by user back to blink.

BUG=400674
R=mkwst@chromium.org,vasilii@chromium.org

Committed: https://crrev.com/27ce3350383649c74870c9417b9e64b6f4194a7b
Cr-Commit-Position: refs/heads/master@{#313053}

[melandory, 2015-01-12 17:44:43]

Patchset #1 (id:1) has been deleted

[melandory, 2015-01-12 17:44:50]

Patchset #1 (id:20001) has been deleted

[melandory, 2015-01-12 17:56:38]

Patchset #1 (id:40001) has been deleted

[vasilii, 2015-01-12 18:36:45]

The approach is correct.

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

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:2: 
??

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

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:124: origin_ =
local_credentials[0]->origin;
TODO: fix the crash when |local_credentials| is empty.

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

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:21: enum class
CredentialType : unsigned int;
doesn't match the definition

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:85: bool
was_chosen,
no need for this parameter anymore

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:113:
ScopedVector<autofill::PasswordForm>& new_password_forms() {
unused

https://codereview.chromium.org/848723002/diff/60001/components/password_mana...
File components/password_manager/content/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/848723002/diff/60001/components/password_mana...
components/password_manager/content/common/credential_manager_types.cc:37: :
type(form_type),
|type| is overwritten below

[melandory, 2015-01-13 09:07:55]

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

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:2: 
On 2015/01/12 18:36:45, vasilii wrote:
> ??

I haven't make code ready for review, that is why I only asked about approach :P

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

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:21: enum class
CredentialType : unsigned int;
On 2015/01/12 18:36:45, vasilii wrote:
> doesn't match the definition

Yes, because I've submit another CL, which changes definition and will allow
forward declaration of CredentialType.

https://codereview.chromium.org/848723002/diff/60001/components/password_mana...
File components/password_manager/content/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/848723002/diff/60001/components/password_mana...
components/password_manager/content/common/credential_manager_types.cc:37: :
type(form_type),
On 2015/01/12 18:36:45, vasilii wrote:
> |type| is overwritten below

Yep, code which removes this accidently went here
https://codereview.chromium.org/835523004

[melandory, 2015-01-13 10:16:02]

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

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:124: origin_ =
local_credentials[0]->origin;
On 2015/01/12 18:36:45, vasilii wrote:
> TODO: fix the crash when |local_credentials| is empty.

Done.

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

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:85: bool
was_chosen,
On 2015/01/12 18:36:45, vasilii wrote:
> no need for this parameter anymore

Done.

[melandory, 2015-01-13 11:58:32]

Patchset #3 (id:100001) has been deleted

[melandory, 2015-01-13 11:58:40]

Patchset #2 (id:80001) has been deleted

[melandory, 2015-01-13 12:12:18]

Patchset #2 (id:120001) has been deleted

[melandory, 2015-01-13 12:48:14]

Patchset #2 (id:140001) has been deleted

[melandory, 2015-01-13 13:37:01]

Patchset #2 (id:160001) has been deleted

[melandory, 2015-01-13 14:44:13]

Patchset #2 (id:180001) has been deleted

[melandory, 2015-01-13 15:37:01]

Please review this CL.

Thanks in advance.

[vasilii, 2015-01-13 18:13:29]

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

https://codereview.chromium.org/848723002/diff/200001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:112:
ScopedVector<autofill::PasswordForm>& new_password_forms() {
Is it still used?

https://codereview.chromium.org/848723002/diff/200001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:167: // Federated
credentials.
The comment should explaine that they are used just in the specific state.

https://codereview.chromium.org/848723002/diff/200001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/848723002/diff/200001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:212:
password_manager::CredentialType type) {
It will be shorter to write a method taking a vector and type.

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:54:
const std::vector<autofill::PasswordForm*>& federated_forms,
We should use ScopedVector here. 
ContentCredentialManagerDispatcher::OnGetPasswordStoreResults contains a memory
leak because of this.

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:57:
EXPECT_FALSE(local_forms.empty() && federated_forms.empty());
The function crashes if local_forms.empty()

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:63:
// TODO(vasilii): Do something clever with |federated_forms|.
TODO is irrelevant

[Mike West, 2015-01-13 19:41:46]

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

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:166:
ScopedVector<autofill::PasswordForm> new_password_forms_;
I think you can kill this as well.

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

https://codereview.chromium.org/848723002/diff/200001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:124: //
TODO(melandory): fix the crash when |local_credentials| is empty.
It's a slightly bigger TODO than that; we need the origin in order to display
the correct title in the bubble (e.g. "Select a credential for `example.com`").
If we can't rely on having local credentials from which to extract the origin,
we'll need to pass it in explicitly. That's probably the right thing to do
anyway (in a separate CL).

[vasilii, 2015-01-14 09:57:48]

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

https://codereview.chromium.org/848723002/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:166:
ScopedVector<autofill::PasswordForm> new_password_forms_;
On 2015/01/13 19:41:46, Mike West wrote:
> I think you can kill this as well.

It is for the traditional bubble. In case user adds a new form while the bubble
is open. However, the code for handling this is broken anyway.

[melandory, 2015-01-15 10:04:30]

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:54:
const std::vector<autofill::PasswordForm*>& federated_forms,
On 2015/01/13 18:13:28, vasilii wrote:
> We should use ScopedVector here. 
> ContentCredentialManagerDispatcher::OnGetPasswordStoreResults contains a
memory
> leak because of this.
Do you mind if I add TODO? I think it's better to separate code changes this
from this CL.

[vasilii, 2015-01-15 12:16:58]

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:54:
const std::vector<autofill::PasswordForm*>& federated_forms,
On 2015/01/15 10:04:30, melandory wrote:
> On 2015/01/13 18:13:28, vasilii wrote:
> > We should use ScopedVector here. 
> > ContentCredentialManagerDispatcher::OnGetPasswordStoreResults contains a
> memory
> > leak because of this.
> Do you mind if I add TODO? I think it's better to separate code changes this
> from this CL.

Sure.

[melandory, 2015-01-16 12:53:20]

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

https://codereview.chromium.org/848723002/diff/200001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:124: //
TODO(melandory): fix the crash when |local_credentials| is empty.
On 2015/01/13 19:41:46, Mike West wrote:
> It's a slightly bigger TODO than that; we need the origin in order to display
> the correct title in the bubble (e.g. "Select a credential for
`example.com`").
> If we can't rely on having local credentials from which to extract the origin,
> we'll need to pass it in explicitly. That's probably the right thing to do
> anyway (in a separate CL).

Done.

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

https://codereview.chromium.org/848723002/diff/200001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:112:
ScopedVector<autofill::PasswordForm>& new_password_forms() {
On 2015/01/13 18:13:28, vasilii wrote:
> Is it still used?

Done.

https://codereview.chromium.org/848723002/diff/200001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/848723002/diff/200001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:212:
password_manager::CredentialType type) {
On 2015/01/13 18:13:28, vasilii wrote:
> It will be shorter to write a method taking a vector and type.

I've changed lambda code, now it has vector as an argument and iterates over it.
Do you think that function in anonym namespace is better?

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
File
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:57:
EXPECT_FALSE(local_forms.empty() && federated_forms.empty());
On 2015/01/13 18:13:29, vasilii wrote:
> The function crashes if local_forms.empty()

Done.

https://codereview.chromium.org/848723002/diff/200001/components/password_man...
components/password_manager/content/browser/content_credential_manager_dispatcher_unittest.cc:63:
// TODO(vasilii): Do something clever with |federated_forms|.
On 2015/01/13 18:13:29, vasilii wrote:
> TODO is irrelevant

Done.

[vasilii, 2015-01-16 14:07:27]

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

https://codereview.chromium.org/848723002/diff/220001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:21: enum class
CredentialType : unsigned int;
I wonder why the type is inherited from "unsigned int". Now we have to carry
this declaration throughout the codebase.

https://codereview.chromium.org/848723002/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/848723002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:210: auto
add_credential_items_with_state = [&layout, this](
Why don't you write a private method given that you capture |this|?
|layout| is a pointer. There is no reason to capture it by reference.

[melandory, 2015-01-19 15:40:15]

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

https://codereview.chromium.org/848723002/diff/220001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:21: enum class
CredentialType : unsigned int;
On 2015/01/16 14:07:26, vasilii wrote:
> I wonder why the type is inherited from "unsigned int". Now we have to carry
> this declaration throughout the codebase. 

Yes, we can amend unsigned int and live with default int. I'll add TODO. Because
a lot of code should be changed in order to make remove "unsigned int", see
https://codereview.chromium.org/835523004/

https://codereview.chromium.org/848723002/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/848723002/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:210: auto
add_credential_items_with_state = [&layout, this](
On 2015/01/16 14:07:26, vasilii wrote:
> Why don't you write a private method given that you capture |this|?
> |layout| is a pointer. There is no reason to capture it by reference.

I thought since this routine is used only by one method it's better to keep it
local. But I'll create private method.

[vasilii, 2015-01-21 17:47:35]

https://codereview.chromium.org/848723002/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/848723002/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:190: void
AddCredentialItemsWithState(
What is the meaning of State here?
With type probably.

https://codereview.chromium.org/848723002/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:191:
views::GridLayout* layout,
nit: I think you can extract it with GetLayoutManager().

[melandory, 2015-01-22 10:21:30]

Patchset #5 (id:260001) has been deleted

[melandory, 2015-01-22 10:21:49]

Patchset #5 (id:280001) has been deleted

[melandory, 2015-01-22 10:25:12]

Patchset #5 (id:300001) has been deleted

[melandory, 2015-01-22 10:47:34]

https://codereview.chromium.org/848723002/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/848723002/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:190: void
AddCredentialItemsWithState(
On 2015/01/21 17:47:35, vasilii wrote:
> What is the meaning of State here?
> With type probably.

Done.

https://codereview.chromium.org/848723002/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:191:
views::GridLayout* layout,
On 2015/01/21 17:47:35, vasilii wrote:
> nit: I think you can extract it with GetLayoutManager().

Then I need to cast it to GridLayout. I do not like it.

[vasilii, 2015-01-22 10:55:58]

lgtm

[Mike West, 2015-01-22 11:39:55]

LGTM % comments. Thanks!

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

https://codereview.chromium.org/848723002/diff/320001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:350:
EXPECT_EQ(password_manager::CredentialType::CREDENTIAL_TYPE_LOCAL,
Can you add a similar test here which ends up with a FEDERATED credential?

https://codereview.chromium.org/848723002/diff/320001/components/password_man...
File components/password_manager/content/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/848723002/diff/320001/components/password_man...
components/password_manager/content/common/credential_manager_types.cc:44:
DCHECK(!password.empty() || !federation.is_empty());
Let's be a bit cautious here: if the type is federated, I'd like us to clear the
password value. If the type is local, I'd like us to clear the federation value.

[vasilii, 2015-01-22 11:46:23]

https://codereview.chromium.org/848723002/diff/320001/components/password_man...
File components/password_manager/content/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/848723002/diff/320001/components/password_man...
components/password_manager/content/common/credential_manager_types.cc:44:
DCHECK(!password.empty() || !federation.is_empty());
On 2015/01/22 11:39:54, Mike West wrote:
> Let's be a bit cautious here: if the type is federated, I'd like us to clear
the
> password value. If the type is local, I'd like us to clear the federation
value.

The same applicable to CREDENTIAL_TYPE_EMPTY.

[melandory, 2015-01-23 10:24:55]

Patchset #6 (id:340001) has been deleted

[melandory, 2015-01-23 15:06:07]

Patchset #6 (id:360001) has been deleted

[melandory, 2015-01-23 15:06:15]

Patchset #6 (id:380001) has been deleted

[melandory, 2015-01-23 15:07:43]

Hi Mike,

I've addressed comments. Can you check that I get it right.

Thanks!

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

https://codereview.chromium.org/848723002/diff/320001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:350:
EXPECT_EQ(password_manager::CredentialType::CREDENTIAL_TYPE_LOCAL,
On 2015/01/22 11:39:54, Mike West wrote:
> Can you add a similar test here which ends up with a FEDERATED credential?

Done.

https://codereview.chromium.org/848723002/diff/320001/components/password_man...
File components/password_manager/content/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/848723002/diff/320001/components/password_man...
components/password_manager/content/common/credential_manager_types.cc:44:
DCHECK(!password.empty() || !federation.is_empty());
On 2015/01/22 11:46:23, vasilii wrote:
> On 2015/01/22 11:39:54, Mike West wrote:
> > Let's be a bit cautious here: if the type is federated, I'd like us to clear
> the
> > password value. If the type is local, I'd like us to clear the federation
> value.
> 
> The same applicable to CREDENTIAL_TYPE_EMPTY.

Done.

[vasilii, 2015-01-23 15:24:18]

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

https://codereview.chromium.org/848723002/diff/400001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:357:
local_credentials.push_back(new autofill::PasswordForm(test_form()));
That's strange that you add test_form() here as a local credential. Later it
suddenly becomes selected as a federated one and looses the password.
The form should go to |federated_credentials|.

[melandory, 2015-01-23 15:32:01]

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

https://codereview.chromium.org/848723002/diff/400001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:357:
local_credentials.push_back(new autofill::PasswordForm(test_form()));
On 2015/01/23 15:24:18, vasilii wrote:
> That's strange that you add test_form() here as a local credential. Later it
> suddenly becomes selected as a federated one and looses the password.
> The form should go to |federated_credentials|.

Fixed.

[vasilii, 2015-01-23 17:04:38]

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

https://codereview.chromium.org/848723002/diff/420001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:129: origin_ =
federated_credentials[0]->origin;
Well, the origin of a federated login isn't what we mean by origin here. That's
why we need to pass it explicitly.
If we suddenly decide to use it in UI, then with the current implementation the
bubble will say "Choose an account for facebook.com" while the user is logging
in to example.com.

[melandory, 2015-01-23 18:16:04]

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

https://codereview.chromium.org/848723002/diff/420001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:129: origin_ =
federated_credentials[0]->origin;
On 2015/01/23 17:04:37, vasilii wrote:
> Well, the origin of a federated login isn't what we mean by origin here.
That's
> why we need to pass it explicitly.
> If we suddenly decide to use it in UI, then with the current implementation
the
> bubble will say "Choose an account for facebook.com" while the user is logging
> in to http://example.com.

Yes, I see now. I'll set empty value for it right now and address todo in follow
up cl.

[melandory, 2015-01-23 18:17:24]

Patchset #8 (id:440001) has been deleted

[melandory, 2015-01-23 20:33:44]

Patchset #8 (id:440002) has been deleted

[melandory, 2015-01-23 20:33:56]

Patchset #8 (id:470001) has been deleted

[melandory, 2015-01-26 08:52:47]

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

https://codereview.chromium.org/848723002/diff/420001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:129: origin_ =
federated_credentials[0]->origin;
On 2015/01/23 18:16:04, melandory wrote:
> On 2015/01/23 17:04:37, vasilii wrote:
> > Well, the origin of a federated login isn't what we mean by origin here.
> That's
> > why we need to pass it explicitly.
> > If we suddenly decide to use it in UI, then with the current implementation
> the
> > bubble will say "Choose an account for facebook.com" while the user is
logging
> > in to http://example.com.
> 
> Yes, I see now. I'll set empty value for it right now and address todo in
follow
> up cl.

https://codereview.chromium.org/877613004

[vasilii, 2015-01-26 09:03:44]

lgtm

[melandory, 2015-01-26 09:15:53]

The CQ bit was checked by melandory@chromium.org

[commit-bot: I haz the power, 2015-01-26 09:16:05]

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

[commit-bot: I haz the power, 2015-01-26 10:03:20]

Committed patchset #8 (id:490001)

[commit-bot: I haz the power, 2015-01-26 10:04:17]

Patchset 8 (id:??) landed as
https://crrev.com/27ce3350383649c74870c9417b9e64b6f4194a7b
Cr-Commit-Position: refs/heads/master@{#313053}