Security icons are not colored |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Issue descriptionVersion: Version 52.0.2743.41 beta (64-bit) OS: Mac What is the expected output? Icons should be tinted according to SecurityLevel What do you see instead? Security icons are grey.
,
Jun 17 2016
Can you provide more info? As far as I knew everything is/was correct, so I'm wondering which icons are wrong, and is it normal mod or Incognito modes or both? Please add screenshots, and pointers to specs.
,
Jun 17 2016
I attached screenshots of what it currently looks like in 52. As you can see, the icons are gray. I also attached screenshots of what it looks like in 53, with colors, which is correct.
,
Jun 17 2016
And the spec is here: https://drive.google.com/corp/drive/u/0/folders/0B6Wxmj9LZL6XZjJURlVaN1hUTzQ
,
Jun 17 2016
Sorry! I'm testing Version 52.0.2743.41 beta (64-bit) on Mac right now. In non-incognito, I'm seeing grey icons as attached. In incognito, I'm actually seeing the old icon, as attached.
,
Jun 17 2016
Very strange. spqchan@, would you please take a look?
,
Jun 17 2016
The old EV certs are also appearing in Beta :(
,
Jun 17 2016
,
Jun 17 2016
emilyschechter@ - Issue 611113 contains two patches, on for evcerts, the other for security icons. It looks like only the security icon patch got cherry-picked back, so we need to get the evcert patch cherry-picked as well.
,
Jun 20 2016
Can confirmed #9. The missing patch was cherry-picked back to M52. The issue should be fixed in the upcoming beta.
,
Jun 22 2016
CL in progress The incognito vector icon is incorrect. Here's a CL to fix it: https://codereview.chromium.org/2083813005/ Here attached are the icons when the CL is applied
,
Jun 22 2016
spqchan@ - the screenshots you have so far are, in order: http evcert http http evcert Incongito https insecure Would you also add the following screenshots: http Incognito - (i) https insecure Incognito - triangle https secure - lock, but not ev https secure Incognito - lock, but not ev
,
Jun 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ac19cbddbeda10015a9bbdd9bce1b53715e56a0 commit 0ac19cbddbeda10015a9bbdd9bce1b53715e56a0 Author: spqchan <spqchan@chromium.org> Date: Thu Jun 23 00:15:26 2016 [Material][Mac] Update the incognito vector icon color to white BUG= 621068 Review-Url: https://codereview.chromium.org/2083813005 Cr-Commit-Position: refs/heads/master@{#401477} [modify] https://crrev.com/0ac19cbddbeda10015a9bbdd9bce1b53715e56a0/chrome/browser/ui/cocoa/location_bar/location_bar_decoration.mm [modify] https://crrev.com/0ac19cbddbeda10015a9bbdd9bce1b53715e56a0/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
,
Jun 23 2016
Sure thing, here is the rest of the screenshots
,
Jun 23 2016
Thank you.
,
Jun 27 2016
** IMPORTANT change in M52 merge date due to first 2 weeks of July no release weeks ** M52 Stable is launching very soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged ASAP. All changes MUST be merged into the release branch by 5pm on July 1 to make into the desktop Stable final build cut. Thank you!
,
Jun 27 2016
,
Jun 27 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jun 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b8f1c6a583ffc3006a96d76994cb9813e2cb11ab commit b8f1c6a583ffc3006a96d76994cb9813e2cb11ab Author: spqchan <spqchan@chromium.org> Date: Mon Jun 27 21:18:44 2016 [Material][Mac] Update the incognito vector icon color to white BUG= 621068 Review-Url: https://codereview.chromium.org/2083813005 Cr-Commit-Position: refs/heads/master@{#401477} (cherry picked from commit 0ac19cbddbeda10015a9bbdd9bce1b53715e56a0) Review URL: https://codereview.chromium.org/2104623002 . Cr-Commit-Position: refs/branch-heads/2743@{#488} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/b8f1c6a583ffc3006a96d76994cb9813e2cb11ab/chrome/browser/ui/cocoa/location_bar/location_bar_decoration.mm [modify] https://crrev.com/b8f1c6a583ffc3006a96d76994cb9813e2cb11ab/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
,
Jun 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6e973be6d3845575acd0e069cf61262960004457 commit 6e973be6d3845575acd0e069cf61262960004457 Author: spqchan <spqchan@chromium.org> Date: Tue Jun 28 00:32:31 2016 [Material][Mac] Update the incognito vector icon color to white BUG= 621068 Review-Url: https://codereview.chromium.org/2083813005 Cr-Commit-Position: refs/heads/master@{#401477} (cherry picked from commit 0ac19cbddbeda10015a9bbdd9bce1b53715e56a0) Review URL: https://codereview.chromium.org/2104723002 . Cr-Commit-Position: refs/branch-heads/2743@{#499} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/6e973be6d3845575acd0e069cf61262960004457/chrome/browser/ui/cocoa/location_bar/location_bar_decoration.mm [modify] https://crrev.com/6e973be6d3845575acd0e069cf61262960004457/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
,
Jun 28 2016
Rechecked this on chrome version 53.0.2782.0 on MAC 10.11.5. Fix is working as intended. Attached screen shots for both Incognito and Non-Incognito windows. Adding TE-Verified labels. Thanks.!
,
Jun 28 2016
,
Jun 28 2016
Re #22: ranjitkan@chromium.org, the bug was specific to Chrome 52 beta and onwards. The bug was already previously fixed for Chrome 53. Could you please verify for 52?
,
Jun 29 2016
Sure, will verify on next beta candidate and with update accordingly. Thanks.!
,
Jun 30 2016
@ felt: Rechecked this on chrome beta version# 52.0.2743.60. Change is not observed on this version. Still able to reproduce it. Attached screen shots for the same. Request you to please take a look into it. Thanks.!
,
Jun 30 2016
Reopening and assigning to spqchan@ to check out what happened.
,
Jun 30 2016
Requesting merge request for https://codereview.chromium.org/2108193004/ once it's LGTMed The CL for one of the icons wasn't landed for M52 so this CL will fix it
,
Jun 30 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jul 1 2016
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 1 2016
This is working fine on Mac OSX 10.11.5 - 53.0.2785.0 [64 bit] Canary. @sarah, please take a look at the attached screen shots and merge the CL in to the M52 branch asap if everything looks good.
,
Jul 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d9bdbbd41f4e4d61d9f512100e9ed03ed122ed05 commit d9bdbbd41f4e4d61d9f512100e9ed03ed122ed05 Author: spqchan <spqchan@chromium.org> Date: Fri Jul 01 19:13:17 2016 [Material][Mac] Fixed HTTP and HTTPS icons in dark mode BUG= 621068 R=avi@chromium.org Review URL: https://codereview.chromium.org/2108193004 . Cr-Commit-Position: refs/branch-heads/2743@{#569} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/d9bdbbd41f4e4d61d9f512100e9ed03ed122ed05/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
,
Jul 1 2016
,
Jul 1 2016
Let me know when you think we should see this in 52 Beta -- I just refreshed Beta and don't see it currently (in 52.0.2743.60 beta)
,
Jul 6 2016
FYI I still don't see this in Chrome Beta (not sure how long it should take though). Sarah, could you confirm it looks ok?
,
Jul 6 2016
Reopening this. I just checked and it looks like the new Beta doesn't include my CL in its range. I'm going to check to see if 52 will get a refresh
,
Jul 6 2016
The next beta release is on Wed July 13
,
Jul 8 2016
If you're talking about your change in #32, that change appears to have landed for the first time on July 1, in which case it hasn't been cherry-picked back to M52 (i.e. I doubt it will just appear in the next beta build on 7/13, if that's what you're thinking). My understanding is the drop-dead for changes to M52 was Thursday, 6/30.
,
Jul 8 2016
I confirmed it with dimu@, it will appear on July 13th if it's merged in the 2743 branch
,
Jul 8 2016
It doesn't look like #32 has been merged in 2743. Usually the cherry-pick commits say "(cherry picked from commit...)" somewhere in the text, which is not the case for #32. Also looking at the actual cl for #32 it appears to be a regular patch that landed on ToT. Note that one of the labels for this bug is "merge-merged-2743" but this refers to the cherry-pick in #21. If I'm wrong that's OK (happens a lot) but if I'm right it means that unless further action is taken, when July 13 rolls around the fix will still not appear in M52.
,
Jul 8 2016
Right, #32 is unusual because it's not a cherry-pick commit, it's a regular patch that got merged into 2743.
I got permission from the TPM to just merge it on 2743 because we don't want it to land on the other branches (hence the confirmation in #31, they wanted to test it on 2743 before letting me merge it).
Anyway, are you unable to find this on the 2743 branch? Because I can see this commit in the logs. The Cr-Commit-Position should be refs/branch-heads/2743@{#569}
,
Jul 8 2016
OK, thanks. Now I'm seeing what you're saying about it landing on 2743.
,
Jul 8 2016
Sarah & Jayson, we can get this checked on the latest 5 PM builds available from Beta branch to make sure the change is in.
,
Jul 8 2016
OK, great (but it sounds like everything should be OK).
,
Jul 13 2016
Still able to repro this issue on MAC (10.11.5) for Google Chrome Beta Version - 52.0.2743.75 Screen-recording is attached.
,
Jul 13 2016
@spqchan: Could you please confirm the fix.
,
Jul 13 2016
The fix seems to be not in the Beta branch. The icons are not colored in the normal window. Please find the attached screenshots.
,
Jul 13 2016
Reason: The CL with the security level text colors did not made it to this. Working on it
,
Jul 13 2016
spqchan@, do you need any help / a security code reviewer?
,
Jul 13 2016
Might it be easier to merge my version of this change?
,
Jul 13 2016
Considering that the main issue is that not all parts of your change had been merged in M52? Probably, although keep in mind your change might introduce an issue where the search icon will turn green in NTP, but the fix for that should be only a few lines. Anyway, two options: 1. Merge your change, resolve the conflicts and make sure the green search icon is fixed. 2. Merge this: https://codereview.chromium.org/2150733002/ I'll be comfortable with either options
,
Jul 13 2016
,
Jul 13 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jul 14 2016
Please merge your change to M52 branch 2743 before 5:00 PM PST Friday (07/15/16) as we are very close to M52 stable candidate cut.
,
Jul 14 2016
Not sure why this didn't get updated, but this got merged: https://chromium.googlesource.com/chromium/src/+/bb23f0a1229af698fb8769fafadf44d7cff521da
,
Jul 14 2016
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 14 2016
The above CL is available @ branch-heads/2743 and will be picked up for any release candidate after- 52.0.2743.76.Did a quick verification and the fix is working as intended. dimu@: Not seeing a bugdroid update, guessing there is some logical error in the Merge script,could you please take a look at the script. spqchan@, please remove Merge-Approved-52 label if there is no pending work related to this bug.
,
Jul 14 2016
It's looks like a bugdroid bug instead of merge script. @sheyang would you please to take a look?
,
Jul 14 2016
,
Jul 14 2016
Hi dimu@, would you give a quick summary? It's not clear to me how bugdroid relates to the security icon here.
,
Jul 14 2016
Also removing ReleaseBlock-Stable since this has landed in 52 per #57. Anything else need to happen before we can move this to fixed?
,
Jul 19 2016
+ReleaseBlock-Stable Can we keep the label until the change is verified for 52? ranjitkan@chromium.org, can you please verify that this is now fixed on 52?
,
Jul 19 2016
Rechecked this on chrome version 52.0.2743.82, merge is working as intended. All the security icons are displayed colored. Attached screenshots and TE-Verified label for the same. Thanks.!
,
Jul 19 2016
Verified the fix on MAC (10.11.5) for Google Chrome Dev Version - 53.0.2785.21 Screen-shots are attached. TE-Verified labels are added.
,
Jul 19 2016
Great! Thanks for checking again ranjitkan@. Removing RBS now that it seems to be in.
,
Jul 19 2016
Should this be fixed in Chrome Beta Version 52.0.2743.75? Because I am still seeing the grey lock icon instead of the green one?!
,
Jul 19 2016
This should be fixed for the next beta candidate scheduled on 07/20.
,
Jul 19 2016
Okay, thank you for the info.
,
Jul 19 2016
,
Dec 9 2016
Security>UX component is deprecated in favor of the Team-Security-UX label |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment 1 by f...@chromium.org
, Jun 17 2016