Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2044223006:
Make toolbar color opaque in browser theme (Closed)
DescriptionMake toolbar color opaque in browser theme
In the bug, the ui assumes using lcd_text by default, but the theme has
transparent toolbar_color, rendering subpixel makes the text seem weird.
This CL forces the color to be opaque (alpha value = 0xff).
It worked before because the can_use_lcd_text property of status bubble
is set before we compute layer draw properties, and the property is
disabled all the time. But now we compute layer draw properties on
demand, the can_use_lcd_text property becomes true as the whole layer
tree is set to be able to use lcd text.
BUG=618278
Committed: https://crrev.com/32c538f0e6cd40ebd53f336f1f3fae7eb404070b
Cr-Commit-Position: refs/heads/master@{#403593}
Patch Set 1 #Patch Set 2 : Disable lcd text when toolbar color is not opaque #Patch Set 3 : Make status bubble background color opaque #
Total comments: 1
Patch Set 4 : Make toolbar color opaque #Patch Set 5 : Make toolbar_color opaque #
Total comments: 2
Patch Set 6 : Toolbar color alpha value is not expected to be supported in unit test. #
Total comments: 3
Patch Set 7 : Resolve nit comments #Patch Set 8 : Fix compile failure #
Messages
Total messages: 44 (16 generated)
sunxd@chromium.org changed reviewers: + ajuma@chromium.org, vollick@chromium.org
sunxd@chromium.org changed reviewers: + estade@chromium.org
sunxd@chromium.org changed reviewers: + sky@chromium.org
Add sky@ as the owner. Hi sky@, do you have any suggestions on how to test the change? Thanks!
Description was changed from ========== Disable lcd text for status bubble when toolbar_color is transparent. In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces NO_SUBPIXEL_RENDERING when the toolbar_color is transparent. BUG=618278 ========== to ========== Disable lcd text for status bubble when toolbar_color is transparent. In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces NO_SUBPIXEL_RENDERING when the toolbar_color is transparent. It worked before because the can_use_lcd_text property of status bubble is set before we compute layer draw properties, and the property is disabled all the time. But now we compute layer draw properties on demand, the can_use_lcd_text property becomes true as the whole layer tree is set to be able to use lcd text. BUG=618278 ==========
On 2016/06/09 20:07:53, sunxd wrote: > Add sky@ as the owner. Hi sky@, do you have any suggestions on how to test the > change? > > Thanks! not an owner but seems good, lgtm
sunxd@chromium.org changed reviewers: + msw@chromium.org
sunxd@chromium.org changed reviewers: + pkasting@chromium.org
lgtm
The toolbar itself doesn't seem to be transparent; why are we respecting the transparency request in the status bubble? Seems like instead the browser should simply ignore the alpha value on this color.
On 2016/06/13 21:08:51, Peter Kasting wrote: > The toolbar itself doesn't seem to be transparent; why are we respecting the > transparency request in the status bubble? > > Seems like instead the browser should simply ignore the alpha value on this > color. If you open a webpage with a dark background, the status bubble is partly transparent, this make me think probably the theme is trying to use a transparent toolbar color intentionally.
On 2016/06/20 14:51:23, sunxd wrote: > On 2016/06/13 21:08:51, Peter Kasting wrote: > > The toolbar itself doesn't seem to be transparent; why are we respecting the > > transparency request in the status bubble? > > > > Seems like instead the browser should simply ignore the alpha value on this > > color. > > If you open a webpage with a dark background, the status bubble is partly > transparent, > this make me think probably the theme is trying to use a transparent toolbar > color > intentionally. That doesn't falsify what I said. We can generally assume themes do things intentionally, but I don't think we should respect transparency requests in the toolbar color no matter what. The theme code should force that color to opaque in the getter.
On 2016/06/20 20:11:41, Peter Kasting wrote: > On 2016/06/20 14:51:23, sunxd wrote: > > On 2016/06/13 21:08:51, Peter Kasting wrote: > > > The toolbar itself doesn't seem to be transparent; why are we respecting the > > > transparency request in the status bubble? > > > > > > Seems like instead the browser should simply ignore the alpha value on this > > > color. > > > > If you open a webpage with a dark background, the status bubble is partly > > transparent, > > this make me think probably the theme is trying to use a transparent toolbar > > color > > intentionally. > > That doesn't falsify what I said. We can generally assume themes do things > intentionally, but I don't think we should respect transparency requests in the > toolbar color no matter what. The theme code should force that color to opaque > in the getter. Yeah, I think so too. But I think probably this CL should only concentrate on bug fix, and maybe have another one that involves feature change, as people might want to argue on it.
On 2016/06/21 13:49:54, sunxd wrote: > On 2016/06/20 20:11:41, Peter Kasting wrote: > > On 2016/06/20 14:51:23, sunxd wrote: > > > On 2016/06/13 21:08:51, Peter Kasting wrote: > > > > The toolbar itself doesn't seem to be transparent; why are we respecting > the > > > > transparency request in the status bubble? > > > > > > > > Seems like instead the browser should simply ignore the alpha value on > this > > > > color. > > > > > > If you open a webpage with a dark background, the status bubble is partly > > > transparent, > > > this make me think probably the theme is trying to use a transparent toolbar > > > color > > > intentionally. > > > > That doesn't falsify what I said. We can generally assume themes do things > > intentionally, but I don't think we should respect transparency requests in > the > > toolbar color no matter what. The theme code should force that color to > opaque > > in the getter. > > Yeah, I think so too. But I think probably this CL should only concentrate on > bug > fix, and maybe have another one that involves feature change, as people might > want > to argue on it. The workaround in this patch is not necessary if we fix the bug properly. We want to avoid bandaids in Chromium code, so we don't want to land the patch here at all. So this CL should be changed to simply force the opacity to 100% to fix the bug correctly.
Description was changed from ========== Disable lcd text for status bubble when toolbar_color is transparent. In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces NO_SUBPIXEL_RENDERING when the toolbar_color is transparent. It worked before because the can_use_lcd_text property of status bubble is set before we compute layer draw properties, and the property is disabled all the time. But now we compute layer draw properties on demand, the can_use_lcd_text property becomes true as the whole layer tree is set to be able to use lcd text. BUG=618278 ========== to ========== Make status bubble background color opaque In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces the status bubble use a opaque version of the toolbar_color (alpha value = 0xff). It worked before because the can_use_lcd_text property of status bubble is set before we compute layer draw properties, and the property is disabled all the time. But now we compute layer draw properties on demand, the can_use_lcd_text property becomes true as the whole layer tree is set to be able to use lcd text. BUG=618278 ==========
On 2016/06/21 21:34:10, Peter Kasting wrote: > On 2016/06/21 13:49:54, sunxd wrote: > > On 2016/06/20 20:11:41, Peter Kasting wrote: > > > On 2016/06/20 14:51:23, sunxd wrote: > > > > On 2016/06/13 21:08:51, Peter Kasting wrote: > > > > > The toolbar itself doesn't seem to be transparent; why are we respecting > > the > > > > > transparency request in the status bubble? > > > > > > > > > > Seems like instead the browser should simply ignore the alpha value on > > this > > > > > color. > > > > > > > > If you open a webpage with a dark background, the status bubble is partly > > > > transparent, > > > > this make me think probably the theme is trying to use a transparent > toolbar > > > > color > > > > intentionally. > > > > > > That doesn't falsify what I said. We can generally assume themes do things > > > intentionally, but I don't think we should respect transparency requests in > > the > > > toolbar color no matter what. The theme code should force that color to > > opaque > > > in the getter. > > > > Yeah, I think so too. But I think probably this CL should only concentrate on > > bug > > fix, and maybe have another one that involves feature change, as people might > > want > > to argue on it. > > The workaround in this patch is not necessary if we fix the bug properly. We > want to avoid bandaids in Chromium code, so we don't want to land the patch here > at all. So this CL should be changed to simply force the opacity to 100% to fix > the bug correctly. OK, so I'll make that change here.
https://codereview.chromium.org/2044223006/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2044223006/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:377: // bubble. Changing things here only fixes the status bubble. Shouldn't we be changing this in the theme provider code?
Description was changed from ========== Make status bubble background color opaque In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces the status bubble use a opaque version of the toolbar_color (alpha value = 0xff). It worked before because the can_use_lcd_text property of status bubble is set before we compute layer draw properties, and the property is disabled all the time. But now we compute layer draw properties on demand, the can_use_lcd_text property becomes true as the whole layer tree is set to be able to use lcd text. BUG=618278 ========== to ========== Make toolbar color opaque in browser theme In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces the color to be opaque (alpha value = 0xff). It worked before because the can_use_lcd_text property of status bubble is set before we compute layer draw properties, and the property is disabled all the time. But now we compute layer draw properties on demand, the can_use_lcd_text property becomes true as the whole layer tree is set to be able to use lcd text. BUG=618278 ==========
On 2016/06/22 05:02:30, Peter Kasting (OOO) wrote: > https://codereview.chromium.org/2044223006/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/status_bubble_views.cc (right): > > https://codereview.chromium.org/2044223006/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/status_bubble_views.cc:377: // bubble. > Changing things here only fixes the status bubble. Shouldn't we be changing > this in the theme provider code? It seems BrowserThemePackTest.SupportsAlpha failed on the trybots because we no longer support alpha of toolbar color, should I change the expected result of that?
Yes, you will need to fix the test by changing the SupportsAlpha case to expect that the toolbar color has been forced to 255 alpha. (Add a comment that we do this intentionally.) https://codereview.chromium.org/2044223006/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2044223006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:377: // bubble. This comment is no longer necessary.
https://codereview.chromium.org/2044223006/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2044223006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:377: // bubble. On 2016/06/29 23:25:36, Peter Kasting (slow) wrote: > This comment is no longer necessary. Done.
LGTM https://codereview.chromium.org/2044223006/diff/100001/chrome/browser/themes/... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/2044223006/diff/100001/chrome/browser/themes/... chrome/browser/themes/browser_theme_pack.cc:794: // toolbar color, see crbug.com/618278. Nit: Avoid referring to bugs in comments, and just give sufficient contet inline; something like "Ignore alpha for the toolbar color, as we don't want to allow transparent toolbars." https://codereview.chromium.org/2044223006/diff/100001/chrome/browser/themes/... chrome/browser/themes/browser_theme_pack.cc:797: SkColorGetG(*color), SkColorGetB(*color)); Nit: Just: *color = SkColorSetA(*color, SK_AlphaOPAQUE); https://codereview.chromium.org/2044223006/diff/100001/chrome/browser/themes/... File chrome/browser/themes/browser_theme_pack_unittest.cc (right): https://codereview.chromium.org/2044223006/diff/100001/chrome/browser/themes/... chrome/browser/themes/browser_theme_pack_unittest.cc:438: // The toolbar color's alpha value is intentionally ignored by theme provider. Nit: I would place this comment below, just above the colors[ThemeProperties::COLOR_TOOLBAR] assignment.
The CQ bit was checked by sunxd@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: linux_chromium_chromeos_ozone_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 sunxd@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 sunxd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, msw@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2044223006/#ps140001 (title: "Fix compile failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make toolbar color opaque in browser theme In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces the color to be opaque (alpha value = 0xff). It worked before because the can_use_lcd_text property of status bubble is set before we compute layer draw properties, and the property is disabled all the time. But now we compute layer draw properties on demand, the can_use_lcd_text property becomes true as the whole layer tree is set to be able to use lcd text. BUG=618278 ========== to ========== Make toolbar color opaque in browser theme In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces the color to be opaque (alpha value = 0xff). It worked before because the can_use_lcd_text property of status bubble is set before we compute layer draw properties, and the property is disabled all the time. But now we compute layer draw properties on demand, the can_use_lcd_text property becomes true as the whole layer tree is set to be able to use lcd text. BUG=618278 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Make toolbar color opaque in browser theme In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces the color to be opaque (alpha value = 0xff). It worked before because the can_use_lcd_text property of status bubble is set before we compute layer draw properties, and the property is disabled all the time. But now we compute layer draw properties on demand, the can_use_lcd_text property becomes true as the whole layer tree is set to be able to use lcd text. BUG=618278 ========== to ========== Make toolbar color opaque in browser theme In the bug, the ui assumes using lcd_text by default, but the theme has transparent toolbar_color, rendering subpixel makes the text seem weird. This CL forces the color to be opaque (alpha value = 0xff). It worked before because the can_use_lcd_text property of status bubble is set before we compute layer draw properties, and the property is disabled all the time. But now we compute layer draw properties on demand, the can_use_lcd_text property becomes true as the whole layer tree is set to be able to use lcd text. BUG=618278 Committed: https://crrev.com/32c538f0e6cd40ebd53f336f1f3fae7eb404070b Cr-Commit-Position: refs/heads/master@{#403593} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/32c538f0e6cd40ebd53f336f1f3fae7eb404070b Cr-Commit-Position: refs/heads/master@{#403593} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
