Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2051123002:
Make COLOR_CONTROL_BACKGROUND customizable from a theme extension (Closed)
|
Created:
4 years, 6 months ago by Julien Isorce Samsung Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake COLOR_CONTROL_BACKGROUND customizable from a theme extension
Since https://codereview.chromium.org/63173016/patch/430001/440015
it is not possible to customize the background of BrowserView's
contents container. Indeed it was previously relying on COLOR_TOOLBAR
and that change made it rely on COLOR_CONTROL_BACKGROUND.
BUG=618335
R=pkasting@chromium.org
TEST=./out/Release/unit_tests --gtest_filter=*CanBuildAndReadPack*
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase and increase kThemePackVersion #
Messages
Total messages: 26 (6 generated)
Description was changed from ========== Make COLOR_CONTROL_BACKGROUND customizable from a theme extension Since https://codereview.chromium.org/63173016/patch/430001/440015 it is not possible to customize the background of BrowserView's contents container. Indeed it was previously relying on COLOR_TOOLBAR and that change made it rely on COLOR_CONTROL_BACKGROUND. BUG=618335 R=pkasting@chromium.org TEST=./out/Release/unit_tests --gtest_filter=*CanBuildAndReadPack* ========== to ========== Make COLOR_CONTROL_BACKGROUND customizable from a theme extension Since https://codereview.chromium.org/63173016/patch/430001/440015 it is not possible to customize the background of BrowserView's contents container. Indeed it was previously relying on COLOR_TOOLBAR and that change made it rely on COLOR_CONTROL_BACKGROUND. BUG=618335 R=pkasting@chromium.org TEST=./out/Release/unit_tests --gtest_filter=*CanBuildAndReadPack* ==========
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051123002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/09 16:34:16, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Hi, plz take a look. Thx.
Did you find out why the relevant color was changed away from COLOR_TOOLBAR? I wasn't involved with the CL that caused this, so I'm not in a position to understand why this was changed, whether it makes sense to fix this, and if so how. https://codereview.chromium.org/2051123002/diff/1/chrome/browser/themes/brows... File chrome/browser/themes/browser_theme_pack_unittest.cc (left): https://codereview.chromium.org/2051123002/diff/1/chrome/browser/themes/brows... chrome/browser/themes/browser_theme_pack_unittest.cc:250: // Make sure we don't have phantom data. Why did this comment disappear? https://codereview.chromium.org/2051123002/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.h (right): https://codereview.chromium.org/2051123002/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.h:28: COLOR_CONTROL_BACKGROUND, If you touch the elements of this enum you need to modify the version number; see the comment above
On 2016/06/10 01:12:25, Peter Kasting wrote: > Did you find out why the relevant color was changed away from COLOR_TOOLBAR? I > wasn't involved with the CL that caused this, so I'm not in a position to > understand why this was changed, whether it makes sense to fix this, and if so > how. I vaguely remember setting this color to probably match the background of SingleSplitView, which was used there at the time. My patch introduced new visible surface (just plain background color only visible when resizing with DevTools open) and I tried to match what was around.
On 2016/06/10 05:35:52, dgozman_slow wrote: > On 2016/06/10 01:12:25, Peter Kasting wrote: > > Did you find out why the relevant color was changed away from COLOR_TOOLBAR? > I > > wasn't involved with the CL that caused this, so I'm not in a position to > > understand why this was changed, whether it makes sense to fix this, and if so > > how. > > I vaguely remember setting this color to probably match the background of > SingleSplitView, which was used there at the time. My patch introduced new > visible surface (just plain background color only visible when resizing with > DevTools open) and I tried to match what was around. What would be the effect of going back to the toolbar color like was used before? Would that look bad? I'm more inclined to make that change than this one, if it makes sense.
On 2016/06/10 05:42:09, Peter Kasting wrote: > On 2016/06/10 05:35:52, dgozman_slow wrote: > > On 2016/06/10 01:12:25, Peter Kasting wrote: > > > Did you find out why the relevant color was changed away from COLOR_TOOLBAR? > > > I > > > wasn't involved with the CL that caused this, so I'm not in a position to > > > understand why this was changed, whether it makes sense to fix this, and if > so > > > how. > > > > I vaguely remember setting this color to probably match the background of > > SingleSplitView, which was used there at the time. My patch introduced new > > visible surface (just plain background color only visible when resizing with > > DevTools open) and I tried to match what was around. > > What would be the effect of going back to the toolbar color like was used > before? Would that look bad? I'm more inclined to make that change than this > one, if it makes sense. I tried on Linux and I could not see any difference until I started to play with alpha channel in the manifest.json: "toolbar": [200, 40, 90], "ntp_background": [0, 0, 50, 0.1] With this configuration I can see the layer initialized with contents_container_->set_background from BrowserView::InitViews(). If one wants to customize it without changing the color of the tabs on top of the browser then it requires to have access to "control_background". Also there are maybe other cases where this layer becomes visible.
https://codereview.chromium.org/2051123002/diff/1/chrome/browser/themes/brows... File chrome/browser/themes/browser_theme_pack_unittest.cc (left): https://codereview.chromium.org/2051123002/diff/1/chrome/browser/themes/brows... chrome/browser/themes/browser_theme_pack_unittest.cc:250: // Make sure we don't have phantom data. On 2016/06/10 01:12:24, Peter Kasting wrote: > Why did this comment disappear? Oups, I'll add it back. Thx https://codereview.chromium.org/2051123002/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.h (right): https://codereview.chromium.org/2051123002/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.h:28: COLOR_CONTROL_BACKGROUND, On 2016/06/10 01:12:24, Peter Kasting wrote: > If you touch the elements of this enum you need to modify the version number; > see the comment above Done.
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkasting@chromium.org changed reviewers: + estade@chromium.org
COLOR_CONTROL_BACKGROUND defaults to the toolbar color on Linux but to white on other views platforms. I'm not really sure when this color is supposed to be visible. It's the base color of the contents container, but the contents container should generally be showing some contents. It seems like this would primarily have to do with what color would display before any content is loaded, and I thought someone patched that somewhere else recently. The detached bookmark bar paints based on this color, but I don't know if that's right either. I'd think the detached bar would want to match whatever color the NTP background uses? For the built-in NTP that'd be white, but that would mean there's a mismatch on Linux. I suspect I'm misunderstanding how this works. +estade who touched the detached bookmark bar painting recently. It would be worth saying why this color needs to be customizable.
On 2016/06/10 23:56:05, Peter Kasting wrote: > The detached bookmark bar paints based on this color, but I don't know if that's > right either. I'd think the detached bar would want to match whatever color the > NTP background uses? For the built-in NTP that'd be white, but that would mean > there's a mismatch on Linux. It's true that with the gtk theme on linux, the detached bar matches the toolbar (greyish) not the ntp bg (white). However it looks fine imo. Custom themes can actually choose the color of the detached bar using COLOR_DETACHED_BOOKMARK_BAR_BACKGROUND. The custom theme I use (infected mushroom) makes the detached bbar color match the toolbar color. The person who wrote the code for the GTK theme (erg) made a similar decision, it seems. I think we always use COLOR_BOOKMARK_TEXT for the button text, regardless of the attached/detached state. If we made the detached bg match the NTP this could make the bookmark text unreadable (think inverted GTK color scheme for example). So we'd have to also change the detached bookmark text to COLOR_NTP_TEXT, I guess. Anyway, tl;dr the status quo seems fine to me. > > I suspect I'm misunderstanding how this works. +estade who touched the detached > bookmark bar painting recently. > > It would be worth saying why this color needs to be customizable. I agree. I guess if you're creating a dark theme you might not like the big flash of white. But this is primarily visible in between page loads, and most web pages have white bgs, so forcing this to white probably causes the least distraction for typical web usage.
On 2016/06/13 16:20:40, Evan Stade wrote: > On 2016/06/10 23:56:05, Peter Kasting wrote: > > The detached bookmark bar paints based on this color, but I don't know if > that's > > right either. I'd think the detached bar would want to match whatever color > the > > NTP background uses? For the built-in NTP that'd be white, but that would > mean > > there's a mismatch on Linux. > > It's true that with the gtk theme on linux, the detached bar matches the toolbar > (greyish) not the ntp bg (white). However it looks fine imo. Hmm. That was never the design intent. It would make the "detached" bar still look attached, just with a separator. More like an infobar. Logically, it's less clear in that case that the bar should disappear when the page is navigated away from the NTP. We wanted this to be "part of the NTP" so that it should go away when the NTP does. > Custom themes can > actually choose the color of the detached bar using > COLOR_DETACHED_BOOKMARK_BAR_BACKGROUND. Seems like this should go away and we should use COLOR_NTP_BACKGROUND? > So we'd have to also change the detached bookmark text to COLOR_NTP_TEXT, I > guess. Yes. > > It would be worth saying why this color needs to be customizable. > > I agree. I guess if you're creating a dark theme you might not like the big > flash of white. But this is primarily visible in between page loads, and most > web pages have white bgs, so forcing this to white probably causes the least > distraction for typical web usage. Seems like COLOR_CONTROL_BACKGROUND should go away entirely...
On 2016/06/13 18:03:29, Peter Kasting wrote: > Seems like COLOR_CONTROL_BACKGROUND should go away entirely... Interesting discussion here. Then should I have a go to replace all occurrences to COLOR_CONTROL_BACKGROUND by COLOR_NTP_BACKGROUND ?
On 2016/06/13 22:06:31, Julien Isorce wrote: > On 2016/06/13 18:03:29, Peter Kasting wrote: > > Seems like COLOR_CONTROL_BACKGROUND should go away entirely... > > Interesting discussion here. Then should I have a go to replace all > occurrences to COLOR_CONTROL_BACKGROUND by COLOR_NTP_BACKGROUND ? I believe the use in the detached bookmark bar was necessary to preserve legacy behavior for themes that define non-opaque toolbar colors/images. If you switched to COLOR_NTP_BACKGROUND it would break some themes.
On 2016/06/13 22:19:44, Evan Stade wrote: > On 2016/06/13 22:06:31, Julien Isorce wrote: > > On 2016/06/13 18:03:29, Peter Kasting wrote: > > > Seems like COLOR_CONTROL_BACKGROUND should go away entirely... > > > > Interesting discussion here. Then should I have a go to replace all > > occurrences to COLOR_CONTROL_BACKGROUND by COLOR_NTP_BACKGROUND ? Seems like it's only used for the following: * The detached bookmark bar, which should probably use the NTP background and text colors * ConstrainedWindowCustomWindow on Mac, which should probably be hardcoded to white * The contents container background on views, which should maybe be the NTP background color, but I'm unsure > I believe the use in the detached bookmark bar was necessary to preserve legacy > behavior for themes that define non-opaque toolbar colors/images. If you > switched to COLOR_NTP_BACKGROUND it would break some themes. Why aren't we ignoring the alpha value on a transparent toolbar color? It makes no sense for the toolbar to be transparent, we son't draw it that way on Windows (I don't think), and it could screw up various calculations (e.g. the tab stroke color) that assume that color is opaque.
On 2016/06/13 22:47:53, Peter Kasting wrote: > On 2016/06/13 22:19:44, Evan Stade wrote: > > On 2016/06/13 22:06:31, Julien Isorce wrote: > > > On 2016/06/13 18:03:29, Peter Kasting wrote: > > > > Seems like COLOR_CONTROL_BACKGROUND should go away entirely... > > > > > > Interesting discussion here. Then should I have a go to replace all > > > occurrences to COLOR_CONTROL_BACKGROUND by COLOR_NTP_BACKGROUND ? > > Seems like it's only used for the following: > > * The detached bookmark bar, which should probably use the NTP background and > text colors > * ConstrainedWindowCustomWindow on Mac, which should probably be hardcoded to > white > * The contents container background on views, which should maybe be the NTP > background color, but I'm unsure > > > I believe the use in the detached bookmark bar was necessary to preserve > legacy > > behavior for themes that define non-opaque toolbar colors/images. If you > > switched to COLOR_NTP_BACKGROUND it would break some themes. > > Why aren't we ignoring the alpha value on a transparent toolbar color? Technically it's not the toolbar color but COLOR_DETACHED_BOOKMARK_BAR_BACKGROUND > It makes > no sense for the toolbar to be transparent, we son't draw it that way on Windows > (I don't think), and it could screw up various calculations (e.g. the tab stroke > color) that assume that color is opaque. We simply never have, afaik. See https://codereview.chromium.org/1861783002 and the associated bug for a theme that relies on us respecting opacity and painting on top of COLOR_CONTROL_BACKGROUND.
On 2016/06/14 16:12:10, Evan Stade wrote: > On 2016/06/13 22:47:53, Peter Kasting wrote: > > On 2016/06/13 22:19:44, Evan Stade wrote: > > > On 2016/06/13 22:06:31, Julien Isorce wrote: > > > > On 2016/06/13 18:03:29, Peter Kasting wrote: > > > > > Seems like COLOR_CONTROL_BACKGROUND should go away entirely... > > > > > > > > Interesting discussion here. Then should I have a go to replace all > > > > occurrences to COLOR_CONTROL_BACKGROUND by COLOR_NTP_BACKGROUND ? > > > > Seems like it's only used for the following: > > > > * The detached bookmark bar, which should probably use the NTP background and > > text colors > > * ConstrainedWindowCustomWindow on Mac, which should probably be hardcoded to > > white > > * The contents container background on views, which should maybe be the NTP > > background color, but I'm unsure > > > > > I believe the use in the detached bookmark bar was necessary to preserve > > legacy > > > behavior for themes that define non-opaque toolbar colors/images. If you > > > switched to COLOR_NTP_BACKGROUND it would break some themes. > > > > Why aren't we ignoring the alpha value on a transparent toolbar color? > > Technically it's not the toolbar color but > COLOR_DETACHED_BOOKMARK_BAR_BACKGROUND > > > It makes > > no sense for the toolbar to be transparent, we son't draw it that way on > Windows > > (I don't think), and it could screw up various calculations (e.g. the tab > stroke > > color) that assume that color is opaque. > > We simply never have, afaik. See https://codereview.chromium.org/1861783002 and > the associated bug for a theme that relies on us respecting opacity and painting > on top of COLOR_CONTROL_BACKGROUND. I think the right thing to do here is to go ahead and switch the detached bookmark bar to using the NTP background/text colors and eliminate COLOR_DETACHED_BOOKMARK_BAR_BACKGROUND entirely. That avoids any opacity issues. Then we change the ConstrainedWindowCustomWindow to use white and the contents container background to use the NTP background color, and eliminate COLOR_CONTROL_BACKGROUND as well. This will be a behavior change, but I think it's the correct behavior change.
On 2016/06/14 17:23:24, Peter Kasting wrote: > On 2016/06/14 16:12:10, Evan Stade wrote: > > On 2016/06/13 22:47:53, Peter Kasting wrote: > > > On 2016/06/13 22:19:44, Evan Stade wrote: > > > > On 2016/06/13 22:06:31, Julien Isorce wrote: > > > > > On 2016/06/13 18:03:29, Peter Kasting wrote: > > > > > > Seems like COLOR_CONTROL_BACKGROUND should go away entirely... > > > > > > > > > > Interesting discussion here. Then should I have a go to replace all > > > > > occurrences to COLOR_CONTROL_BACKGROUND by COLOR_NTP_BACKGROUND ? > > > > > > Seems like it's only used for the following: > > > > > > * The detached bookmark bar, which should probably use the NTP background > and > > > text colors > > > * ConstrainedWindowCustomWindow on Mac, which should probably be hardcoded > to > > > white > > > * The contents container background on views, which should maybe be the NTP > > > background color, but I'm unsure > > > > > > > I believe the use in the detached bookmark bar was necessary to preserve > > > legacy > > > > behavior for themes that define non-opaque toolbar colors/images. If you > > > > switched to COLOR_NTP_BACKGROUND it would break some themes. > > > > > > Why aren't we ignoring the alpha value on a transparent toolbar color? > > > > Technically it's not the toolbar color but > > COLOR_DETACHED_BOOKMARK_BAR_BACKGROUND > > > > > It makes > > > no sense for the toolbar to be transparent, we son't draw it that way on > > Windows > > > (I don't think), and it could screw up various calculations (e.g. the tab > > stroke > > > color) that assume that color is opaque. > > > > We simply never have, afaik. See https://codereview.chromium.org/1861783002 > and > > the associated bug for a theme that relies on us respecting opacity and > painting > > on top of COLOR_CONTROL_BACKGROUND. > > I think the right thing to do here is to go ahead and switch the detached > bookmark bar to using the NTP background/text colors and eliminate > COLOR_DETACHED_BOOKMARK_BAR_BACKGROUND entirely. That avoids any opacity > issues. Then we change the ConstrainedWindowCustomWindow to use white and the > contents container background to use the NTP background color, and eliminate > COLOR_CONTROL_BACKGROUND as well. > > This will be a behavior change, but I think it's the correct behavior change. Thx for the detailed plan. I'll have a go.
On 2016/06/14 20:13:47, Julien Isorce wrote: > On 2016/06/14 17:23:24, Peter Kasting wrote: > > I think the right thing to do here is to go ahead and switch the detached > > bookmark bar to using the NTP background/text colors and eliminate > > COLOR_DETACHED_BOOKMARK_BAR_BACKGROUND entirely. That avoids any opacity > > issues. Then we change the ConstrainedWindowCustomWindow to use white and the > > contents container background to use the NTP background color, and eliminate > > COLOR_CONTROL_BACKGROUND as well. > > > > This will be a behavior change, but I think it's the correct behavior change. > > Thx for the detailed plan. I'll have a go. Done here: https://codereview.chromium.org/2062353002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
