Show user credentials chooser bubble.

Show user credentials chooser bubble.

The bubble is supposed to be triggered by the Credential Managment javascript API.

BUG=400674
Committed: https://crrev.com/571006301b4c7a24b07e2eaffc6ce0f36e216283
Cr-Commit-Position: refs/heads/master@{#305628}

[vasilii, 2014-11-19 19:25:51]

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

[vasilii, 2014-11-19 19:26:58]

Hi Mike,

please review.

[Mike West, 2014-11-19 19:56:37]

Looks great, thanks for putting this together so quickly! I'll read it more
detail tomorrow morning; a few nits for now.

https://codereview.chromium.org/733463003/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/733463003/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:13259: +      <message
name="IDS_MANAGE_PASSWORDS_CHOOSE_TITLE" desc="The title of the password bubble
when a credential should be choosen.">
Nit: s/choosen/chosen/

https://codereview.chromium.org/733463003/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/733463003/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.h:53: const
std::vector<autofill::PasswordForm*>& form_to_save,
I think you reversed the variable names here: this should be 'forms', the
previous should be 'form_to_save'.

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:111:
autofill::PasswordForm form;
Nit: Can you move the federations TODO comment up here somewhere?

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:127:
pending_request_id_)))
Nit: {}s around the next line, since the `if` condition is multi-line.

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:69: //
Returns true if the prompt is indeed displayed. In this case only
Nit: Might be clearer as "If the prompt is not displayed, returns false and does
not call |callback|".

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:71: // Note:
The implementation owns all PasswordForms in the vector.
Nit: s/owns/takes ownership of/

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
File components/password_manager/core/common/password_manager_ui.cc (right):

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/core/common/password_manager_ui.cc:31: return
PENDING_CREDENTIALS_AND_BUBBLE_STATE;
This doesn't seem right. Shouldn't it move to MANAGE_STATE?

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/core/common/password_manager_ui.h:41:
PENDING_CREDENTIALS_AND_BUBBLE_STATE,
Nit: Might be clearer as CREDENTIAL_REQUEST_STATE. The bubble is a side-effect
of the credential request, not a separate thing, as it is in the
PENDING_PASSWORD_AND_BUBBLE_STATE.

[Mike West, 2014-11-20 08:19:32]

More questions. :)

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:196: void
ManagePasswordsBubbleModel::OnChooseCredetials(
s/OnChooseCredetials/OnChooseCredentials/ (missing an 'n')

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:115: friend void
InitPasswordsModelFromController(
Why `friend`?

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:113: // The map is
useless because usernames may overlap.
TODO(vasilii)? :)

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:117: if
(bubble_shown_)
When wouldn't the bubble be shown?

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:118:
credentials_callback_ = callback;
We need to callback to Blink even if the user doesn't pick a credential,
otherwise the Promise will never resolve. Do you handle this case somewhere
else?

Also, don't we need to take ownership of the callback here?

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:182: state_ =
password_manager::ui::INACTIVE_STATE;
This seems wrong; if I have another window open, and I save a password there,
the bubble would close here. Why should those behaviors be tied together?

Also: we need to callback into Blink to resolve the Promise with an empty
credential when we close the bubble automagically. I don't see where that's done
here.

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (left):

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:156: bool
autofill_blocked_;
This was unused? Weird.

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:159: friend void
InitPasswordsModelFromController(
Why `friend`?

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.cc:53: void
ManagePasswordsUIControllerMock::ChooseCredential(
Do you not need mocks for setting credentials to be chosen from?

[vasilii, 2014-11-20 15:08:06]

https://codereview.chromium.org/733463003/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/733463003/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:13259: +      <message
name="IDS_MANAGE_PASSWORDS_CHOOSE_TITLE" desc="The title of the password bubble
when a credential should be choosen.">
On 2014/11/19 19:56:37, Mike West wrote:
> Nit: s/choosen/chosen/

Done.

https://codereview.chromium.org/733463003/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/733463003/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.h:53: const
std::vector<autofill::PasswordForm*>& form_to_save,
On 2014/11/19 19:56:37, Mike West wrote:
> I think you reversed the variable names here: this should be 'forms', the
> previous should be 'form_to_save'.

I also think so :-)

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:196: void
ManagePasswordsBubbleModel::OnChooseCredetials(
On 2014/11/20 08:19:31, Mike West wrote:
> s/OnChooseCredetials/OnChooseCredentials/ (missing an 'n')

Done.

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:115: friend void
InitPasswordsModelFromController(
On 2014/11/20 08:19:31, Mike West wrote:
> Why `friend`?

So ManagePasswordsUIController header doesn't depend on
ManagePasswordsBubbleModel.

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:113: // The map is
useless because usernames may overlap.
On 2014/11/20 08:19:32, Mike West wrote:
> TODO(vasilii)? :)

TODO what? It's useful for other states.

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:117: if
(bubble_shown_)
On 2014/11/20 08:19:32, Mike West wrote:
> When wouldn't the bubble be shown?

There is a condition in ShowBubbleWithoutUserInteraction() about
input_in_progress(). Another one in ManagePasswordsBubbleView::ShowBubble -
IsShowing().

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:118:
credentials_callback_ = callback;
On 2014/11/20 08:19:31, Mike West wrote:
> We need to callback to Blink even if the user doesn't pick a credential,
> otherwise the Promise will never resolve. Do you handle this case somewhere
> else?
> 
> Also, don't we need to take ownership of the callback here?

if we return false then this case is handled immediately in
content_credential_manager_dispatcher.cc and we don't need a callback. The
callback is copyable so there is no ownership.

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:182: state_ =
password_manager::ui::INACTIVE_STATE;
On 2014/11/20 08:19:31, Mike West wrote:
> This seems wrong; if I have another window open, and I save a password there,
> the bubble would close here. Why should those behaviors be tied together?
> 
> Also: we need to callback into Blink to resolve the Promise with an empty
> credential when we close the bubble automagically. I don't see where that's
done
> here.

You are right. I removed this code. What should we do? Repopulate the bubble?

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (left):

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:156: bool
autofill_blocked_;
On 2014/11/20 08:19:32, Mike West wrote:
> This was unused? Weird.

Yes.

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:159: friend void
InitPasswordsModelFromController(
On 2014/11/20 08:19:32, Mike West wrote:
> Why `friend`?

We steal ScopedVector<autofill::PasswordForm> from this class.

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.cc:53: void
ManagePasswordsUIControllerMock::ChooseCredential(
On 2014/11/20 08:19:32, Mike West wrote:
> Do you not need mocks for setting credentials to be chosen from?

For testing the model - no, because
ManagePasswordsUIController::OnChooseCredentials isn't called from the model.
For testing ManagePasswordsUIController I'm happy with the real method.

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:111:
autofill::PasswordForm form;
On 2014/11/19 19:56:37, Mike West wrote:
> Nit: Can you move the federations TODO comment up here somewhere?

Done.

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:127:
pending_request_id_)))
On 2014/11/19 19:56:37, Mike West wrote:
> Nit: {}s around the next line, since the `if` condition is multi-line.

Done.

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:69: //
Returns true if the prompt is indeed displayed. In this case only
On 2014/11/19 19:56:37, Mike West wrote:
> Nit: Might be clearer as "If the prompt is not displayed, returns false and
does
> not call |callback|".

Done.

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:71: // Note:
The implementation owns all PasswordForms in the vector.
On 2014/11/19 19:56:37, Mike West wrote:
> Nit: s/owns/takes ownership of/

Done.

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
File components/password_manager/core/common/password_manager_ui.cc (right):

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/core/common/password_manager_ui.cc:31: return
PENDING_CREDENTIALS_AND_BUBBLE_STATE;
On 2014/11/19 19:56:37, Mike West wrote:
> This doesn't seem right. Shouldn't it move to MANAGE_STATE?

Honestly, the code here has vague meaning to me. We change the state explicitly
in other places (see ManagePasswordsUIController::SavePassword for example).

We can move to MANAGE_STATE. But what passwords do we demonstrate there?

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/733463003/diff/1/components/password_manager/...
components/password_manager/core/common/password_manager_ui.h:41:
PENDING_CREDENTIALS_AND_BUBBLE_STATE,
On 2014/11/19 19:56:37, Mike West wrote:
> Nit: Might be clearer as CREDENTIAL_REQUEST_STATE. The bubble is a side-effect
> of the credential request, not a separate thing, as it is in the
> PENDING_PASSWORD_AND_BUBBLE_STATE.

Done.

[Mike West, 2014-11-21 11:21:12]

Thanks for taking another pass! Let's chat after lunch.

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:113: // The map is
useless because usernames may overlap.
On 2014/11/20 15:08:05, vasilii wrote:
> On 2014/11/20 08:19:32, Mike West wrote:
> > TODO(vasilii)? :)
> 
> TODO what? It's useful for other states.

Yeah, I misunderstood this comment. Sorry for the noise.

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:117: if
(bubble_shown_)
On 2014/11/20 15:08:05, vasilii wrote:
> On 2014/11/20 08:19:32, Mike West wrote:
> > When wouldn't the bubble be shown?
> 
> There is a condition in ShowBubbleWithoutUserInteraction() about
> input_in_progress(). Another one in ManagePasswordsBubbleView::ShowBubble -
> IsShowing().

Hrm. Ok. I hope that case will be impossible to reach for `request()`, but we'll
have to think about it in the future.

For example, if we auto-sign-in a user, we'll want to pop up a toast regardless
of whether they're typing in the omnibox or on the page.

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:118:
credentials_callback_ = callback;
On 2014/11/20 15:08:05, vasilii wrote:
> On 2014/11/20 08:19:31, Mike West wrote:
> > We need to callback to Blink even if the user doesn't pick a credential,
> > otherwise the Promise will never resolve. Do you handle this case somewhere
> > else?
> > 
> > Also, don't we need to take ownership of the callback here?
> 
> if we return false then this case is handled immediately in
> content_credential_manager_dispatcher.cc and we don't need a callback. The
> callback is copyable so there is no ownership.

Acknowledged.

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:182: state_ =
password_manager::ui::INACTIVE_STATE;
On 2014/11/20 15:08:05, vasilii wrote:
> On 2014/11/20 08:19:31, Mike West wrote:
> > This seems wrong; if I have another window open, and I save a password
there,
> > the bubble would close here. Why should those behaviors be tied together?
> > 
> > Also: we need to callback into Blink to resolve the Promise with an empty
> > credential when we close the bubble automagically. I don't see where that's
> done
> > here.
> 
> You are right. I removed this code. What should we do? Repopulate the bubble?

I'd just add a TODO for now; I don't think we have a good answer. One option
would be to refresh the bubble, another would just be to pretend that nothing
happened. Since the credential is only returned to the page if the user selects
it, I'm not terribly worried about user confusion for the moment.

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

https://codereview.chromium.org/733463003/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:225:
ManagePasswordsBubbleModel* model) {
I think I'd prefer to see this live either on the controller or the model, and
to manipulate the other via accessors rather than friending and reaching into
both object's internal state.

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

https://codereview.chromium.org/733463003/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.cc:40:
OnBubbleShown();
Hrm. I'm surprised this doesn't change behavior in some way in the other tests.
Why do we need to pretend we showed the bubble every time we update the
visibility? Doesn't the actual method also close the bubble, or just leave it
alone, depending on the state?

https://codereview.chromium.org/733463003/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/733463003/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager_metrics_util.h:32:
AUTOMATIC_WITH_CREDENTIALS_PENDING,
Maybe "AUTOMATIC_CREDENTIAL_REQUEST" would be clearer.

https://codereview.chromium.org/733463003/diff/40001/components/password_mana...
File components/password_manager/core/common/password_manager_ui.cc (right):

https://codereview.chromium.org/733463003/diff/40001/components/password_mana...
components/password_manager/core/common/password_manager_ui.cc:31: return
CREDENTIAL_REQUEST_STATE;
We should talk about this, because it doesn't make any sense as written. This is
probably the existing code's fault, as I guess it doesn't support your use case,
but we'll need to change something here before landing it.

[vasilii, 2014-11-21 16:45:38]

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

https://codereview.chromium.org/733463003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:182: state_ =
password_manager::ui::INACTIVE_STATE;
On 2014/11/21 11:21:12, Mike West wrote:
> On 2014/11/20 15:08:05, vasilii wrote:
> > On 2014/11/20 08:19:31, Mike West wrote:
> > > This seems wrong; if I have another window open, and I save a password
> there,
> > > the bubble would close here. Why should those behaviors be tied together?
> > > 
> > > Also: we need to callback into Blink to resolve the Promise with an empty
> > > credential when we close the bubble automagically. I don't see where
that's
> > done
> > > here.
> > 
> > You are right. I removed this code. What should we do? Repopulate the
bubble?
> 
> I'd just add a TODO for now; I don't think we have a good answer. One option
> would be to refresh the bubble, another would just be to pretend that nothing
> happened. Since the credential is only returned to the page if the user
selects
> it, I'm not terribly worried about user confusion for the moment.

Done.

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

https://codereview.chromium.org/733463003/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:225:
ManagePasswordsBubbleModel* model) {
On 2014/11/21 11:21:12, Mike West wrote:
> I think I'd prefer to see this live either on the controller or the model, and
> to manipulate the other via accessors rather than friending and reaching into
> both object's internal state.

Done.

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

https://codereview.chromium.org/733463003/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.cc:40:
OnBubbleShown();
On 2014/11/21 11:21:12, Mike West wrote:
> Hrm. I'm surprised this doesn't change behavior in some way in the other
tests.
> Why do we need to pretend we showed the bubble every time we update the
> visibility? Doesn't the actual method also close the bubble, or just leave it
> alone, depending on the state?

I call ManagePasswordsUIController::OnChooseCredentials in the tests. It returns
true only if the bubble was shown in UpdateBubbleAndIconVisibility().

https://codereview.chromium.org/733463003/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/733463003/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager_metrics_util.h:32:
AUTOMATIC_WITH_CREDENTIALS_PENDING,
On 2014/11/21 11:21:12, Mike West wrote:
> Maybe "AUTOMATIC_CREDENTIAL_REQUEST" would be clearer.

Done.

https://codereview.chromium.org/733463003/diff/40001/components/password_mana...
File components/password_manager/core/common/password_manager_ui.cc (right):

https://codereview.chromium.org/733463003/diff/40001/components/password_mana...
components/password_manager/core/common/password_manager_ui.cc:31: return
CREDENTIAL_REQUEST_STATE;
On 2014/11/21 11:21:12, Mike West wrote:
> We should talk about this, because it doesn't make any sense as written. This
is
> probably the existing code's fault, as I guess it doesn't support your use
case,
> but we'll need to change something here before landing it.

I introduced the second state.

[Mike West, 2014-11-21 16:50:19]

LGTM.

Not sure it's worth introducing the IsCredentialsState method since you only use
it in one place, but it's up to you. :)

Thanks for going a few rounds on this!

[vasilii, 2014-11-21 17:15:07]

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

[vasilii, 2014-11-21 17:15:29]

On 2014/11/21 16:50:19, Mike West wrote:
> LGTM.
> 
> Not sure it's worth introducing the IsCredentialsState method since you only
use
> it in one place, but it's up to you. :)
> 
> Thanks for going a few rounds on this!

That's why everything stopped working after my changes!

Rachel, please review
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller_unittest.mm
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm

[groby-ooo-7-16, 2014-11-21 19:15:13]

c/b/ui/cocoa RSLGTM

[groby-ooo-7-16, 2014-11-21 19:15:41]

I suppose codereview doesn't like RS any more. So - rubber stamp LGTM

[vasilii, 2014-11-25 11:39:58]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-11-25 11:40:41]

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

[commit-bot: I haz the power, 2014-11-25 11:44:13]

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

[commit-bot: I haz the power, 2014-11-25 11:44:14]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[vasilii, 2014-11-25 12:21:51]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-11-25 12:22:15]

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

[commit-bot: I haz the power, 2014-11-25 13:08:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-25 13:09:38]

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