Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1134863002:
[Smart Lock] Smart Lock branding in save password infobar available only for signed users. (Closed)
|
Created:
5 years, 7 months ago by melandory Modified:
5 years, 7 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org, vabr+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Smart Lock] Smart Lock branding in save password infobar should available only for signed in users and guarded by Finch.
Restrict new title for the smart lock infobar only to the signed in users, who belongs to experiment.
BUG=454815
Committed: https://crrev.com/c90c14a66836737744516e01f63cc3baf05799d2
Cr-Commit-Position: refs/heads/master@{#329387}
Patch Set 1 : #
Total comments: 5
Patch Set 2 : #
Total comments: 2
Messages
Total messages: 24 (14 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
melandory@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, review this CL, please.
LGTM with a comment. Also, please add the bug 486739 in the CL description. Thanks! Vaclav https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/save_password_infobar_delegate.cc (right): https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... chrome/browser/password_manager/save_password_infobar_delegate.cc:136: password_manager::CredentialSourceType::CREDENTIAL_SOURCE_API || Did you mean && instead of ||? (At least that's what the description says.)
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
LGTM 2, as my real question is already existing behavior. https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/save_password_infobar_delegate.cc (right): https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... chrome/browser/password_manager/save_password_infobar_delegate.cc:136: password_manager::CredentialSourceType::CREDENTIAL_SOURCE_API || On 2015/05/11 at 15:29:10, vabr (Chromium) wrote: > Did you mean && instead of ||? > (At least that's what the description says.) Moreover, why are we changing the branding based on whether or not the API is being used? It goes into the same storage, and has the same properties, right? I'd suggest that this should just be toggled based on the `IsEnabledSmartLockBranding` bit, rather than on the source type. https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... chrome/browser/password_manager/save_password_infobar_delegate.h:63: content::WebContents* web_contents, Nit: Could you pass in the profile instead? Or perhaps just the boolean result of ` password_bubble_experiment::IsEnabledSmartLockBranding(profile)`? Fewer references to `WebContents` seems better.
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:180001) has been deleted
https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/save_password_infobar_delegate.cc (right): https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... chrome/browser/password_manager/save_password_infobar_delegate.cc:136: password_manager::CredentialSourceType::CREDENTIAL_SOURCE_API || On 2015/05/11 15:51:48, Mike West (traveling. slow.) wrote: > On 2015/05/11 at 15:29:10, vabr (Chromium) wrote: > > Did you mean && instead of ||? > > (At least that's what the description says.) > Moreover, why are we changing the branding based on whether or not the API is > being used? It goes into the same storage, and has the same properties, right? > I'd suggest that this should just be toggled based on the > `IsEnabledSmartLockBranding` bit, rather than on the source type. I meant || here. Because my understanding was that in future we'll show Smart Lock branding for every user and my idea was that infobar which I pop up on API call represents the "final" version of infobar (title and buttons). But I asked sabine yesterday and she confirmed that I got it wrong. So I've remove CREDENTIAL_SOURCE_API here completely. And an addition, despite I fixed title, infobar for API calls still be different (it has another buttons set), the idea is that the final design is not there and infobar which is shown on api calls represents the latest version, but since it's not approved by UX yet we are showing it only for API calls. https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/1134863002/diff/60001/chrome/browser/password... chrome/browser/password_manager/save_password_infobar_delegate.h:63: content::WebContents* web_contents, On 2015/05/11 15:51:48, Mike West (traveling. slow.) wrote: > Nit: Could you pass in the profile instead? Or perhaps just the boolean result > of ` password_bubble_experiment::IsEnabledSmartLockBranding(profile)`? Fewer > references to `WebContents` seems better. Done.
LGTM, though I remain confused about what we're calling things and when. https://codereview.chromium.org/1134863002/diff/200001/chrome/browser/passwor... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/1134863002/diff/200001/chrome/browser/passwor... chrome/browser/password_manager/save_password_infobar_delegate.h:89: // users. Aren't we going to use the "smart lock" terminology for everyone?
https://codereview.chromium.org/1134863002/diff/200001/chrome/browser/passwor... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/1134863002/diff/200001/chrome/browser/passwor... chrome/browser/password_manager/save_password_infobar_delegate.h:89: // users. On 2015/05/12 09:20:16, Mike West wrote: > Aren't we going to use the "smart lock" terminology for everyone? Nope. We will use string "Do you want <ph name="PASSWORD_MANAGER_BRAND"/> to save your password for this site" where PASSWORD_MANAGER_BRAND = "Google Smart Lock" for signed in users and PASSWORD_MANAGER_BRAND = "Google Chrome" or "Chromium" for others.
LGTM as well. Cheers, Vaclav
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1134863002/200001
Message was sent while issue was closed.
Committed patchset #2 (id:200001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c90c14a66836737744516e01f63cc3baf05799d2 Cr-Commit-Position: refs/heads/master@{#329387} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
