Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899053002:
Fix the MD Incognito NTP background color regression (Closed)
|
Created:
3 years, 7 months ago by msramek Modified:
3 years, 7 months ago Reviewers:
Evan Stade CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, pedrosimonetti+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the MD Incognito NTP background color regression
The CL https://codereview.chromium.org/2891983003/ intended to fix the white
flash while loading Incognito NTP by moving the color definition from CSS
to ThemeProperties. Unfortunately, the approach had two problems:
1. The color in ThemeService can be overriden in the NTPResourceCache to
SkColor(0x32, 0x32, 0x32). However, the new MD Incognito NTP uses
SkColor(0x30, 0x30, 0x30) (which used to be defined in the CSS), which
creates a visible seam between the background and the content area.
2. The color definition in ThemeProperties is not always reached. ThemeService
can route the query to CustomThemeSupplier. For example, on Linux, the
request may be answered by SystemThemeX11, which has no concept of
Incognito, and thus always returns the default (white) background color.
This means that the white flash issue is not solved.
This CL only addresses the problem 1.
BUG=693525
Review-Url: https://codereview.chromium.org/2899053002
Cr-Commit-Position: refs/heads/master@{#474343}
Committed: https://chromium.googlesource.com/chromium/src/+/fe9fc5308c333d5dd084872675cff77ff87c894c
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments. #
Total comments: 2
Messages
Total messages: 18 (9 generated)
Description was changed from ========== The CL https://codereview.chromium.org/2891983003/ intended to fix the white flash while loading Incognito NTP by moving the color definition from CSS to ThemeProperties. Unfortunately, the approach had two problems: 1. The color in ThemeService can be overriden in the NTPResourceCache to SkColor(0x32, 0x32, 0x32). However, the new MD Incognito NTP uses SkColor(0x30, 0x30, 0x30) (which used to be defined in the CSS), which creates a visible seam between the background and the content area. 2. The color definition in ThemeProperties is not always reached. ThemeService can route the query to CustomThemeSupplier. For example, on Linux, the request may be answered by SystemThemeX11, which has no concept of Incognito, and thus always returns the default (white) background color. This means that the white flash issue is not solved. This CL only addresses the problem 1. BUG=693525 ========== to ========== Fix the MD Incognito NTP background color regression The CL https://codereview.chromium.org/2891983003/ intended to fix the white flash while loading Incognito NTP by moving the color definition from CSS to ThemeProperties. Unfortunately, the approach had two problems: 1. The color in ThemeService can be overriden in the NTPResourceCache to SkColor(0x32, 0x32, 0x32). However, the new MD Incognito NTP uses SkColor(0x30, 0x30, 0x30) (which used to be defined in the CSS), which creates a visible seam between the background and the content area. 2. The color definition in ThemeProperties is not always reached. ThemeService can route the query to CustomThemeSupplier. For example, on Linux, the request may be answered by SystemThemeX11, which has no concept of Incognito, and thus always returns the default (white) background color. This means that the white flash issue is not solved. This CL only addresses the problem 1. BUG=693525 ==========
msramek@chromium.org changed reviewers: + estade@chromium.org
Hi Evan, Please have a look! This is a followup to https://codereview.chromium.org/2891983003/ which we both reviewed recently. That CL created a regression in my Incognito NTP redesign, which I'm patching in this CL. I also further debugged the white flash problem, but that takes a larger CL to fix properly and I want to focus on the regression first. Thanks, Martin
https://codereview.chromium.org/2899053002/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (left): https://codereview.chromium.org/2899053002/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:502: : SkColorSetRGB(0x32, 0x32, 0x32); I'm confused. Can you just remove this ternary and always use GetThemeColor(tp, ThemeProperties::COLOR_NTP_BACKGROUND)?
https://codereview.chromium.org/2899053002/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (left): https://codereview.chromium.org/2899053002/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:502: : SkColorSetRGB(0x32, 0x32, 0x32); On 2017/05/24 00:23:43, Evan Stade wrote: > I'm confused. Can you just remove this ternary and always use GetThemeColor(tp, > ThemeProperties::COLOR_NTP_BACKGROUND)? Nope. If ThemeService routes that to ThemeProperties, which was patched in https://codereview.chromium.org/2891983003/ to return 0x30, it's fine. But if it routes to a CustomThemeSupplier subclass (e.g. when you select the GTK theme on Linux), it will incorrectly return *white*. That's because CustomThemeSupplier doesn't have a concept of incognito variants, and the non-incognito variant of COLOR_NTP_BACKGROUND is white. This seems to be the origin of the white flash bug (crbug.com/21798). I could move this override one level lower into ThemeService, but I don't think it's that much cleaner solution. The cleanest solution is probably to define a new constant COLOR_NTP_BACKGROUND_INCOGNITO and set it to 0x32 in all CustomThemeSupplier subclasses. I can do that if you think it makes sense, although I'm not familiar with the themes code. In this CL, I'm simply keeping the override of the background color which was always here, just change it to 0x30, since we have slightly changed the color for the |features::kMaterialDesignIncognitoNTP| redesign.
thanks for the explanation. https://codereview.chromium.org/2899053002/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (left): https://codereview.chromium.org/2899053002/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:502: : SkColorSetRGB(0x32, 0x32, 0x32); On 2017/05/24 01:05:21, msramek wrote: > On 2017/05/24 00:23:43, Evan Stade wrote: > > I'm confused. Can you just remove this ternary and always use > GetThemeColor(tp, > > ThemeProperties::COLOR_NTP_BACKGROUND)? > > Nope. > > If ThemeService routes that to ThemeProperties, which was patched in > https://codereview.chromium.org/2891983003/ to return 0x30, it's fine. > > But if it routes to a CustomThemeSupplier subclass (e.g. when you select the GTK > theme on Linux), it will incorrectly return *white*. That's because > CustomThemeSupplier doesn't have a concept of incognito variants, and the > non-incognito variant of COLOR_NTP_BACKGROUND is white. > > This seems to be the origin of the white flash bug (crbug.com/21798). > > I could move this override one level lower into ThemeService, but I don't think > it's that much cleaner solution. > > The cleanest solution is probably to define a new constant > COLOR_NTP_BACKGROUND_INCOGNITO and set it to 0x32 in all CustomThemeSupplier > subclasses. I can do that if you think it makes sense, although I'm not familiar > with the themes code. > > In this CL, I'm simply keeping the override of the background color which was > always here, just change it to 0x30, since we have slightly changed the color > for the |features::kMaterialDesignIncognitoNTP| redesign. it seems like the new ternary (the feature flag one) should go in theme_properties.cc. The last branch of this HasCustomImage ternary can change to ThemeProperties::GetDefaultColor(), I think.
The CQ bit was checked by msramek@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...
https://codereview.chromium.org/2899053002/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (left): https://codereview.chromium.org/2899053002/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:502: : SkColorSetRGB(0x32, 0x32, 0x32); On 2017/05/24 02:00:25, Evan Stade wrote: > On 2017/05/24 01:05:21, msramek wrote: > > On 2017/05/24 00:23:43, Evan Stade wrote: > > > I'm confused. Can you just remove this ternary and always use > > GetThemeColor(tp, > > > ThemeProperties::COLOR_NTP_BACKGROUND)? > > > > Nope. > > > > If ThemeService routes that to ThemeProperties, which was patched in > > https://codereview.chromium.org/2891983003/ to return 0x30, it's fine. > > > > But if it routes to a CustomThemeSupplier subclass (e.g. when you select the > GTK > > theme on Linux), it will incorrectly return *white*. That's because > > CustomThemeSupplier doesn't have a concept of incognito variants, and the > > non-incognito variant of COLOR_NTP_BACKGROUND is white. > > > > This seems to be the origin of the white flash bug (crbug.com/21798). > > > > I could move this override one level lower into ThemeService, but I don't > think > > it's that much cleaner solution. > > > > The cleanest solution is probably to define a new constant > > COLOR_NTP_BACKGROUND_INCOGNITO and set it to 0x32 in all CustomThemeSupplier > > subclasses. I can do that if you think it makes sense, although I'm not > familiar > > with the themes code. > > > > In this CL, I'm simply keeping the override of the background color which was > > always here, just change it to 0x30, since we have slightly changed the color > > for the |features::kMaterialDesignIncognitoNTP| redesign. > > it seems like the new ternary (the feature flag one) should go in > theme_properties.cc. > > The last branch of this HasCustomImage ternary can change to > ThemeProperties::GetDefaultColor(), I think. Done. Thanks for the suggestion, I agree that that's a bit cleaner.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks, lgtm https://codereview.chromium.org/2899053002/diff/20001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/2899053002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:73: // TODO(msramek): Remove the old entry when the new NTP fully launches. nit: I don't really mind if you leave this TODO here but it seems unnecessary given that when you remove the feature flag and are no longer using this constant the compiler will remind you to remove it.
Thanks! https://codereview.chromium.org/2899053002/diff/20001/chrome/browser/themes/t... File chrome/browser/themes/theme_properties.cc (right): https://codereview.chromium.org/2899053002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_properties.cc:73: // TODO(msramek): Remove the old entry when the new NTP fully launches. On 2017/05/24 17:30:26, Evan Stade wrote: > nit: I don't really mind if you leave this TODO here but it seems unnecessary > given that when you remove the feature flag and are no longer using this > constant the compiler will remind you to remove it. I just imagined that having two background colors for the same thing, especially with such a tiny difference, might be confusing to a random reader; and I wanted to emphasize that it's just temporary. I would then leave it here for that purpose.
The CQ bit was checked by msramek@chromium.org
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": 20001, "attempt_start_ts": 1495647597510230,
"parent_rev": "9de1000c0a7625e951a5a80303ab63d24b67542c", "commit_rev":
"fe9fc5308c333d5dd084872675cff77ff87c894c"}
Message was sent while issue was closed.
Description was changed from ========== Fix the MD Incognito NTP background color regression The CL https://codereview.chromium.org/2891983003/ intended to fix the white flash while loading Incognito NTP by moving the color definition from CSS to ThemeProperties. Unfortunately, the approach had two problems: 1. The color in ThemeService can be overriden in the NTPResourceCache to SkColor(0x32, 0x32, 0x32). However, the new MD Incognito NTP uses SkColor(0x30, 0x30, 0x30) (which used to be defined in the CSS), which creates a visible seam between the background and the content area. 2. The color definition in ThemeProperties is not always reached. ThemeService can route the query to CustomThemeSupplier. For example, on Linux, the request may be answered by SystemThemeX11, which has no concept of Incognito, and thus always returns the default (white) background color. This means that the white flash issue is not solved. This CL only addresses the problem 1. BUG=693525 ========== to ========== Fix the MD Incognito NTP background color regression The CL https://codereview.chromium.org/2891983003/ intended to fix the white flash while loading Incognito NTP by moving the color definition from CSS to ThemeProperties. Unfortunately, the approach had two problems: 1. The color in ThemeService can be overriden in the NTPResourceCache to SkColor(0x32, 0x32, 0x32). However, the new MD Incognito NTP uses SkColor(0x30, 0x30, 0x30) (which used to be defined in the CSS), which creates a visible seam between the background and the content area. 2. The color definition in ThemeProperties is not always reached. ThemeService can route the query to CustomThemeSupplier. For example, on Linux, the request may be answered by SystemThemeX11, which has no concept of Incognito, and thus always returns the default (white) background color. This means that the white flash issue is not solved. This CL only addresses the problem 1. BUG=693525 Review-Url: https://codereview.chromium.org/2899053002 Cr-Commit-Position: refs/heads/master@{#474343} Committed: https://chromium.googlesource.com/chromium/src/+/fe9fc5308c333d5dd084872675cf... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/fe9fc5308c333d5dd084872675cf... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
