[CM API] Account Chooser with Sign in button for single account

[CM API] Account Chooser with Sign in button for single account

BUG=612504
Committed: https://crrev.com/ae78f7fefdeea8deb1a14a98626909628da6a36b
Cr-Commit-Position: refs/heads/master@{#404350}

[melandory, 2016-07-07 21:06:07]

melandory@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[melandory, 2016-07-07 21:06:08]

PTAL, screenshot is in the bug.

Thanks!

[gone, 2016-07-07 21:30:42]

Fundamentally lgtm, but please wait for Hwi on the button coloring question in
the bug.

https://codereview.chromium.org/2126353003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java
(right):

https://codereview.chromium.org/2126353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:175:
if (!mSigninButtonText.isEmpty()) builder.setPositiveButton(mSigninButtonText,
this);
Defensively use !TextUtils.isEmpty(mSigninButtonText) to avoid problems with
mSigninButtonText being null.

https://codereview.chromium.org/2126353003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/account_chooser_dialog_android_unittest.cc
(right):

https://codereview.chromium.org/2126353003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/account_chooser_dialog_android_unittest.cc:123:
histogram_tester.ExpectUniqueSample(
Should this test also check that ACCOUNT_CHOOSER_CREDENTIAL_CHOSEN is 0 (and the
above test check the opposite)?

https://codereview.chromium.org/2126353003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/account_chooser_dialog_android_unittest.cc:149:
password_manager::metrics_util::ACCOUNT_CHOOSER_CREDENTIAL_CHOSEN, 1);
Should this also check that SIGN_IN is 0?

[gone, 2016-07-07 21:32:48]

Hwi clarified, so you're good to go.

[melandory, 2016-07-08 08:02:19]

https://codereview.chromium.org/2126353003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java
(right):

https://codereview.chromium.org/2126353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:175:
if (!mSigninButtonText.isEmpty()) builder.setPositiveButton(mSigninButtonText,
this);
On 2016/07/07 21:30:42, dfalcantara wrote:
> Defensively use !TextUtils.isEmpty(mSigninButtonText) to avoid problems with
> mSigninButtonText being null.

Done.

https://codereview.chromium.org/2126353003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/account_chooser_dialog_android_unittest.cc
(right):

https://codereview.chromium.org/2126353003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/account_chooser_dialog_android_unittest.cc:123:
histogram_tester.ExpectUniqueSample(
On 2016/07/07 21:30:42, dfalcantara wrote:
> Should this test also check that ACCOUNT_CHOOSER_CREDENTIAL_CHOSEN is 0 (and
the
> above test check the opposite)?

Done.

https://codereview.chromium.org/2126353003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/account_chooser_dialog_android_unittest.cc:149:
password_manager::metrics_util::ACCOUNT_CHOOSER_CREDENTIAL_CHOSEN, 1);
On 2016/07/07 21:30:42, dfalcantara wrote:
> Should this also check that SIGN_IN is 0?

Done.

[melandory, 2016-07-08 09:15:40]

Patchset #2 (id:20001) has been deleted

[melandory, 2016-07-08 09:15:47]

Patchset #2 (id:40001) has been deleted

[melandory, 2016-07-08 09:15:57]

https://codereview.chromium.org/2126353003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/account_chooser_dialog_android_unittest.cc
(right):

https://codereview.chromium.org/2126353003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/account_chooser_dialog_android_unittest.cc:123:
histogram_tester.ExpectUniqueSample(
On 2016/07/08 08:02:19, melandory wrote:
> On 2016/07/07 21:30:42, dfalcantara wrote:
> > Should this test also check that ACCOUNT_CHOOSER_CREDENTIAL_CHOSEN is 0 (and
> the
> > above test check the opposite)?
> 
> Done.

Actually, re-read comment for ExpectUniqueSample, I think checking
ACCOUNT_CHOOSER_CREDENTIAL_CHOSEN not needed:
"// We know the exact number of samples in a bucket, and that no other bucket
  // should have samples. Measures the diff from the snapshot taken when this
  // object was constructed"

[melandory, 2016-07-08 09:16:18]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 09:17:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 09:47:44]

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

[commit-bot: I haz the power, 2016-07-08 09:47:45]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-07-08 09:48:37]

The CQ bit was checked by melandory@chromium.org

[melandory, 2016-07-08 09:48:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2126353003/#ps60001
(title: "remove unnssary checks")

[commit-bot: I haz the power, 2016-07-08 09:49:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 14:06:07]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-07-08 14:06:26]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 14:08:36]

Description was changed from

==========
[CM API] Account Chooser with Sign in button for single account

BUG=612504
==========

to

==========
[CM API] Account Chooser with Sign in button for single account

BUG=612504

Committed: https://crrev.com/ae78f7fefdeea8deb1a14a98626909628da6a36b
Cr-Commit-Position: refs/heads/master@{#404350}
==========

[commit-bot: I haz the power, 2016-07-08 14:08:37]

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