New issue
Advanced search Search tips

Issue 615825 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Implement Sign in promo in save password bubble

Project Member Reported by vasi...@chromium.org, May 30 2016

Issue description

Comment 1 by ew...@chromium.org, May 31 2016

Note we're going without the "Smart lock" branding for now.

Thanks Vasilii!
The "Smart branding" in the UI is controlled globally by our team. You are right that currently it's disabled.
Linux
Screenshot from 2016-06-02 14:06:36.png
13.3 KB View Download
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749

commit 8f82c72fbf4be9c5e4bd08999c17bc13d54c8749
Author: vasilii <vasilii@chromium.org>
Date: Mon Jun 06 16:56:18 2016

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

BUG= 615825 

Review-Url: https://codereview.chromium.org/2036323002
Cr-Commit-Position: refs/heads/master@{#398050}

[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/chrome/browser/ui/passwords/manage_passwords_bubble_model.cc
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/chrome/browser/ui/passwords/manage_passwords_ui_controller.h
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/chrome/browser/ui/passwords/passwords_model_delegate.h
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/components/password_manager/core/browser/password_bubble_experiment.cc
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/components/password_manager/core/browser/password_bubble_experiment.h
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/components/password_manager/core/browser/password_bubble_experiment_unittest.cc
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/components/password_manager/core/browser/password_manager_metrics_util.cc
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/components/password_manager/core/browser/password_manager_metrics_util.h
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/components/password_manager/core/common/password_manager_pref_names.cc
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/components/password_manager/core/common/password_manager_pref_names.h
[modify] https://crrev.com/8f82c72fbf4be9c5e4bd08999c17bc13d54c8749/tools/metrics/histograms/histograms.xml

Mac
Screen Shot 2016-06-09 at 20.01.29.png
15.4 KB View Download
Looking great, thanks Vasilii! Let me know when it's available for me to test via a flag :)
It's already on Windows. But I didn't introduce a flag, just via Finch. The config isn
't landed, therefore, we can already decide which % of which channel we are targeting.
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73dc7a161bcc1525c3fea37ef9991de7e3331664

commit 73dc7a161bcc1525c3fea37ef9991de7e3331664
Author: vasilii <vasilii@chromium.org>
Date: Fri Jun 10 14:39:37 2016

Implement the Sync promo in the password bubble on Mac.

BUG= 615825 

Review-Url: https://codereview.chromium.org/2040143006
Cr-Commit-Position: refs/heads/master@{#399179}

[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/base_passwords_content_view_controller.h
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/base_passwords_content_view_controller.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/confirmation_password_saved_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/manage_passwords_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/passwords_bubble_controller.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/passwords_list_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller_unittest.mm
[add] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/signin_promo_view_controller.h
[add] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/signin_promo_view_controller.mm
[add] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/signin_promo_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/chrome_browser_ui.gypi
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/chrome_tests_unit.gypi

Now it's on Mac too. It's controlled by Finch.
Test the feature by launching Chrome with parameters

chrome.exe --force-fieldtrials=PasswordSmartBubble/SomeGroup  --force-fieldtrial-params=PasswordSmartBubble.SomeGroup:dismissal_count/2

Project Member

Comment 16 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73dc7a161bcc1525c3fea37ef9991de7e3331664

commit 73dc7a161bcc1525c3fea37ef9991de7e3331664
Author: vasilii <vasilii@chromium.org>
Date: Fri Jun 10 14:39:37 2016

Implement the Sync promo in the password bubble on Mac.

BUG= 615825 

Review-Url: https://codereview.chromium.org/2040143006
Cr-Commit-Position: refs/heads/master@{#399179}

[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/base_passwords_content_view_controller.h
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/base_passwords_content_view_controller.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/confirmation_password_saved_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/manage_passwords_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/passwords_bubble_controller.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/passwords_list_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller_unittest.mm
[add] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/signin_promo_view_controller.h
[add] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/signin_promo_view_controller.mm
[add] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/signin_promo_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/chrome_browser_ui.gypi
[modify] https://crrev.com/73dc7a161bcc1525c3fea37ef9991de7e3331664/chrome/chrome_tests_unit.gypi

Comment 17 by ew...@chromium.org, Jun 21 2016

Hey Vasilii, what's the easiest way to test this on Mac? Is this ready for cross-functional reviews (e.g. UI, accessibility, etc.) yet?
Test the feature by launching Chrome with parameters

chrome.exe --force-fieldtrials=PasswordSmartBubble/SomeGroup  --force-fieldtrial-params=PasswordSmartBubble.SomeGroup:dismissal_count/2

"2" here is the maximum number we show the prompt.
It's ready, I'm eager to fix the bugs they find.

Comment 19 by ew...@chromium.org, Jun 22 2016

Is there an equivalent way to test it on Mac?
It's on any OS.

Comment 21 by ew...@chromium.org, Jun 22 2016

Sorry, I was getting tripped up with "chrome.exe," which obviously doesn't exist on Mac. I figured out it's just /path/to/Google Chrome Canary.app/Contents/MacOS/Google Chrome Canary though.

I tried running that ("./Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --force-fieldtrials=PasswordSmartBubble/SomeGroup --force-fieldtrial-params=PasswordSmartBubble.SomeGroup:dismissal_count/50"), and it's not triggering the promo for me when I save a password. Is there any other setup I need to do for it to work?
Because I copy-pasted the string from another feature :-) This is what you should type

'/Applications/Google Chrome Canary.app/Contents/MacOS/Google Chrome Canary' --force-fieldtrials=SignInPasswordPromo/SomeGroup --force-fieldtrial-params=SignInPasswordPromo.SomeGroup:dismissal_threshold/2

Comment 23 by ew...@chromium.org, Jun 27 2016

Got it, thanks Vasilii!
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0b53a62d7be6089bb030227fd90fc28efc18b83

commit d0b53a62d7be6089bb030227fd90fc28efc18b83
Author: vasilii <vasilii@chromium.org>
Date: Tue Jun 28 16:38:38 2016

Add 'x' button to the signin promo in the password bubble.

BUG= 615825 

Review-Url: https://codereview.chromium.org/2109673002
Cr-Commit-Position: refs/heads/master@{#402489}

[modify] https://crrev.com/d0b53a62d7be6089bb030227fd90fc28efc18b83/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc

Comment 25 by ew...@chromium.org, Jun 28 2016

Cc: gogerald@chromium.org
Are we recording metrics in UMA for this promo? The sign in team has a standard way of recording metrics. If we want to match the way we're recording metrics for the rest of our sign in promos (which I'd prefer), we'd have:

- Signin_Impression_FromPasswordBubble. A user action that's recorded when the promo is shown
- Signin_Signin_FromPasswordBubble. A user action that's recorded when the user clicks "Sign in" in the bubble
- Signin.SigninStartedAccessPoint.[Password bubble]. A new bucket in the Signin.SigninStartedAccessPoint histogram that gets recorded when the sign in flow is initiated from the password bubble
- Signin.SigninCompletedAccessPoint.[Password bubble]. A new bucket in the Signin.SigninCompletedAccessPoint histogram that gets recorded when the sign in flow that's initiated from the password bubble gets completed.

Then we can calculate:

- CTR: Signin_Signin_FromPasswordBubble/Signin_Signin_FromPasswordBubble
- Completion of the sign in flow: Signin.SigninCompletedAccessPoint.[Password bubble]/Signin.SigninStartedAccessPoint.[Password bubble]
- Overall completion rate of the funnel: (CTR) * (Completion of the sign in flow)

You can look at what we're doing with the BookmarkBubble as an example. Ganggui (cc'ed) implemented all our metrics tracking. He and I can answer any questions you have!

Does that make sense to you?
From the UI review thread: "The padding between the left edge of the bubble and where the text starts seems to vary in Alex's gif. Can we fix that?"

Looks like the left-edge-padding is different between the initial password save bubble and the sign in promo bubble.
Vasilii, as just discussed, could you also please add UMA metrics for clicks on the 'Save' button through the keyboard vs. mouse click? Thanks!
It won't show anything. The bubble is opened with focus (=manually) only in 00.23% cases.
Re 25: a new bucket in Signin.Signin* already exists. Interaction with the promo are already recorded in the following way:

"PasswordManager.SignInPromo" histogram records what user did with the promo (Sign in, No thanks, dismissed) every time it's shown. It shows CTR.
When the user dismisses the promo permanently (Sign in or No thanks) "PasswordManager.SignInPromoCountTilClick" records the number of times it was shown to the user.
Project Member

Comment 31 by bugdroid1@chromium.org, Jul 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0ca3a71dd593c4e447867179703e4dd8a65f50c

commit b0ca3a71dd593c4e447867179703e4dd8a65f50c
Author: vasilii <vasilii@chromium.org>
Date: Tue Jul 05 16:22:13 2016

Fix left padding in the signin promo in the password bubble on Mac.

BUG= 615825 

Review-Url: https://codereview.chromium.org/2117183003
Cr-Commit-Position: refs/heads/master@{#403789}

[modify] https://crrev.com/b0ca3a71dd593c4e447867179703e4dd8a65f50c/chrome/browser/ui/cocoa/passwords/signin_promo_view_controller.mm

Re #30: terrific, thanks Vasilii! That all sounds great.

Could we possibly add two new user actions (Signin_Impression_FromPasswordBubble and Signin_Signin_FromPasswordBubble) that get recorded when the promo is shown and when "Sign in" is clicked, respectively?

I know it's duplicating data in the PasswordManager.SignInPromo histogram, but then we'll be able to use the User Action Sequencer to look at the funnel for signing in across all our user actions/access points :)
Cc: nyerramilli@chromium.org
Labels: TE-Verified-53.0.2785.8 TE-Verified-M53
Tried with 'chrome.exe --force-fieldtrials=SignInPasswordPromo/SomeGroup --force-fieldtrial-params=SignInPasswordPromo.SomeGroup:dismissal_threshold/2' on Win7, Mac OS X & Ubuntu 14.04 using Dev # 53.0.2785.8 - seeing sign in promo with 'x' button.

attached screenshot for reference, adding TE-Verified labels.


615825.jpg
60.3 KB View Download
Friendly ping re #32. Would it be possible to add the two signin-related user actions as well?

Then we'd be able to e.g. add the CTR for the bubble to this dashboard: https://uma.googleplex.com/p/chrome/timeline_v2/?sid=147ce20da1e3a7eb6b9b605dcd942d16
Labels: Merge-Request-53

Comment 37 by ew...@chromium.org, Jul 13 2016

To be clear, I think we need to merge two CLs that missed branch.

(1) CL in #35: https://codereview.chromium.org/2138203002
(2) CL in #31: https://codereview.chromium.org/2117183003
Labels: -merge-merged-2795
Merge request for r403789. The UI change is simple.

Comment 40 by dimu@google.com, Jul 14 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Project Member

Comment 41 by bugdroid1@chromium.org, Jul 15 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/775f55fa6349f34b0f8d53825503c655eebc06dc

commit 775f55fa6349f34b0f8d53825503c655eebc06dc
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Fri Jul 15 11:12:58 2016

Fix left padding in the signin promo in the password bubble on Mac.

BUG= 615825 

Review-Url: https://codereview.chromium.org/2117183003
Cr-Commit-Position: refs/heads/master@{#403789}
(cherry picked from commit b0ca3a71dd593c4e447867179703e4dd8a65f50c)

Review URL: https://codereview.chromium.org/2153103002 .

Cr-Commit-Position: refs/branch-heads/2785@{#147}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/775f55fa6349f34b0f8d53825503c655eebc06dc/chrome/browser/ui/cocoa/passwords/signin_promo_view_controller.mm

Status: Fixed (was: Started)
Project Member

Comment 44 by bugdroid1@chromium.org, Sep 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75c073b7e738fae19168313dcb967b17ac30763c

commit 75c073b7e738fae19168313dcb967b17ac30763c
Author: vasilii <vasilii@chromium.org>
Date: Thu Sep 29 17:22:09 2016

Sign-in promo in the password bubble: test threshold=2 on the waterfall.

BUG= 615825 

Review-Url: https://codereview.chromium.org/2379973002
Cr-Commit-Position: refs/heads/master@{#421859}

[modify] https://crrev.com/75c073b7e738fae19168313dcb967b17ac30763c/testing/variations/fieldtrial_testing_config.json

Project Member

Comment 45 by bugdroid1@chromium.org, Oct 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b63ccca158a9891abef94629ae04f3e34148e17f

commit b63ccca158a9891abef94629ae04f3e34148e17f
Author: vasilii <vasilii@chromium.org>
Date: Fri Oct 28 18:40:22 2016

Sign-in promo in the password bubble: test dismissal_threshold=3 on the waterfall.

BUG= 615825 

Review-Url: https://codereview.chromium.org/2457703006
Cr-Commit-Position: refs/heads/master@{#428438}

[modify] https://crrev.com/b63ccca158a9891abef94629ae04f3e34148e17f/testing/variations/fieldtrial_testing_config.json

Project Member

Comment 46 by bugdroid1@chromium.org, Jan 25 2017

Sign in to add a comment