Don't fall back to default incognito colors and tints when using a

Don't fall back to default incognito colors and tints when using a
custom theme.

BUG=596136
Committed: https://crrev.com/66736a7734ef10200f9a653502464260877fd397
Cr-Commit-Position: refs/heads/master@{#383895}

[Evan Stade, 2016-03-18 20:48:03]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org

[Evan Stade, 2016-03-18 20:48:08]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-18 20:48:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813393002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813393002/1

[Peter Kasting, 2016-03-18 20:53:50]

https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:499: return
ThemeProperties::GetDefaultColor(id, incognito && !theme_supplier_);
Hmm.  Couldn't this also break some themes, if we have color IDs that differ
between normal and incognito pre-MD?  In such cases, the old behavior would have
been that we fall back to a default incognito color for these IDs, but now we're
falling back to a non-incognito color.

[Evan Stade, 2016-03-18 21:15:21]

https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:499: return
ThemeProperties::GetDefaultColor(id, incognito && !theme_supplier_);
On 2016/03/18 20:53:50, Peter Kasting wrote:
> Hmm.  Couldn't this also break some themes, if we have color IDs that differ
> between normal and incognito pre-MD?  In such cases, the old behavior would
have
> been that we fall back to a default incognito color for these IDs, but now
we're
> falling back to a non-incognito color.

Perhaps, but I checked about 8 themes and the only breakages I saw were due to
bad theme values (e.g. they made the toolbar color dark but didn't change the
button tint). DEFAULT_TINT_BUTTONS in pre-MD is the same for incognito and not
incognito, and this is probably true of a lot of colors.

Do you think it would be safer/preferable to make this

bool use_incognito = incognito && (!theme_supplier_ || !IsModeMaterial());

[Peter Kasting, 2016-03-18 21:22:53]

https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:499: return
ThemeProperties::GetDefaultColor(id, incognito && !theme_supplier_);
On 2016/03/18 21:15:21, Evan Stade wrote:
> On 2016/03/18 20:53:50, Peter Kasting wrote:
> > Hmm.  Couldn't this also break some themes, if we have color IDs that differ
> > between normal and incognito pre-MD?  In such cases, the old behavior would
> have
> > been that we fall back to a default incognito color for these IDs, but now
> we're
> > falling back to a non-incognito color.
> 
> Perhaps, but I checked about 8 themes and the only breakages I saw were due to
> bad theme values (e.g. they made the toolbar color dark but didn't change the
> button tint). DEFAULT_TINT_BUTTONS in pre-MD is the same for incognito and not
> incognito, and this is probably true of a lot of colors.
> 
> Do you think it would be safer/preferable to make this
> 
> bool use_incognito = incognito && (!theme_supplier_ || !IsModeMaterial());

No; instead I think we should only disable |use_incognito| for cases where we've
added an incognito-specific default that didn't use to have an
incognito-specific default.  That will make us behave exactly the way we used
to.  Themes might rely on the other behaviors, and restricting to MD would just
mean the themes would only break in MD, rahter than preventing them from
breaking at all.

[Evan Stade, 2016-03-18 21:29:52]

On 2016/03/18 21:22:53, Peter Kasting wrote:
>
https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
> File chrome/browser/themes/theme_service.cc (right):
> 
>
https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
> chrome/browser/themes/theme_service.cc:499: return
> ThemeProperties::GetDefaultColor(id, incognito && !theme_supplier_);
> On 2016/03/18 21:15:21, Evan Stade wrote:
> > On 2016/03/18 20:53:50, Peter Kasting wrote:
> > > Hmm.  Couldn't this also break some themes, if we have color IDs that
differ
> > > between normal and incognito pre-MD?  In such cases, the old behavior
would
> > have
> > > been that we fall back to a default incognito color for these IDs, but now
> > we're
> > > falling back to a non-incognito color.
> > 
> > Perhaps, but I checked about 8 themes and the only breakages I saw were due
to
> > bad theme values (e.g. they made the toolbar color dark but didn't change
the
> > button tint). DEFAULT_TINT_BUTTONS in pre-MD is the same for incognito and
not
> > incognito, and this is probably true of a lot of colors.
> > 
> > Do you think it would be safer/preferable to make this
> > 
> > bool use_incognito = incognito && (!theme_supplier_ || !IsModeMaterial());
> 
> No; instead I think we should only disable |use_incognito| for cases where
we've
> added an incognito-specific default that didn't use to have an
> incognito-specific default.  That will make us behave exactly the way we used
> to.  Themes might rely on the other behaviors, and restricting to MD would
just
> mean the themes would only break in MD, rahter than preventing them from
> breaking at all.

But the defaults (incognito and non) are already changing in MD, so themes that
fall back to defaults will be changing and could break (for some definition of
break).

[Peter Kasting, 2016-03-18 21:47:53]

On 2016/03/18 21:29:52, Evan Stade wrote:
> On 2016/03/18 21:22:53, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
> > File chrome/browser/themes/theme_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
> > chrome/browser/themes/theme_service.cc:499: return
> > ThemeProperties::GetDefaultColor(id, incognito && !theme_supplier_);
> > On 2016/03/18 21:15:21, Evan Stade wrote:
> > > On 2016/03/18 20:53:50, Peter Kasting wrote:
> > > > Hmm.  Couldn't this also break some themes, if we have color IDs that
> differ
> > > > between normal and incognito pre-MD?  In such cases, the old behavior
> would
> > > have
> > > > been that we fall back to a default incognito color for these IDs, but
now
> > > we're
> > > > falling back to a non-incognito color.
> > > 
> > > Perhaps, but I checked about 8 themes and the only breakages I saw were
due
> to
> > > bad theme values (e.g. they made the toolbar color dark but didn't change
> the
> > > button tint). DEFAULT_TINT_BUTTONS in pre-MD is the same for incognito and
> not
> > > incognito, and this is probably true of a lot of colors.
> > > 
> > > Do you think it would be safer/preferable to make this
> > > 
> > > bool use_incognito = incognito && (!theme_supplier_ || !IsModeMaterial());
> > 
> > No; instead I think we should only disable |use_incognito| for cases where
> we've
> > added an incognito-specific default that didn't use to have an
> > incognito-specific default.  That will make us behave exactly the way we
used
> > to.  Themes might rely on the other behaviors, and restricting to MD would
> just
> > mean the themes would only break in MD, rahter than preventing them from
> > breaking at all.
> 
> But the defaults (incognito and non) are already changing in MD, so themes
that
> fall back to defaults will be changing and could break (for some definition of
> break).

If that's the case then something is broken no matter what we do.

Let me sketch out the different cases here:

(a) Colors where a theme implicitly uses the default non-incognito color
(b) Colors where a theme implicitly uses the default incognito color
(c) Colors which used to be (a) but are now (b) because we added an
incognito-specific color

Both cases (a) and (b) are things we may break anyway simply by changing what
our default colors are.  There's really no great solution for themes here;
they'll have to be updated or else look broken.

Case (c) has the additional problem that we've added a new incognito-specific
color, so even if the theme wouldn't have been broken had it stayed in (a), it
now is broken.  (My understanding is that the example on the linked bug is in
this bucket.)

Basically, your proposal is to force (b) and (c) to be treated as (a), either
all the time or in MD.  My proposal is to leave (b) alone and treat (c) as (a),
all the time.

Of these choices, I think the latter is safer, especially if the incognito color
scheme is very significantly different than the normal color scheme.  When
that's the case, forcing (b) to be treated like (a) seems like it will greatly
increase the risk of problems.  If any colors in (b) are not overridable by
themes, this may also cripple theme authors' abilities to match the incognito
theme with what parts of the custom theme they control.

This isn't a slam dunk; I can think of a number of counterarguments.

[commit-bot: I haz the power, 2016-03-18 22:14:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-18 22:14:24]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-03-29 22:38:43]

On 2016/03/18 21:47:53, Peter Kasting wrote:
> On 2016/03/18 21:29:52, Evan Stade wrote:
> > On 2016/03/18 21:22:53, Peter Kasting wrote:
> > >
> >
>
https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
> > > File chrome/browser/themes/theme_service.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1813393002/diff/1/chrome/browser/themes/theme...
> > > chrome/browser/themes/theme_service.cc:499: return
> > > ThemeProperties::GetDefaultColor(id, incognito && !theme_supplier_);
> > > On 2016/03/18 21:15:21, Evan Stade wrote:
> > > > On 2016/03/18 20:53:50, Peter Kasting wrote:
> > > > > Hmm.  Couldn't this also break some themes, if we have color IDs that
> > differ
> > > > > between normal and incognito pre-MD?  In such cases, the old behavior
> > would
> > > > have
> > > > > been that we fall back to a default incognito color for these IDs, but
> now
> > > > we're
> > > > > falling back to a non-incognito color.
> > > > 
> > > > Perhaps, but I checked about 8 themes and the only breakages I saw were
> due
> > to
> > > > bad theme values (e.g. they made the toolbar color dark but didn't
change
> > the
> > > > button tint). DEFAULT_TINT_BUTTONS in pre-MD is the same for incognito
and
> > not
> > > > incognito, and this is probably true of a lot of colors.
> > > > 
> > > > Do you think it would be safer/preferable to make this
> > > > 
> > > > bool use_incognito = incognito && (!theme_supplier_ ||
!IsModeMaterial());
> > > 
> > > No; instead I think we should only disable |use_incognito| for cases where
> > we've
> > > added an incognito-specific default that didn't use to have an
> > > incognito-specific default.  That will make us behave exactly the way we
> used
> > > to.  Themes might rely on the other behaviors, and restricting to MD would
> > just
> > > mean the themes would only break in MD, rahter than preventing them from
> > > breaking at all.
> > 
> > But the defaults (incognito and non) are already changing in MD, so themes
> that
> > fall back to defaults will be changing and could break (for some definition
of
> > break).
> 
> If that's the case then something is broken no matter what we do.
> 
> Let me sketch out the different cases here:
> 
> (a) Colors where a theme implicitly uses the default non-incognito color
> (b) Colors where a theme implicitly uses the default incognito color
> (c) Colors which used to be (a) but are now (b) because we added an
> incognito-specific color
> 
> Both cases (a) and (b) are things we may break anyway simply by changing what
> our default colors are.  There's really no great solution for themes here;
> they'll have to be updated or else look broken.
> 
> Case (c) has the additional problem that we've added a new incognito-specific
> color, so even if the theme wouldn't have been broken had it stayed in (a), it
> now is broken.  (My understanding is that the example on the linked bug is in
> this bucket.)
> 
> Basically, your proposal is to force (b) and (c) to be treated as (a), either
> all the time or in MD.  My proposal is to leave (b) alone and treat (c) as
(a),
> all the time.
> 
> Of these choices, I think the latter is safer, especially if the incognito
color
> scheme is very significantly different than the normal color scheme.  When
> that's the case, forcing (b) to be treated like (a) seems like it will greatly
> increase the risk of problems.  If any colors in (b) are not overridable by
> themes, this may also cripple theme authors' abilities to match the incognito
> theme with what parts of the custom theme they control.
> 
> This isn't a slam dunk; I can think of a number of counterarguments.

Sorry it took me a while to get back to this. I don't think (b) really existed
before MD because only two colors had incognito variants:
COLOR_FRAME_INCOGNITO_INACTIVE and TINT_FRAME_INCOGNITO_INACTIVE. These are
overrideable by themes, but the default values are now changing to something
dramatically different, so I don't think it's a good idea to keep falling back
to the incognito variants. Better to fall back to the only slightly different
non-incognito version.

As far as other new colors go, i.e. case (c), the incognito colors are also
dramatically different from the normal colors. We expect custom-themed incognito
mode to be a lot closer to custom-themed normal mode than classic-themed
incognito mode, hence (c) should fall back to (a) (I think you already agree
with this latter point).

[Peter Kasting, 2016-03-29 23:31:33]

On 2016/03/29 22:38:43, Evan Stade wrote:
> As far as other new colors go, i.e. case (c), the incognito colors are also
> dramatically different from the normal colors. We expect custom-themed
incognito
> mode to be a lot closer to custom-themed normal mode than classic-themed
> incognito mode, hence (c) should fall back to (a) (I think you already agree
> with this latter point).

It would be good to capture some of this "why" in the comments being added.

LGTM

[Evan Stade, 2016-03-29 23:44:39]

On 2016/03/29 23:31:33, Peter Kasting wrote:
> It would be good to capture some of this "why" in the comments being added.

thanks, done

[Evan Stade, 2016-03-29 23:44:55]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-03-29 23:44:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1813393002/#ps20001
(title: "add comment")

[commit-bot: I haz the power, 2016-03-29 23:45:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813393002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813393002/20001

[commit-bot: I haz the power, 2016-03-30 01:26:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-30 01:28:33]

Description was changed from

==========
Don't fall back to default incognito colors and tints when using a
custom theme.

BUG=596136
==========

to

==========
Don't fall back to default incognito colors and tints when using a
custom theme.

BUG=596136

Committed: https://crrev.com/66736a7734ef10200f9a653502464260877fd397
Cr-Commit-Position: refs/heads/master@{#383895}
==========

[commit-bot: I haz the power, 2016-03-30 01:28:34]

Patchset 2 (id:??) landed as
https://crrev.com/66736a7734ef10200f9a653502464260877fd397
Cr-Commit-Position: refs/heads/master@{#383895}