Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2604273004:
Style the Form-Not-Secure warning on Mac (Closed)
|
Created:
3 years, 11 months ago by estark Modified:
3 years, 11 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, estade+watch_chromium.org, sebsg+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, mathp+autofillwatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStyle the Form-Not-Secure warning on Mac
This CL fixes the layout/styling of the "Login not secure"/"Payment not secure"
warning in the autofill dropdown on HTTP pages. The fixes are to:
- make the icon/title/subtitle appear in the correct order
- use the correct icons
BUG=672662
Review-Url: https://codereview.chromium.org/2604273004
Cr-Commit-Position: refs/heads/master@{#442151}
Committed: https://chromium.googlesource.com/chromium/src/+/f05489ec7aa13934819a0ac5363a65d491fa8e82
Patch Set 1 : missing #include #Patch Set 2 : add comment #
Total comments: 4
Messages
Total messages: 46 (34 generated)
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...
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...
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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) 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 ========== Define HTTP warning icons on Mac BUG=672662 ========== to ========== Style the Form-Not-Secure warning on Mac This CL fixes the layout/styling of the "Login not secure"/"Payment not secure" warning in the autofill dropdown on HTTP pages. The fixes are to: - make the icon/title/subtitle appear in the correct order - use the correct icons BUG=672662 ==========
estark@chromium.org changed reviewers: + groby@chromium.org
groby, can you please review? See go/fns-ui-spec for specs and https://bugs.chromium.org/p/chromium/issues/detail?id=672662#c3 for screenshots of this fix. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mathp@chromium.org changed reviewers: + mathp@chromium.org
lgtm with nit https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:288: if (icon == base::ASCIIToUTF16("httpWarning")) { could you try removing https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/autofill_popu... and 59? it seems we are not really getting the image resources for these and rather using vectors on all platforms.
https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:288: if (icon == base::ASCIIToUTF16("httpWarning")) { On 2017/01/05 20:37:18, Mathieu Perreault wrote: > could you try removing > https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/autofill_popu... > and 59? it seems we are not really getting the image resources for these and > rather using vectors on all platforms. I think we use rely on those lines on Android, no? https://cs.chromium.org/chromium/src/chrome/browser/ui/android/autofill/autof...
On 2017/01/05 21:24:45, estark (slow thru Jan 6) wrote: > https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): > > https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:288: if (icon == > base::ASCIIToUTF16("httpWarning")) { > On 2017/01/05 20:37:18, Mathieu Perreault wrote: > > could you try removing > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/autofill_popu... > > and 59? it seems we are not really getting the image resources for these and > > rather using vectors on all platforms. > > I think we use rely on those lines on Android, no? > https://cs.chromium.org/chromium/src/chrome/browser/ui/android/autofill/autof... My bad! Indeed Android has a way to map the ID to a Vector automagically, which confused me. Sorry
On 2017/01/05 21:31:49, Mathieu Perreault wrote: > On 2017/01/05 21:24:45, estark (slow thru Jan 6) wrote: > > > https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... > > File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): > > > > > https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... > > chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:288: if (icon == > > base::ASCIIToUTF16("httpWarning")) { > > On 2017/01/05 20:37:18, Mathieu Perreault wrote: > > > could you try removing > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/autofill_popu... > > > and 59? it seems we are not really getting the image resources for these and > > > rather using vectors on all platforms. > > > > I think we use rely on those lines on Android, no? > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/android/autofill/autof... > > My bad! Indeed Android has a way to map the ID to a Vector automagically, which > confused me. Sorry No worries. Thanks for the quick review!
Slightly overeager friendly ping to groby; I know it hasn't been that long since I sent this, but after the lgtm from mathp I just wanted to make sure you know that I'll still need your approval as an OWNER for this. Thanks!
On 2017/01/06 21:52:22, estark (slow thru Jan 6) wrote: > Slightly overeager friendly ping to groby; I know it hasn't been that long since > I sent this, but after the lgtm from mathp I just wanted to make sure you know > that I'll still need your approval as an OWNER for this. Thanks! I believe groby is OOO.
estark@chromium.org changed reviewers: + rsesek@chromium.org
+rsesek since groby is OOO (thanks for the heads-up, mathp)
LGTM w/ a comment https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:288: if (icon == base::ASCIIToUTF16("httpWarning")) { Is there not a constant for this available?
Thanks Robert. https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2604273004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:288: if (icon == base::ASCIIToUTF16("httpWarning")) { On 2017/01/06 22:28:10, Robert Sesek wrote: > Is there not a constant for this available? Hmm, doesn't appear to be; these icons are selected via string literals throughout the autofill code (see the uses of https://cs.chromium.org/chromium/src/components/autofill/core/browser/suggest... and also https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/autofill_popu...)
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": 120001, "attempt_start_ts": 1483760089860540,
"parent_rev": "c48bc79acb2ed96e2f71856c01b57ce369681ba7", "commit_rev":
"f05489ec7aa13934819a0ac5363a65d491fa8e82"}
Message was sent while issue was closed.
Description was changed from ========== Style the Form-Not-Secure warning on Mac This CL fixes the layout/styling of the "Login not secure"/"Payment not secure" warning in the autofill dropdown on HTTP pages. The fixes are to: - make the icon/title/subtitle appear in the correct order - use the correct icons BUG=672662 ========== to ========== Style the Form-Not-Secure warning on Mac This CL fixes the layout/styling of the "Login not secure"/"Payment not secure" warning in the autofill dropdown on HTTP pages. The fixes are to: - make the icon/title/subtitle appear in the correct order - use the correct icons BUG=672662 Review-Url: https://codereview.chromium.org/2604273004 Cr-Commit-Position: refs/heads/master@{#442151} Committed: https://chromium.googlesource.com/chromium/src/+/f05489ec7aa13934819a0ac5363a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f05489ec7aa13934819a0ac5363a... |
