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

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

The bubble is never popped up at the moment. The CL only implements the UI elements.

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

[vasilii, 2016-06-02 12:43:08]

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

[vasilii, 2016-06-02 12:43:08]

Hi Vaclav,

please review the CL. The screenshot is in the bug tracker.

[vabr (Chromium), 2016-06-02 13:10:55]

Thanks, Vasilii.

This LGTM, but I have some comments and questions.

Cheers,
Vaclav

https://codereview.chromium.org/2033713002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2033713002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6020: +        <message
name="IDS_PASSWORD_MANAGER_NO_THANKS" desc="In Title Case: The title of the No
Thanks button in the sign-in promo bubble.">
(Here and below.)
"the sign-in promo bubble" might not be clear enough for the translators. What
about "the bubble promoting Chrome sign-in"?

https://codereview.chromium.org/2033713002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6020: +        <message
name="IDS_PASSWORD_MANAGER_NO_THANKS" desc="In Title Case: The title of the No
Thanks button in the sign-in promo bubble.">
It is a bit surprising that the name sounds generic (just "password manager" "no
thanks"), while the description is about the specific case of the sign-in promo.
We should decide what is better here and make the name and description
consistent.

https://codereview.chromium.org/2033713002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6032: +      <message
name="IDS_PASSWORD_MANAGER_SIGNIN_PROMO_TITLE" desc="The title of the Chrome
sign-in promo appearing in the password bubble.">
It's interestingly long for a title. I understand that it might be a title from
the UI library point of view, but for the humans it looks more like a content.
To ensure the translators are not confused, what about this description?
desc="Promotion text for Chrome sign-in appearing in the password bubble after a
password is saved."

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

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:428: if (false /*
there will be a real condition soon*/) {
nit: Please prefix the comment with TODO(crbug.com/615825).

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:76: // Called by the
view when the "Sign in" button in the promo bubble clicked.
nit: clicked -> is clicked

(Also below.)

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:115: bool
ShouldShowSignInPromo();
The method has side-effects which are not mentioned in the comment, and are not
obvious from the method name. What about:

bool UpdateToShowSignInPromoIfAppropriate()

or something similar?

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

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:584: // Reuse
the string because it has the same meaning.
Are you sure the meaning is the same? Here it means signing into Chrome, in the
account chooser it means signing into a page.

The verbs for that are the same in English, German, Czech and Slovak, at least,
so perhaps it is fine. But then we should drop the "ACCOUNT_CHOOSER" in the
string name and make sure the strings description does not mention which kind of
sign-in is meant.

Or we should stay on the safe side and have two strings for the two different
concepts.

But I'm a bit worried about the current mix. We had issues in the past with
people confusing content-area sign-in with Chrome sign-in, and it had scary
consequences (mixing sync accounts etc.).

[vasilii, 2016-06-02 13:36:18]

https://codereview.chromium.org/2033713002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2033713002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6020: +        <message
name="IDS_PASSWORD_MANAGER_NO_THANKS" desc="In Title Case: The title of the No
Thanks button in the sign-in promo bubble.">
On 2016/06/02 13:10:55, vabr (Chromium) wrote:
> It is a bit surprising that the name sounds generic (just "password manager"
"no
> thanks"), while the description is about the specific case of the sign-in
promo.
> We should decide what is better here and make the name and description
> consistent.

Done.

https://codereview.chromium.org/2033713002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6020: +        <message
name="IDS_PASSWORD_MANAGER_NO_THANKS" desc="In Title Case: The title of the No
Thanks button in the sign-in promo bubble.">
On 2016/06/02 13:10:55, vabr (Chromium) wrote:
> (Here and below.)
> "the sign-in promo bubble" might not be clear enough for the translators. What
> about "the bubble promoting Chrome sign-in"?

Done.

https://codereview.chromium.org/2033713002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6032: +      <message
name="IDS_PASSWORD_MANAGER_SIGNIN_PROMO_TITLE" desc="The title of the Chrome
sign-in promo appearing in the password bubble.">
On 2016/06/02 13:10:55, vabr (Chromium) wrote:
> It's interestingly long for a title. I understand that it might be a title
from
> the UI library point of view, but for the humans it looks more like a content.
> To ensure the translators are not confused, what about this description?
> desc="Promotion text for Chrome sign-in appearing in the password bubble after
a
> password is saved."

Done.

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

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:428: if (false /*
there will be a real condition soon*/) {
On 2016/06/02 13:10:55, vabr (Chromium) wrote:
> nit: Please prefix the comment with TODO(crbug.com/615825).

Done.

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:76: // Called by the
view when the "Sign in" button in the promo bubble clicked.
On 2016/06/02 13:10:55, vabr (Chromium) wrote:
> nit: clicked -> is clicked
> 
> (Also below.)

Done.

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:115: bool
ShouldShowSignInPromo();
On 2016/06/02 13:10:55, vabr (Chromium) wrote:
> The method has side-effects which are not mentioned in the comment, and are
not
> obvious from the method name. What about:
> 
> bool UpdateToShowSignInPromoIfAppropriate()
> 
> or something similar?

Done.

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

https://codereview.chromium.org/2033713002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:584: // Reuse
the string because it has the same meaning.
On 2016/06/02 13:10:55, vabr (Chromium) wrote:
> Are you sure the meaning is the same? Here it means signing into Chrome, in
the
> account chooser it means signing into a page.
> 
> The verbs for that are the same in English, German, Czech and Slovak, at
least,
> so perhaps it is fine. But then we should drop the "ACCOUNT_CHOOSER" in the
> string name and make sure the strings description does not mention which kind
of
> sign-in is meant.
> 
> Or we should stay on the safe side and have two strings for the two different
> concepts.
> 
> But I'm a bit worried about the current mix. We had issues in the past with
> people confusing content-area sign-in with Chrome sign-in, and it had scary
> consequences (mixing sync accounts etc.).

I split the strings.

[vabr (Chromium), 2016-06-02 14:16:40]

LGTM!

[vasilii, 2016-06-02 14:20:31]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2016-06-02 14:21:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033713002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2033713002/40001

[commit-bot: I haz the power, 2016-06-02 14:38:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-02 14:40:27]

Description was changed from

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

The bubble is never popped up at the moment. The CL only implements the UI
elements.

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

to

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

The bubble is never popped up at the moment. The CL only implements the UI
elements.

BUG=615825

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

[commit-bot: I haz the power, 2016-06-02 14:40:28]

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