Credential Manager API pops up an auto-signin toast.

Credential Manager API pops up an auto-signin toast.

BUG=400674
TBR=isherman@chromium.org

Committed: https://crrev.com/9a920cc17f7b4a4cc05818921a5e4b7783728479
Cr-Commit-Position: refs/heads/master@{#317022}

[vasilii, 2015-02-18 18:31:24]

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

[vasilii, 2015-02-18 18:31:25]

mkwst@chromium.org: Please review changes in 
*.*

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

[Mike West, 2015-02-18 22:28:05]

Windows bots are unhappy because VS doesn't like switch statements that don't
either have a `default:` specified, or are followed by a `return`. :(

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

https://codereview.chromium.org/924733003/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:13497: +        Signing in with Google Smart
Lock
This seems strange. Is there no string with the username?

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

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:186:
display_disposition_ = metrics_util::AUTOMATIC_AUTO_SIGNIN_TOAST;
Nit: AUTOMATIC_AUTO is a bit much. How about AUTOMATIC_SIGNIN_TOAST?

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:235: if (state_ ==
password_manager::ui::PENDING_PASSWORD_STATE &&
What about clicking away in MANAGE state?

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

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:398: return;
Why do we need this? Don't we just never call into the bubble if we're outside
the experiment?

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:271: //
ManagePasswordsBubbleView::AccountChooserView ------------------------------
Nit: AccountChooserView -> AutoSignInView

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:275: //
"Cancel" button.
Nit: Please update this comment as well.

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:303:
credential->SetEnabled(false);
// TODO?

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:312: }
// TODO?

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1028: } else
if (model()->state() == password_manager::ui::AUTO_SIGNIN_STATE) {
Did the previous CL change the state back to MANAGE? Or did we just talk about
doing that... I don't remember?

[vasilii, 2015-02-19 09:37:35]

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

https://codereview.chromium.org/924733003/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:13497: +        Signing in with Google Smart
Lock
On 2015/02/18 22:28:04, Mike West wrote:
> This seems strange. Is there no string with the username?

This is the string I picked from the mocks. But I agree with being proactive!

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

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:186:
display_disposition_ = metrics_util::AUTOMATIC_AUTO_SIGNIN_TOAST;
On 2015/02/18 22:28:04, Mike West wrote:
> Nit: AUTOMATIC_AUTO is a bit much. How about AUTOMATIC_SIGNIN_TOAST?

Done.

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:235: if (state_ ==
password_manager::ui::PENDING_PASSWORD_STATE &&
On 2015/02/18 22:28:04, Mike West wrote:
> What about clicking away in MANAGE state?

We care only about the "Save" bubble. The "Manage" bubble isn't automatic. User
may open it to check something and dismiss without clicking anything. It doesn't
mean that the bubble UI is annoying to him. Therefore, we don't want it to skew
the statistics.

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

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:398: return;
On 2015/02/18 22:28:04, Mike West wrote:
> Why do we need this? Don't we just never call into the bubble if we're outside
> the experiment?

That's another experiment of showing the "Save" bubble not too often.
This is the line I removed in https://codereview.chromium.org/928753003. I
shouldn't have done that.

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:271: //
ManagePasswordsBubbleView::AccountChooserView ------------------------------
On 2015/02/18 22:28:04, Mike West wrote:
> Nit: AccountChooserView -> AutoSignInView

Done.

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:275: //
"Cancel" button.
On 2015/02/18 22:28:04, Mike West wrote:
> Nit: Please update this comment as well.

Done.

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:303:
credential->SetEnabled(false);
On 2015/02/18 22:28:04, Mike West wrote:
> // TODO?

Done.

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:312: }
On 2015/02/18 22:28:04, Mike West wrote:
> // TODO?

Done.

https://codereview.chromium.org/924733003/diff/1/chrome/browser/ui/views/pass...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1028: } else
if (model()->state() == password_manager::ui::AUTO_SIGNIN_STATE) {
On 2015/02/18 22:28:04, Mike West wrote:
> Did the previous CL change the state back to MANAGE? Or did we just talk about
> doing that... I don't remember?

Just talk. In the previous CL I saved all the forms for the site. However,
currently we change back to the inactive state.

[Mike West, 2015-02-19 09:39:58]

LGTM with the TODOs we just discussed.

[vasilii, 2015-02-19 10:34:22]

I'm TBRing isherman@ for  tools/metrics/histograms/histograms.xml

[vasilii, 2015-02-19 10:34:31]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2015-02-19 10:35:25]

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

[commit-bot: I haz the power, 2015-02-19 10:38:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-19 10:39:27]

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

[Ilya Sherman, 2015-02-19 20:23:33]

histograms.xml lgtm.  Sorry, I somehow missed the original review request :/