Issue metadata
Sign in to add a comment
|
Regression : Tab alert indicator appears black in color (instead of red).
Reported by
avsha...@etouch.net,
May 15 2018
|
||||||||||||||||||||||
Issue descriptionChrome Version : 66.0.3359.181 (Official Build) a10b9cedb40738cb152f8148ddab4891df876959-refs/branch-heads/3359@{#828} 64-bit OS : Mac(10.12.6, 10.13.1, 10.13.5) Test URL : https://permission.site/ What steps will reproduce the problem? 1. Launch chrome, navigate to above test URL and click on 'Microphone' button on the page. 2. Click on 'Allow' in permission bubble and observe the color of tab alert indicator. Actual Result : Tab alert indicator appears black in color (instead of red). Expected Result : Tab alert indicator should appear red in color. This is a regression issue, broken in M-66 and providing the bisect using Per-Revision script: Good Build : 66.0.3345.0 (Revision : 536027) Bad Build : 66.0.3346.0 (Revision : 536238) Change Log URL : https://chromium.googlesource.com/chromium/src/+log/77e3c37a5378e1e70aad28e260a662a68818ec60..14f736d1b6aefdf1dfe01d1cbd99433a8ac86e62 Suspect : https://chromium.googlesource.com/chromium/src/+/14f736d1b6aefdf1dfe01d1cbd99433a8ac86e62 @Malay : Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner. Note : 1. Tab alert indicator is seen in red color on Win(7, 8, 8.1, 10) and Linux(14.04 LTS) OS. 2. Issue is also reproducible in Canary #68.0.3431.0, Beta #67.0.3396.40 and Dev #68.0.3423.2
,
May 15 2018
Mac in Chrome 66 should be doing its own cocoa implementation of things. You'd have to investigate the source code to see how that intersects with your code, but it wouldn't be using the views tabstrip files.
,
May 15 2018
Assigning to OWNER of cocoa version of alert icon indicator who have better knowledge of the codebase. Context: The alert indicator icon file for the media capture state no longer holds the color of the icon. It needs to be added in the native code as done here: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab.cc?dr=C&l=368
,
May 15 2018
Sorry, I cannot take assignment of this bug. I worked on this indicator icon years ago, but I'm not a UI main and don't have sufficient background on this issue. Since your change caused the regression, it seems it would be your responsibility to fix it, or find a UI OWNER to help delegate?
,
May 15 2018
I tried looking into the code, but I don't have enough experience in Objective C to make the necessary changes or understand the caveats on what might go wrong. I was hoping you could reassign or point me to someone who could help resolve this.
,
May 15 2018
In the past, I've worked with avi@, rsesek@, and thakis@ on cocoa UI changes. They should be able to help, or know someone who can help (or anyone in https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/OWNERS).
,
May 15 2018
Thanks!
,
May 15 2018
It's not clear to me how this originally worked. In alert_indicator_button_cocoa.mm, updateIconForState asks TabView for the iconColor, which is always monochrome (think mute indicator, etc). TabView's iconColor should be more like Views's Tab::GetAlertIndicatorColor as noted in comment 3.
,
May 15 2018
Actually, -[TabView iconColor] is used elsewhere. TabView should get an -alertIndicatorColor as in Tab::GetAlertIndicatorColor, and -[AlertIndicatorButton updateIconForState:] should call it.
,
May 16 2018
I guess I'll take it then?
,
May 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6680bbcc64b208500b1fc8c21cb22aa8805ad686 commit 6680bbcc64b208500b1fc8c21cb22aa8805ad686 Author: Avi Drissman <avi@chromium.org> Date: Wed May 16 20:32:40 2018 Return color to the microphone tab indicator. BUG= 843075 Change-Id: Iccb161d3d584cf434ae20b5ec91134b04996c999 Reviewed-on: https://chromium-review.googlesource.com/1062411 Reviewed-by: Sidney San Martín <sdy@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#559268} [modify] https://crrev.com/6680bbcc64b208500b1fc8c21cb22aa8805ad686/chrome/browser/ui/cocoa/tabs/alert_indicator_button_cocoa.mm [modify] https://crrev.com/6680bbcc64b208500b1fc8c21cb22aa8805ad686/chrome/browser/ui/cocoa/tabs/tab_view.h [modify] https://crrev.com/6680bbcc64b208500b1fc8c21cb22aa8805ad686/chrome/browser/ui/cocoa/tabs/tab_view.mm
,
May 16 2018
This is fixed. This will be in 68, and turns out to be a pretty straightforward merge to 67 and 66.
,
May 16 2018
This bug requires manual review: We are only 12 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 16 2018
Good point. 66 has 12 days left in stable, so there's no point in merging it.
,
May 16 2018
Cl listed at #11 is not baked/verified in canary yet. Also since this M66 regression, can't this wait until M68 as we're only taking critical/safe merges in at this point for M67 which is going to stable soon?
,
May 16 2018
This is not a critical change (just improperly colored UI) so I wouldn't be heartbroken if you say no.
,
May 16 2018
Rejecting merge to M67 based on comment #16 and per offline chat with avi@. Thank you.
,
May 16 2018
Changing it back to Merge-Review-67 per chat with avi@ this should be straightforward merge. Pls update the bug with canary result on Friday morning.
,
May 17 2018
Update : Retested above issue in latest Canary build #68.0.3433.0 on Mac(10.12.6, 10.13.1, 10.13.5) OS and the issue is fixed. Now, tab alert indicator appears in red color as expected. Kindly review an attached screen-cast. Thank you..!
,
May 17 2018
Issue 844066 has been merged into this issue.
,
May 18 2018
The NextAction date has arrived: 2018-05-18
,
May 18 2018
This looks solid. No crashes or any disruption seen. Your call.
,
May 18 2018
This bug requires manual review: We are only 10 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 18 2018
Approving merge to M67 branch 3396 based on comment #12, #19 and #22. Please merge. Thank you.
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e275baac6c25a72eeb0072d1f1cbd482e3261f2e commit e275baac6c25a72eeb0072d1f1cbd482e3261f2e Author: Avi Drissman <avi@chromium.org> Date: Fri May 18 16:37:11 2018 Return color to the microphone tab indicator. BUG= 843075 TBR=avi@chromium.org Change-Id: Iccb161d3d584cf434ae20b5ec91134b04996c999 Reviewed-on: https://chromium-review.googlesource.com/1062411 Reviewed-by: Sidney San Martín <sdy@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#559268}(cherry picked from commit 6680bbcc64b208500b1fc8c21cb22aa8805ad686) Reviewed-on: https://chromium-review.googlesource.com/1066114 Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#641} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/e275baac6c25a72eeb0072d1f1cbd482e3261f2e/chrome/browser/ui/cocoa/tabs/alert_indicator_button_cocoa.mm [modify] https://crrev.com/e275baac6c25a72eeb0072d1f1cbd482e3261f2e/chrome/browser/ui/cocoa/tabs/tab_view.h [modify] https://crrev.com/e275baac6c25a72eeb0072d1f1cbd482e3261f2e/chrome/browser/ui/cocoa/tabs/tab_view.mm
,
May 21 2018
Issue 845098 has been merged into this issue.
,
May 23 2018
Update : Retested above issue in latest Beta build # 67.0.3396.56 on Mac(10.12.6, 10.13.1, 10.13.5) and the issue is fixed. Now, Tab alert indicator appears red in color as expected. Kindly review an attached screen-cast. Thank you..! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by malaykeshav@chromium.org
, May 15 2018