Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576143002:
Tweak payment autofill disabled message in form-not-secure (Closed)
|
Created:
4 years ago by estark Modified:
4 years ago Reviewers:
Mathieu CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, lshang Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTweak payment autofill disabled message in form-not-secure
When the "Payment not secure" message is shown, the item below it should
be "Payment autofilling disabled" instead of the "Automatic credit card
filling is disabled..." message, and it should use the normal font
rather than the warning font.
This CL adds a new string and uses it for this warning
message when the form-not-secure experiment is enabled. See
https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a
screenshot.
The new warning message is styled the same as the existing warning messge.
To match the form-not-secure mocks, for simplicity, I've removed the warning
font list and uses the normal font for the warning message, whether it is the
original or new wording.
BUG=672666
Committed: https://crrev.com/f871c1f29a63064081b6f65300d6a1262d4ad0d2
Cr-Commit-Position: refs/heads/master@{#438931}
Patch Set 1 #Patch Set 2 : fix comment wrapping #
Total comments: 4
Patch Set 3 : Remove new PopupItemId and remove warning_font_list_ #
Messages
Total messages: 33 (22 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Description was changed from ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and PopupItemId and uses them for this warning message when the form-not-secure experiment is enabled. BUG=672666 ========== to ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and PopupItemId and uses them for this warning message when the form-not-secure experiment is enabled. See https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a screenshot. BUG=672666 ==========
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 checked by estark@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...
estark@chromium.org changed reviewers: + mathp@chromium.org
mathp, can you please review? This is for the HTTP-bad form-not-secure project that lshang@ was working on. I and others will be picking up some polishing and tweaks that she won't get to before she leaves Chrome. Thanks!
https://codereview.chromium.org/2576143002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2576143002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:596: ? POPUP_ITEM_ID_PAYMENT_DISABLED_MESSAGE I'm not sure if introducing a new PopupItemId is the right thing to do here; the only place I need to distinguish from INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE is in AutofillPopupLayoutModel::GetValueFontListForRow, and I could just check whether the experiment is enabled there. Any preference?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2576143002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2576143002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:178: case POPUP_ITEM_ID_PAYMENT_DISABLED_MESSAGE: I think we should align so that both kinds of warning messages have the same font. Getting rid of warning_font_list_ doesn't sound so bad to me. It will still have a dimmed color so I don't mind it not being italic. WDYT? https://codereview.chromium.org/2576143002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2576143002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:596: ? POPUP_ITEM_ID_PAYMENT_DISABLED_MESSAGE On 2016/12/14 23:46:08, estark wrote: > I'm not sure if introducing a new PopupItemId is the right thing to do here; the > only place I need to distinguish from INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE > is in AutofillPopupLayoutModel::GetValueFontListForRow, and I could just check > whether the experiment is enabled there. Any preference? Replied over there. I think we should get rid of warning_font_list_ and have just one PopupItemId
The CQ bit was checked by estark@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...
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by estark@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...
Description was changed from ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and PopupItemId and uses them for this warning message when the form-not-secure experiment is enabled. See https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a screenshot. BUG=672666 ========== to ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and PopupItemId and uses them for this warning message when the form-not-secure experiment is enabled. See https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a screenshot. For simplicity, this removes the warning font list and uses the normal font for the warning message, whether it is the original or new wording. BUG=672666 ==========
Thanks! I removed the new PopupItemId and the warning_font_list_, PTAL. https://codereview.chromium.org/2576143002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2576143002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:178: case POPUP_ITEM_ID_PAYMENT_DISABLED_MESSAGE: On 2016/12/15 17:52:06, Mathieu Perreault wrote: > I think we should align so that both kinds of warning messages have the same > font. Getting rid of warning_font_list_ doesn't sound so bad to me. It will > still have a dimmed color so I don't mind it not being italic. WDYT? Done.
lgtm please update the change description.
Description was changed from ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and PopupItemId and uses them for this warning message when the form-not-secure experiment is enabled. See https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a screenshot. For simplicity, this removes the warning font list and uses the normal font for the warning message, whether it is the original or new wording. BUG=672666 ========== to ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and uses it for this warning message when the form-not-secure experiment is enabled. See https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a screenshot. The new warning message is styled the same as the existing warning messge. To match the form-not-secure mocks, for simplicity, I've removed the warning font list and uses the normal font for the warning message, whether it is the original or new wording. BUG=672666 ==========
On 2016/12/15 19:29:15, Mathieu Perreault wrote: > lgtm > > please update the change description. Done. Thanks for the review!
The CQ bit was unchecked by estark@chromium.org
The CQ bit was checked by estark@chromium.org
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estark@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": 60001, "attempt_start_ts": 1481834782289360,
"parent_rev": "eda3372da421b7edf9bb9c70bb952489b2ecc085", "commit_rev":
"5de9ba300f987dd57516e440c59affdbf154b4a5"}
Message was sent while issue was closed.
Description was changed from ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and uses it for this warning message when the form-not-secure experiment is enabled. See https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a screenshot. The new warning message is styled the same as the existing warning messge. To match the form-not-secure mocks, for simplicity, I've removed the warning font list and uses the normal font for the warning message, whether it is the original or new wording. BUG=672666 ========== to ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and uses it for this warning message when the form-not-secure experiment is enabled. See https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a screenshot. The new warning message is styled the same as the existing warning messge. To match the form-not-secure mocks, for simplicity, I've removed the warning font list and uses the normal font for the warning message, whether it is the original or new wording. BUG=672666 Review-Url: https://codereview.chromium.org/2576143002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and uses it for this warning message when the form-not-secure experiment is enabled. See https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a screenshot. The new warning message is styled the same as the existing warning messge. To match the form-not-secure mocks, for simplicity, I've removed the warning font list and uses the normal font for the warning message, whether it is the original or new wording. BUG=672666 Review-Url: https://codereview.chromium.org/2576143002 ========== to ========== Tweak payment autofill disabled message in form-not-secure When the "Payment not secure" message is shown, the item below it should be "Payment autofilling disabled" instead of the "Automatic credit card filling is disabled..." message, and it should use the normal font rather than the warning font. This CL adds a new string and uses it for this warning message when the form-not-secure experiment is enabled. See https://bugs.chromium.org/p/chromium/issues/detail?id=672666#c2 for a screenshot. The new warning message is styled the same as the existing warning messge. To match the form-not-secure mocks, for simplicity, I've removed the warning font list and uses the normal font for the warning message, whether it is the original or new wording. BUG=672666 Committed: https://crrev.com/f871c1f29a63064081b6f65300d6a1262d4ad0d2 Cr-Commit-Position: refs/heads/master@{#438931} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f871c1f29a63064081b6f65300d6a1262d4ad0d2 Cr-Commit-Position: refs/heads/master@{#438931} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
