Implement the UI logic behind the Sign In promo in the password bubble

Implement the UI logic behind the Sign In promo in the password bubble.

BUG=615825
Committed: https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749
Cr-Commit-Position: refs/heads/master@{#398050}

[vasilii, 2016-06-03 17:29:25]

Description was changed from

==========
pretty


format


Implement the UI logic behind the Sign In promo in the password bubble.


comments


fix model


Implement the Chrome Sign-in promo bubble for password manager on Views.

BUG=
==========

to

==========
Implement the UI logic behind the Sign In promo in the password bubble.

BUG=615825
==========

[vasilii, 2016-06-03 17:29:25]

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

[vasilii, 2016-06-03 17:30:30]

Hi Ilya,

please review histograms.xml

[Ilya Sherman, 2016-06-03 21:21:25]

Histograms LGTM % comments inline.

https://codereview.chromium.org/2036323002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/2036323002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_metrics_util.h:133:
enum ChromeSignInUserAction {
nit: What is the "Chrome" part of this enum name referring to?  Is it disabled
in Chromium builds?

https://codereview.chromium.org/2036323002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2036323002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:37407: +    User action on the Chrome
Sign in promo that appears in the password bubble.
nit: Either "sign in" or "Sign In"

https://codereview.chromium.org/2036323002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:37407: +    User action on the Chrome
Sign in promo that appears in the password bubble.
It would be nice to go into a bit more detail on what promo this is exactly.  I
can't quite tell from this description; I can guess, but I'd prefer not to have
to guess.

[vasilii, 2016-06-06 15:08:46]

https://codereview.chromium.org/2036323002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/2036323002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_metrics_util.h:133:
enum ChromeSignInUserAction {
On 2016/06/03 21:21:24, Ilya Sherman wrote:
> nit: What is the "Chrome" part of this enum name referring to?  Is it disabled
> in Chromium builds?

I think SyncSignIn is better. It has nothing to do with Chrome vs. Chromium. It
is sign in for syncing.

https://codereview.chromium.org/2036323002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2036323002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:37407: +    User action on the Chrome
Sign in promo that appears in the password bubble.
On 2016/06/03 21:21:25, Ilya Sherman wrote:
> It would be nice to go into a bit more detail on what promo this is exactly. 
I
> can't quite tell from this description; I can guess, but I'd prefer not to
have
> to guess.

Done.

https://codereview.chromium.org/2036323002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:37407: +    User action on the Chrome
Sign in promo that appears in the password bubble.
On 2016/06/03 21:21:25, Ilya Sherman wrote:
> nit: Either "sign in" or "Sign In"

Done.

[vasilii, 2016-06-06 15:09:28]

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

[vasilii, 2016-06-06 15:09:28]

Hi Vaclav,

please review.

[vabr (Chromium), 2016-06-06 15:23:14]

LGTM!

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

https://codereview.chromium.org/2036323002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:176: //
web_contents() can't be nullptr in this block.
nit: The standard way to say this is:
DCHECK(web_contents());

https://codereview.chromium.org/2036323002/diff/40001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc (left):

https://codereview.chromium.org/2036323002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Optional: unless this removal is related to the rest of the CL, should we split
it into a separate one?

https://codereview.chromium.org/2036323002/diff/40001/components/password_man...
File
components/password_manager/core/browser/password_bubble_experiment_unittest.cc
(right):

https://codereview.chromium.org/2036323002/diff/40001/components/password_man...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:117:
variations::testing::ClearAllVariationIDs();
Would this work in a destructor instead?

(See
https://github.com/google/googletest/blob/master/googletest/docs/V1_7_FAQ.md#...
and
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/J2K-2Dmupb8...)

[vasilii, 2016-06-06 15:45:15]

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

https://codereview.chromium.org/2036323002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:176: //
web_contents() can't be nullptr in this block.
On 2016/06/06 15:23:14, vabr (Chromium) wrote:
> nit: The standard way to say this is:
> DCHECK(web_contents());

Done.

https://codereview.chromium.org/2036323002/diff/40001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc (left):

https://codereview.chromium.org/2036323002/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2016/06/06 15:23:13, vabr (Chromium) wrote:
> Optional: unless this removal is related to the rest of the CL, should we
split
> it into a separate one?

Done.

https://codereview.chromium.org/2036323002/diff/40001/components/password_man...
File
components/password_manager/core/browser/password_bubble_experiment_unittest.cc
(right):

https://codereview.chromium.org/2036323002/diff/40001/components/password_man...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:117:
variations::testing::ClearAllVariationIDs();
On 2016/06/06 15:23:14, vabr (Chromium) wrote:
> Would this work in a destructor instead?
> 
> (See
>
https://github.com/google/googletest/blob/master/googletest/docs/V1_7_FAQ.md#...
> and
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/J2K-2Dmupb8...)

Done.

[vasilii, 2016-06-06 15:48:02]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-06-06 15:48:03]

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

[commit-bot: I haz the power, 2016-06-06 15:48:15]

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

[commit-bot: I haz the power, 2016-06-06 16:56:34]

Description was changed from

==========
Implement the UI logic behind the Sign In promo in the password bubble.

BUG=615825
==========

to

==========
Implement the UI logic behind the Sign In promo in the password bubble.

BUG=615825
==========

[commit-bot: I haz the power, 2016-06-06 16:56:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-06 16:59:14]

Description was changed from

==========
Implement the UI logic behind the Sign In promo in the password bubble.

BUG=615825
==========

to

==========
Implement the UI logic behind the Sign In promo in the password bubble.

BUG=615825

Committed: https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749
Cr-Commit-Position: refs/heads/master@{#398050}
==========

[commit-bot: I haz the power, 2016-06-06 16:59:15]

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