Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541873004:
Change default frame colors. Change custom titlebar colors on Windows. (Closed)
DescriptionChange default frame colors. Change custom titlebar colors on Windows.
Default frame colors are changed so that inactive frames are always a
blend of 80% white/20% active color to increase contrast.
* The normal/inactive frame is changed to #F6F6F6 on all platforms. (OSX
was already at this color.)
* The incognito/inactive frame is changed to #D4D5D5 on all platforms
except OSX.
* The tints applied to theme images are changed to match the new
lightness ratio as well. Inactive tint is changed to 80% lightness.
Incognito/inactive is changed to be 74% lightness, which approximates
blending 80% white on top of the active tint's 30% black. Also
increased saturation change of incognito/inactive to match
incognito/active.
Windows will now pick up the user colors specified by the system and use
them when custom drawing the titlebar. It will never draw the titlebar
white unless the user explicitly specifies that color. It uses an 80%
white/20% active blend to calculate inactive frame colors if the user
has no frame color specified, just like above.
BUG=505013, 645682
Committed: https://crrev.com/5afcb514af8ccd26ef17078957483f1be72bc592
Cr-Commit-Position: refs/heads/master@{#439030}
Patch Set 1 #
Total comments: 19
Patch Set 2 : comments 1 #Patch Set 3 : comments 2 #Patch Set 4 : final osx behavior #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== Change default frame colors. Change custom titlebar colors on Windows. Default frame colors are changed so that inactive frames are always a blend of 80% white/20% active color to increase contrast. * The normal/inactive frame is changed to #F6F6F6 on all platforms. (OSX was already at this color.) * The incognito/inactive frame is changed to #D4D5D5 on all platforms except OSX. * The tints applied to theme images are changed to match the new lightness ratio as well. Inactive tint is changed to 80% lightness. Incognito/inactive is changed to be 74% lightness, which is the blend of 80% white on top of the active tint's 30% black. Also increased saturation change of incognito/inactive to match incognito/active. Windows will now pick up the user colors specified by the system and use them when custom drawing the titlebar. It will never draw the titlebar white unless the user explicitly specifies that color. It uses an 80% white/20% active blend to calculate inactive frame colors if the user has no frame color specified, just like above. BUG=505013,645682 ========== to ========== Change default frame colors. Change custom titlebar colors on Windows. Default frame colors are changed so that inactive frames are always a blend of 80% white/20% active color to increase contrast. * The normal/inactive frame is changed to #F6F6F6 on all platforms. (OSX was already at this color.) * The incognito/inactive frame is changed to #D4D5D5 on all platforms except OSX. * The tints applied to theme images are changed to match the new lightness ratio as well. Inactive tint is changed to 80% lightness. Incognito/inactive is changed to be 74% lightness, which is the blend of 80% white on top of the active tint's 30% black. Also increased saturation change of incognito/inactive to match incognito/active. Windows will now pick up the user colors specified by the system and use them when custom drawing the titlebar. It will never draw the titlebar white unless the user explicitly specifies that color. It uses an 80% white/20% active blend to calculate inactive frame colors if the user has no frame color specified, just like above. BUG=505013,645682 ==========
bsep@chromium.org changed reviewers: + estade@chromium.org, pkasting@chromium.org
The CQ bit was checked by bsep@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 ========== Change default frame colors. Change custom titlebar colors on Windows. Default frame colors are changed so that inactive frames are always a blend of 80% white/20% active color to increase contrast. * The normal/inactive frame is changed to #F6F6F6 on all platforms. (OSX was already at this color.) * The incognito/inactive frame is changed to #D4D5D5 on all platforms except OSX. * The tints applied to theme images are changed to match the new lightness ratio as well. Inactive tint is changed to 80% lightness. Incognito/inactive is changed to be 74% lightness, which is the blend of 80% white on top of the active tint's 30% black. Also increased saturation change of incognito/inactive to match incognito/active. Windows will now pick up the user colors specified by the system and use them when custom drawing the titlebar. It will never draw the titlebar white unless the user explicitly specifies that color. It uses an 80% white/20% active blend to calculate inactive frame colors if the user has no frame color specified, just like above. BUG=505013,645682 ========== to ========== Change default frame colors. Change custom titlebar colors on Windows. Default frame colors are changed so that inactive frames are always a blend of 80% white/20% active color to increase contrast. * The normal/inactive frame is changed to #F6F6F6 on all platforms. (OSX was already at this color.) * The incognito/inactive frame is changed to #D4D5D5 on all platforms except OSX. * The tints applied to theme images are changed to match the new lightness ratio as well. Inactive tint is changed to 80% lightness. Incognito/inactive is changed to be 74% lightness, which approximates blending 80% white on top of the active tint's 30% black. Also increased saturation change of incognito/inactive to match incognito/active. Windows will now pick up the user colors specified by the system and use them when custom drawing the titlebar. It will never draw the titlebar white unless the user explicitly specifies that color. It uses an 80% white/20% active blend to calculate inactive frame colors if the user has no frame color specified, just like above. BUG=505013,645682 ==========
Theme changes look pretty straight-forward and lgtm as I assume some designer has approved screenshots. I'll leave Peter to comment on the Windows specific code. Does this affect crbug.com/643912? The users on that bug would surely be glad to know their concerns are being addressed. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:37: SkColorSetRGB(0xD4, 0xD5, 0xD5); seems a little odd that this is ever so slightly not grey (i.e. the R is off by 1)
It sort of affects the inactive tab contrast... making the titlebar not white will indirectly help but that's a ways from being done. But definitely won't directly help. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:37: SkColorSetRGB(0xD4, 0xD5, 0xD5); On 2016/11/30 23:27:48, Evan Stade wrote: > seems a little odd that this is ever so slightly not grey (i.e. the R is off by > 1) Yes, I agree it's odd. I decided to leave it because the non-inactive color is also slightly-not-grey and so this hints that it's a blend of that. I could make it perfect-grey if you think it's too odd. Or I could cheat and exaggerate it slightly to better preserve the active hue.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/30 23:39:30, Bret Sepulveda wrote: > It sort of affects the inactive tab contrast... making the titlebar not white > will indirectly help but that's a ways from being done. But definitely won't > directly help. > > https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... > File chrome/browser/themes/theme_properties.cc (right): > > https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... > chrome/browser/themes/theme_properties.cc:37: SkColorSetRGB(0xD4, 0xD5, 0xD5); > On 2016/11/30 23:27:48, Evan Stade wrote: > > seems a little odd that this is ever so slightly not grey (i.e. the R is off > by > > 1) > > Yes, I agree it's odd. I decided to leave it because the non-inactive color is > also slightly-not-grey and so this hints that it's a blend of that. I could make > it perfect-grey if you think it's too odd. Or I could cheat and exaggerate it > slightly to better preserve the active hue. I trust whatever a designer thinks looks good, and if they have no opinion I'd tend towards using a grey.
https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:27: const SkColor kDefaultColorFrameInactive = SkColorSetRGB(0xF6, 0xF6, 0xF6); Shouldn't this be 0xF5? 0xCC = 204 204 + (255 - 204) * 0.8 = 244.8 245 = 0xF5 Better yet, can we kill kDefaultColorFrameInactive, kDefaultColorFrameIncognito, and kDefaultColorFrameIncognitoInactive in favor of computing these using the shifts?? https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:33: SkColorSetRGB(0x1E, 0x1E, 0x1E); It worries me that we're leaving the Mac incognito colors unchanged, but we're changing the tints for all platforms. This seems inconsistent. SHould the tints also be different on Mac? Or are we OK with the default incognito window acting completely different than a themed incognito window there? https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:37: SkColorSetRGB(0xD4, 0xD5, 0xD5); On 2016/11/30 23:39:30, Bret Sepulveda wrote: > On 2016/11/30 23:27:48, Evan Stade wrote: > > seems a little odd that this is ever so slightly not grey (i.e. the R is off > by > > 1) > > Yes, I agree it's odd. I decided to leave it because the non-inactive color is > also slightly-not-grey and so this hints that it's a blend of that. I could make > it perfect-grey if you think it's too odd. Or I could cheat and exaggerate it > slightly to better preserve the active hue. Let's leave it as #D4D5D5. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_service_win.cc (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.cc:80: color_utils::AlphaBlend(dwm_frame_color_, SK_ColorWHITE, 0x33); Nit: If you wint up preserving this, consider shifting by TINT_FRAME_INACTIVE instead. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_service_win.h (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.h:13: // are relevant to earlier versions of Windows. Should this be renamed ThemeServiceWin10 then? Not for this patch: can we also respect user-set frame colors on Win 7/8? Presumably the only difference is getting the right values to begin with. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.h:37: bool use_dwm_frame_color_; Maybe instead of this bool, use a base::Optional<SkColor> for the two colors below. This will also let us avoid needing to compute the inactive frame color in the .cc file when the active frame color is set but the inactive color is not, as these two will be tested individually.
https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:27: const SkColor kDefaultColorFrameInactive = SkColorSetRGB(0xF6, 0xF6, 0xF6); On 2016/12/01 06:19:04, Peter Kasting wrote: > Shouldn't this be 0xF5? > > 0xCC = 204 > 204 + (255 - 204) * 0.8 = 244.8 > 245 = 0xF5 > > Better yet, can we kill kDefaultColorFrameInactive, kDefaultColorFrameIncognito, > and kDefaultColorFrameIncognitoInactive in favor of computing these using the > shifts?? Yes it's supposed to be F5, but I thought a tiny shift towards white was worth eliminating a preprocessor block. I put it back tho. As for eliminating the pre-computed colors, I'm not sure. Since I'm not really sure how the OSX colors are determined and whether we can change them I'm inclined to just make the minimal change for now. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:33: SkColorSetRGB(0x1E, 0x1E, 0x1E); On 2016/12/01 06:19:04, Peter Kasting wrote: > It worries me that we're leaving the Mac incognito colors unchanged, but we're > changing the tints for all platforms. This seems inconsistent. SHould the > tints also be different on Mac? Or are we OK with the default incognito window > acting completely different than a themed incognito window there? That's a good point. I added some new switches to keep the old tints on OSX. Based on the colors that are here it already works differently between default and themed... but again I'd rather not change it without more context. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_service_win.cc (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.cc:80: color_utils::AlphaBlend(dwm_frame_color_, SK_ColorWHITE, 0x33); On 2016/12/01 06:19:04, Peter Kasting wrote: > Nit: If you wint up preserving this, consider shifting by TINT_FRAME_INACTIVE > instead. Done. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_service_win.h (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.h:13: // are relevant to earlier versions of Windows. On 2016/12/01 06:19:04, Peter Kasting wrote: > Should this be renamed ThemeServiceWin10 then? > > Not for this patch: can we also respect user-set frame colors on Win 7/8? > Presumably the only difference is getting the right values to begin with. I think the name is okay, since it's an OS-specific override of a cross-platform base class and this is the convention. We could get the values on 7/8, it's just a question of what we'd do with them. For 7 it'd only be relevant to basic mode. For 8 we could custom draw the titlebar there too but I doubt it's worth the effort. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.h:37: bool use_dwm_frame_color_; On 2016/12/01 06:19:04, Peter Kasting wrote: > Maybe instead of this bool, use a base::Optional<SkColor> for the two colors > below. > > This will also let us avoid needing to compute the inactive frame color in the > .cc file when the active frame color is set but the inactive color is not, as > these two will be tested individually. I don't quite understand what you mean here. Do you want to compute the inactive frame color on the fly somewhere else? Either way we have to compute it at some point. I kind of like the bool value because we're using either both colors or neither color, there's not really a case where we only want one, unless we're computing the inactive color on the fly.
https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:27: const SkColor kDefaultColorFrameInactive = SkColorSetRGB(0xF6, 0xF6, 0xF6); On 2016/12/08 00:16:07, Bret Sepulveda wrote: > On 2016/12/01 06:19:04, Peter Kasting wrote: > > Shouldn't this be 0xF5? > > > > 0xCC = 204 > > 204 + (255 - 204) * 0.8 = 244.8 > > 245 = 0xF5 > > > > Better yet, can we kill kDefaultColorFrameInactive, > kDefaultColorFrameIncognito, > > and kDefaultColorFrameIncognitoInactive in favor of computing these using the > > shifts?? > > Yes it's supposed to be F5, but I thought a tiny shift towards white was worth > eliminating a preprocessor block. Oh, my intent was to just change the OS X color too. (I doubt that change will be noticed by anyone...) > Since I'm not really > sure how the OSX colors are determined and whether we can change them I'm > inclined to just make the minimal change for now. I'd ask bettes@ about this stuff. Maybe we will decide to keep the current behavior, at least for now, but I'd like to at least understand what the context and the long-term plan is. Perhaps we could do something like, leave Mac as returning hardcoded colors, and have non-Mac compute these colors based on the tints. Then in the future we can try to remove the Mac #ifs if possible. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_service_win.h (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.h:13: // are relevant to earlier versions of Windows. On 2016/12/08 00:16:07, Bret Sepulveda wrote: > On 2016/12/01 06:19:04, Peter Kasting wrote: > > Should this be renamed ThemeServiceWin10 then? > > > > Not for this patch: can we also respect user-set frame colors on Win 7/8? > > Presumably the only difference is getting the right values to begin with. > > I think the name is okay, since it's an OS-specific override of a cross-platform > base class and this is the convention. > > We could get the values on 7/8, it's just a question of what we'd do with them. > For 7 it'd only be relevant to basic mode. For 8 we could custom draw the > titlebar there too but I doubt it's worth the effort. Fixing Win 7 basic seems worth doing; that's used by various accessibility folks, and technically it seems like it should be easy. For Win 8, I wouldn't bother unless Win 8 titlebars are close enough to Win 10 that we only need minor tweaks to custom-draw them. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.h:37: bool use_dwm_frame_color_; On 2016/12/08 00:16:07, Bret Sepulveda wrote: > On 2016/12/01 06:19:04, Peter Kasting wrote: > > Maybe instead of this bool, use a base::Optional<SkColor> for the two colors > > below. > > > > This will also let us avoid needing to compute the inactive frame color in the > > .cc file when the active frame color is set but the inactive color is not, as > > these two will be tested individually. > > I don't quite understand what you mean here. Do you want to compute the inactive > frame color on the fly somewhere else? Either way we have to compute it at some > point. > > I kind of like the bool value because we're using either both colors or neither > color, there's not really a case where we only want one, unless we're computing > the inactive color on the fly. My intent was that this class is only used to report user-set colors, and that computing the inactive frame color from the active one be done elsewhere. The motivation is that we already need to do that computation for theme images, and I'm hoping to also do it for the default (unthemed) colors, to replace most of the hardcoded colors (see comments in theme_properties.cc). So I'm looking to have all this happen in one spot, basically.
https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_properties.cc:27: const SkColor kDefaultColorFrameInactive = SkColorSetRGB(0xF6, 0xF6, 0xF6); On 2016/12/08 00:24:50, Peter Kasting wrote: > On 2016/12/08 00:16:07, Bret Sepulveda wrote: > > On 2016/12/01 06:19:04, Peter Kasting wrote: > > > Shouldn't this be 0xF5? > > > > > > 0xCC = 204 > > > 204 + (255 - 204) * 0.8 = 244.8 > > > 245 = 0xF5 > > > > > > Better yet, can we kill kDefaultColorFrameInactive, > > kDefaultColorFrameIncognito, > > > and kDefaultColorFrameIncognitoInactive in favor of computing these using > the > > > shifts?? > > > > Yes it's supposed to be F5, but I thought a tiny shift towards white was worth > > eliminating a preprocessor block. > > Oh, my intent was to just change the OS X color too. (I doubt that change will > be noticed by anyone...) > > > Since I'm not really > > sure how the OSX colors are determined and whether we can change them I'm > > inclined to just make the minimal change for now. > > I'd ask bettes@ about this stuff. Maybe we will decide to keep the current > behavior, at least for now, but I'd like to at least understand what the context > and the long-term plan is. > > Perhaps we could do something like, leave Mac as returning hardcoded colors, and > have non-Mac compute these colors based on the tints. Then in the future we can > try to remove the Mac #ifs if possible. Ok, changed to F5. I'll find out what we want to do for OSX. For this patch I'll make sure we don't change anything and come back to it. https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_service_win.h (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.h:37: bool use_dwm_frame_color_; On 2016/12/08 00:24:50, Peter Kasting wrote: > On 2016/12/08 00:16:07, Bret Sepulveda wrote: > > On 2016/12/01 06:19:04, Peter Kasting wrote: > > > Maybe instead of this bool, use a base::Optional<SkColor> for the two colors > > > below. > > > > > > This will also let us avoid needing to compute the inactive frame color in > the > > > .cc file when the active frame color is set but the inactive color is not, > as > > > these two will be tested individually. > > > > I don't quite understand what you mean here. Do you want to compute the > inactive > > frame color on the fly somewhere else? Either way we have to compute it at > some > > point. > > > > I kind of like the bool value because we're using either both colors or > neither > > color, there's not really a case where we only want one, unless we're > computing > > the inactive color on the fly. > > My intent was that this class is only used to report user-set colors, and that > computing the inactive frame color from the active one be done elsewhere. > > The motivation is that we already need to do that computation for theme images, > and I'm hoping to also do it for the default (unthemed) colors, to replace most > of the hardcoded colors (see comments in theme_properties.cc). So I'm looking > to have all this happen in one spot, basically. Oh okay I see what you mean now. I don't think the object hierarchy will do exactly what you want. ThemeProperties is the last link in the chain, making it hard to pass the ThemeServiceWin values "backwards" to ThemeProperties. So if you want to eliminate the pre-computed defaults we'd have to do the inactive color calculation in two places.
LGTM https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_service_win.h (right): https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service_win.h:37: bool use_dwm_frame_color_; On 2016/12/08 02:16:59, Bret Sepulveda wrote: > On 2016/12/08 00:24:50, Peter Kasting wrote: > > On 2016/12/08 00:16:07, Bret Sepulveda wrote: > > > On 2016/12/01 06:19:04, Peter Kasting wrote: > > > > Maybe instead of this bool, use a base::Optional<SkColor> for the two > colors > > > > below. > > > > > > > > This will also let us avoid needing to compute the inactive frame color in > > the > > > > .cc file when the active frame color is set but the inactive color is not, > > as > > > > these two will be tested individually. > > > > > > I don't quite understand what you mean here. Do you want to compute the > > inactive > > > frame color on the fly somewhere else? Either way we have to compute it at > > some > > > point. > > > > > > I kind of like the bool value because we're using either both colors or > > neither > > > color, there's not really a case where we only want one, unless we're > > computing > > > the inactive color on the fly. > > > > My intent was that this class is only used to report user-set colors, and that > > computing the inactive frame color from the active one be done elsewhere. > > > > The motivation is that we already need to do that computation for theme > images, > > and I'm hoping to also do it for the default (unthemed) colors, to replace > most > > of the hardcoded colors (see comments in theme_properties.cc). So I'm looking > > to have all this happen in one spot, basically. > > Oh okay I see what you mean now. I don't think the object hierarchy will do > exactly what you want. ThemeProperties is the last link in the chain, making it > hard to pass the ThemeServiceWin values "backwards" to ThemeProperties. So if > you want to eliminate the pre-computed defaults we'd have to do the inactive > color calculation in two places. Hmm. I know we'd probably have to write some similar code in ThemeService and ThemeProperties (to use the maybe-themed tints/colors and base colors, respectively), but it seems like ultimately both would boil down to something like: return color_utils::HSLShift(frame_color, GetTint(inactive_tint, incognito)); ...which seems like a better place to be than doing a direct AlphaBlend() with 0x33 in the code here. (Without trying to implement this, it's not clear to me how much further this can be simplified. A single helper function that both ThemeProperties and ThemeService can call?) I have said my piece on how I think this should work, I will leave it to you to decide what should be implemented in this CL, what should be implemented as followups, and what's not worth worrying about.
On 2016/12/08 04:59:47, Peter Kasting wrote: > LGTM > > https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... > File chrome/browser/themes/theme_service_win.h (right): > > https://codereview.chromium.org/2541873004/diff/1/chrome/browser/themes/theme... > chrome/browser/themes/theme_service_win.h:37: bool use_dwm_frame_color_; > On 2016/12/08 02:16:59, Bret Sepulveda wrote: > > On 2016/12/08 00:24:50, Peter Kasting wrote: > > > On 2016/12/08 00:16:07, Bret Sepulveda wrote: > > > > On 2016/12/01 06:19:04, Peter Kasting wrote: > > > > > Maybe instead of this bool, use a base::Optional<SkColor> for the two > > colors > > > > > below. > > > > > > > > > > This will also let us avoid needing to compute the inactive frame color > in > > > the > > > > > .cc file when the active frame color is set but the inactive color is > not, > > > as > > > > > these two will be tested individually. > > > > > > > > I don't quite understand what you mean here. Do you want to compute the > > > inactive > > > > frame color on the fly somewhere else? Either way we have to compute it at > > > some > > > > point. > > > > > > > > I kind of like the bool value because we're using either both colors or > > > neither > > > > color, there's not really a case where we only want one, unless we're > > > computing > > > > the inactive color on the fly. > > > > > > My intent was that this class is only used to report user-set colors, and > that > > > computing the inactive frame color from the active one be done elsewhere. > > > > > > The motivation is that we already need to do that computation for theme > > images, > > > and I'm hoping to also do it for the default (unthemed) colors, to replace > > most > > > of the hardcoded colors (see comments in theme_properties.cc). So I'm > looking > > > to have all this happen in one spot, basically. > > > > Oh okay I see what you mean now. I don't think the object hierarchy will do > > exactly what you want. ThemeProperties is the last link in the chain, making > it > > hard to pass the ThemeServiceWin values "backwards" to ThemeProperties. So if > > you want to eliminate the pre-computed defaults we'd have to do the inactive > > color calculation in two places. > > Hmm. I know we'd probably have to write some similar code in ThemeService and > ThemeProperties (to use the maybe-themed tints/colors and base colors, > respectively), but it seems like ultimately both would boil down to something > like: > > return color_utils::HSLShift(frame_color, GetTint(inactive_tint, incognito)); > > ...which seems like a better place to be than doing a direct AlphaBlend() with > 0x33 in the code here. > > (Without trying to implement this, it's not clear to me how much further this > can be simplified. A single helper function that both ThemeProperties and > ThemeService can call?) > > I have said my piece on how I think this should work, I will leave it to you to > decide what should be implemented in this CL, what should be implemented as > followups, and what's not worth worrying about. FYI: I discussed with designers and we don't want to change OSX's frame color, but we can change the tints. So I removed all OSX-specific code except for the incognito colors (since the normal-mode colors are now the same) and computed the default inactive colors based on the default active color and default tint, like Peter suggested. I trust this is non-controversial enough to not require a re-review, so I will submit it.
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2541873004/#ps60001 (title: "final osx behavior")
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": 60001, "attempt_start_ts": 1481854820830280,
"parent_rev": "b3885566f1de70df5d11ebe71bac72fbdf48a9c0", "commit_rev":
"7f28b5a26f5425e7b0c8d32a269542dd069c9827"}
Message was sent while issue was closed.
Description was changed from ========== Change default frame colors. Change custom titlebar colors on Windows. Default frame colors are changed so that inactive frames are always a blend of 80% white/20% active color to increase contrast. * The normal/inactive frame is changed to #F6F6F6 on all platforms. (OSX was already at this color.) * The incognito/inactive frame is changed to #D4D5D5 on all platforms except OSX. * The tints applied to theme images are changed to match the new lightness ratio as well. Inactive tint is changed to 80% lightness. Incognito/inactive is changed to be 74% lightness, which approximates blending 80% white on top of the active tint's 30% black. Also increased saturation change of incognito/inactive to match incognito/active. Windows will now pick up the user colors specified by the system and use them when custom drawing the titlebar. It will never draw the titlebar white unless the user explicitly specifies that color. It uses an 80% white/20% active blend to calculate inactive frame colors if the user has no frame color specified, just like above. BUG=505013,645682 ========== to ========== Change default frame colors. Change custom titlebar colors on Windows. Default frame colors are changed so that inactive frames are always a blend of 80% white/20% active color to increase contrast. * The normal/inactive frame is changed to #F6F6F6 on all platforms. (OSX was already at this color.) * The incognito/inactive frame is changed to #D4D5D5 on all platforms except OSX. * The tints applied to theme images are changed to match the new lightness ratio as well. Inactive tint is changed to 80% lightness. Incognito/inactive is changed to be 74% lightness, which approximates blending 80% white on top of the active tint's 30% black. Also increased saturation change of incognito/inactive to match incognito/active. Windows will now pick up the user colors specified by the system and use them when custom drawing the titlebar. It will never draw the titlebar white unless the user explicitly specifies that color. It uses an 80% white/20% active blend to calculate inactive frame colors if the user has no frame color specified, just like above. BUG=505013,645682 Review-Url: https://codereview.chromium.org/2541873004 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Change default frame colors. Change custom titlebar colors on Windows. Default frame colors are changed so that inactive frames are always a blend of 80% white/20% active color to increase contrast. * The normal/inactive frame is changed to #F6F6F6 on all platforms. (OSX was already at this color.) * The incognito/inactive frame is changed to #D4D5D5 on all platforms except OSX. * The tints applied to theme images are changed to match the new lightness ratio as well. Inactive tint is changed to 80% lightness. Incognito/inactive is changed to be 74% lightness, which approximates blending 80% white on top of the active tint's 30% black. Also increased saturation change of incognito/inactive to match incognito/active. Windows will now pick up the user colors specified by the system and use them when custom drawing the titlebar. It will never draw the titlebar white unless the user explicitly specifies that color. It uses an 80% white/20% active blend to calculate inactive frame colors if the user has no frame color specified, just like above. BUG=505013,645682 Review-Url: https://codereview.chromium.org/2541873004 ========== to ========== Change default frame colors. Change custom titlebar colors on Windows. Default frame colors are changed so that inactive frames are always a blend of 80% white/20% active color to increase contrast. * The normal/inactive frame is changed to #F6F6F6 on all platforms. (OSX was already at this color.) * The incognito/inactive frame is changed to #D4D5D5 on all platforms except OSX. * The tints applied to theme images are changed to match the new lightness ratio as well. Inactive tint is changed to 80% lightness. Incognito/inactive is changed to be 74% lightness, which approximates blending 80% white on top of the active tint's 30% black. Also increased saturation change of incognito/inactive to match incognito/active. Windows will now pick up the user colors specified by the system and use them when custom drawing the titlebar. It will never draw the titlebar white unless the user explicitly specifies that color. It uses an 80% white/20% active blend to calculate inactive frame colors if the user has no frame color specified, just like above. BUG=505013,645682 Committed: https://crrev.com/5afcb514af8ccd26ef17078957483f1be72bc592 Cr-Commit-Position: refs/heads/master@{#439030} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5afcb514af8ccd26ef17078957483f1be72bc592 Cr-Commit-Position: refs/heads/master@{#439030} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
