Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2832913002:
Harmonizing SmartLock dialog (Closed)
DescriptionHarmonizing SmartLock dialog
Including more Smartlock & password dialog tests.
BUG=651682
TBR=grt@chromium.org
Review-Url: https://codereview.chromium.org/2832913002
Cr-Original-Commit-Position: refs/heads/master@{#466680}
Committed: https://chromium.googlesource.com/chromium/src/+/55c39ecb9bbf8e30bac883e53b6f0b09cd02f0c2
Review-Url: https://codereview.chromium.org/2832913002
Cr-Commit-Position: refs/heads/master@{#467110}
Committed: https://chromium.googlesource.com/chromium/src/+/be89a27c0b24eec0e9ef42c5131061d28cb61c2f
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added different string for the auto signin in material mode. #
Total comments: 8
Patch Set 3 : Some post-commit feedback #Patch Set 4 : Rebase #
Messages
Total messages: 35 (25 generated)
Description was changed from ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 ========== to ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 ==========
kylixrd@chromium.org changed reviewers: + pkasting@chromium.org, vabr@chromium.org
This only addresses the arrangement of the labels and the spacing between the avatar image and the labels. The dialog size and font weights are still in process on other fronts.
Thanks! I have to believe you that the resulting UI looks like on the mocks. I also have one comment below. Other than that, LGTM. Cheers, Vaclav https://codereview.chromium.org/2832913002/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2832913002/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:230: l10n_util::GetStringFUTF16(IDS_MANAGE_PASSWORDS_AUTO_SIGNIN_TITLE, Inserting an empty string here seems to be a bit of a hack. Could we introduce a non-parametric version of this string instead, so that in languages where the placeholder does not come at the end, the result still makes sense?
The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Here's a screen shot of the dialog sans the proper dialog width and font style/color. https://drive.google.com/open?id=0ByQVK9J-OaJNR3ktZVRoM1VJNEE https://codereview.chromium.org/2832913002/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2832913002/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:230: l10n_util::GetStringFUTF16(IDS_MANAGE_PASSWORDS_AUTO_SIGNIN_TITLE, On 2017/04/21 14:28:15, vabr (Chromium) wrote: > Inserting an empty string here seems to be a bit of a hack. Could we introduce a > non-parametric version of this string instead, so that in languages where the > placeholder does not come at the end, the result still makes sense? That thought had crossed my mind. Next patch has the change.
Description was changed from ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 ========== to ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 TBR=grt@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/21 16:17:30, kylix_rd wrote: > Here's a screen shot of the dialog sans the proper dialog width and font > style/color. > > https://drive.google.com/open?id=0ByQVK9J-OaJNR3ktZVRoM1VJNEE > > https://codereview.chromium.org/2832913002/diff/1/chrome/browser/ui/views/pas... > File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): > > https://codereview.chromium.org/2832913002/diff/1/chrome/browser/ui/views/pas... > chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:230: > l10n_util::GetStringFUTF16(IDS_MANAGE_PASSWORDS_AUTO_SIGNIN_TITLE, > On 2017/04/21 14:28:15, vabr (Chromium) wrote: > > Inserting an empty string here seems to be a bit of a hack. Could we introduce > a > > non-parametric version of this string instead, so that in languages where the > > placeholder does not come at the end, the result still makes sense? > > That thought had crossed my mind. Next patch has the change. Thanks! In that case LGTM without any further comments. Cheers, Vaclav
kylixrd@chromium.org changed reviewers: - pkasting@chromium.org
The CQ bit was checked by kylixrd@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493053752820060,
"parent_rev": "e09511329119adbacc5e291b5d16b31fcb14a4d4", "commit_rev":
"55c39ecb9bbf8e30bac883e53b6f0b09cd02f0c2"}
Message was sent while issue was closed.
Description was changed from ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 TBR=grt@chromium.org ========== to ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 TBR=grt@chromium.org Review-Url: https://codereview.chromium.org/2832913002 Cr-Commit-Position: refs/heads/master@{#466680} Committed: https://chromium.googlesource.com/chromium/src/+/55c39ecb9bbf8e30bac883e53b6f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/55c39ecb9bbf8e30bac883e53b6f...
Message was sent while issue was closed.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Message was sent while issue was closed.
LGTM https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/credentials_item_view.cc (right): https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/credentials_item_view.cc:153: DISTANCE_UNRELATED_CONTROL_HORIZONTAL), Semantically, should this and the one below be RELATED_LABEL_HORIZONTAL? I realize that's a behavior change pre-MD, which I'm OK with. It's more a question of whether this is enough like "icon plus a label" or if it's more "some random image, and separately some text items", in which case the constant you chose is more correct. https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:246: } Nit: I'd like to avoid this code duplication. Maybe: base::string16 upper_text, lower_text = form.username_value; if (ChromeLayoutProvider::Get()->IsHarmonyMode()) { upper_text = l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_AUTO_SIGNIN_TITLE_MD); } else { lower_text = l10n_util::GetStringFUTF16( IDS_MANAGE_PASSWORDS_AUTO_SIGNIN_TITLE, lower_text); } CredentialsItemView* credential = new CredentialsItemView( this, upper_text, lower_text, kButtonHoverColor, &form, parent_->model()->GetProfile()->GetRequestContext()); https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc (right): https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:34: base::MakeUnique<autofill::PasswordForm>(*test_form())); Nit: Use = {...} to init-at-definition instead of define + push_back May also be possible to go further and just do this below: SetupAutoSignin({base::MakeUnique<autofill::PasswordForm>(*test_form())}); https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:481: GURL icon_url("https://google.com/icon.png"); Nit: Inline into next line
Message was sent while issue was closed.
Here's some additional changes based on the feedback. I'll re-land this. https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/credentials_item_view.cc (right): https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/credentials_item_view.cc:153: DISTANCE_UNRELATED_CONTROL_HORIZONTAL), On 2017/04/24 22:48:31, Peter Kasting wrote: > Semantically, should this and the one below be RELATED_LABEL_HORIZONTAL? I > realize that's a behavior change pre-MD, which I'm OK with. It's more a > question of whether this is enough like "icon plus a label" or if it's more > "some random image, and separately some text items", in which case the constant > you chose is more correct. The kSpacing value was 12, so I chose the element that was 12 pre-MD. However, you're analysis makes sense. The DISTANCE_RELATED_LABEL_HORIZONTAL pre-MD value is 10, so that would move the image 2 dips closer. However, it will still be a harmony-unit (16) in MD mode. https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:246: } On 2017/04/24 22:48:31, Peter Kasting wrote: > Nit: I'd like to avoid this code duplication. Maybe: > > base::string16 upper_text, lower_text = form.username_value; > if (ChromeLayoutProvider::Get()->IsHarmonyMode()) { > upper_text = > l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_AUTO_SIGNIN_TITLE_MD); > } else { > lower_text = l10n_util::GetStringFUTF16( > IDS_MANAGE_PASSWORDS_AUTO_SIGNIN_TITLE, lower_text); > } > CredentialsItemView* credential = new CredentialsItemView( > this, upper_text, lower_text, kButtonHoverColor, &form, > parent_->model()->GetProfile()->GetRequestContext()); I like this solution. Thanks. https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc (right): https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:34: base::MakeUnique<autofill::PasswordForm>(*test_form())); On 2017/04/24 22:48:31, Peter Kasting wrote: > Nit: Use = {...} to init-at-definition instead of define + push_back > > May also be possible to go further and just do this below: > > SetupAutoSignin({base::MakeUnique<autofill::PasswordForm>(*test_form())}); Due to the use of std::unique_ptr, {...} initialization doesn't work because of deleted constructors. https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2832913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:481: GURL icon_url("https://google.com/icon.png"); On 2017/04/24 22:48:31, Peter Kasting wrote: > Nit: Inline into next line Done.
Message was sent while issue was closed.
Description was changed from ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 TBR=grt@chromium.org Review-Url: https://codereview.chromium.org/2832913002 Cr-Commit-Position: refs/heads/master@{#466680} Committed: https://chromium.googlesource.com/chromium/src/+/55c39ecb9bbf8e30bac883e53b6f... ========== to ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 TBR=grt@chromium.org Review-Url: https://codereview.chromium.org/2832913002 Cr-Commit-Position: refs/heads/master@{#466680} Committed: https://chromium.googlesource.com/chromium/src/+/55c39ecb9bbf8e30bac883e53b6f... ==========
The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2832913002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493153844326680,
"parent_rev": "13d62726734aef77f87472ccbad08a5a4987b430", "commit_rev":
"be89a27c0b24eec0e9ef42c5131061d28cb61c2f"}
Message was sent while issue was closed.
Description was changed from ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 TBR=grt@chromium.org Review-Url: https://codereview.chromium.org/2832913002 Cr-Commit-Position: refs/heads/master@{#466680} Committed: https://chromium.googlesource.com/chromium/src/+/55c39ecb9bbf8e30bac883e53b6f... ========== to ========== Harmonizing SmartLock dialog Including more Smartlock & password dialog tests. BUG=651682 TBR=grt@chromium.org Review-Url: https://codereview.chromium.org/2832913002 Cr-Original-Commit-Position: refs/heads/master@{#466680} Committed: https://chromium.googlesource.com/chromium/src/+/55c39ecb9bbf8e30bac883e53b6f... Review-Url: https://codereview.chromium.org/2832913002 Cr-Commit-Position: refs/heads/master@{#467110} Committed: https://chromium.googlesource.com/chromium/src/+/be89a27c0b24eec0e9ef42c51310... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/be89a27c0b24eec0e9ef42c51310... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
