Change chrome:// favicons in tabstrip based on theming.

Change chrome:// favicons in tabstrip based on theming.

This hack is cool because it works for incognito and themes, but it's not
cool because it will have to be copied over for bookmark buttons and normal
websites that have the same issue with their icons can't address it in the same
way.

BUG=526663
TBR=groby@chromium.org

Committed: https://crrev.com/80c47667a3be9304f80c055bd5611527e67fcbe5
Cr-Commit-Position: refs/heads/master@{#403581}

[Evan Stade, 2016-06-23 19:07:04]

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

[Evan Stade, 2016-06-23 19:07:05]

Scott and Peter, wdyt of this general approach?

[sky, 2016-06-23 20:53:54]

What do other browsers do in similar scenarios? Can we instead design our icons
to work better with dark schemes?

[Evan Stade, 2016-06-23 21:35:27]

On 2016/06/23 20:53:54, sky wrote:
> What do other browsers do in similar scenarios? Can we instead design our
icons
> to work better with dark schemes?

see the bug for screenshots --- Sebastien tried that but the outcome was
suboptimal imo.

[sky, 2016-06-23 22:51:02]

You should attach screenshots of what your change looks like to the
bug for comparison and ping Sebastien. IMO web site favicons aren't
going to change with different themes, so it seems to me we shouldn't
have to change ours either. That said, we should definitely make sure
our icons look good with incognito.

  -Scott

On Thu, Jun 23, 2016 at 2:35 PM,  <estade@chromium.org> wrote:
> On 2016/06/23 20:53:54, sky wrote:
>> What do other browsers do in similar scenarios? Can we instead design our
> icons
>> to work better with dark schemes?
>
> see the bug for screenshots --- Sebastien tried that but the outcome was
> suboptimal imo.
>
> https://codereview.chromium.org/2091053002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-06-24 15:55:00]

On 2016/06/23 22:51:02, sky wrote:
> You should attach screenshots of what your change looks like to the
> bug for comparison and ping Sebastien. IMO web site favicons aren't
> going to change with different themes, so it seems to me we shouldn't
> have to change ours either. That said, we should definitely make sure
> our icons look good with incognito.
> 
>   -Scott

right. I guess the feedback I'm looking for at this point is whether this is too
hacky to even consider. I'd have to work on getting the right HSL but I don't
want to spend that time if others are staunchly opposed to this approach.

The color should match the other icons in the UI (for the default theme that's
#5a5a5a) and I am pretty confident Sebastien would be happy with the screenshots
if I were able to achieve that.

> 
> On Thu, Jun 23, 2016 at 2:35 PM,  <mailto:estade@chromium.org> wrote:
> > On 2016/06/23 20:53:54, sky wrote:
> >> What do other browsers do in similar scenarios? Can we instead design our
> > icons
> >> to work better with dark schemes?
> >
> > see the bug for screenshots --- Sebastien tried that but the outcome was
> > suboptimal imo.
> >
> > https://codereview.chromium.org/2091053002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-06-24 16:05:18]

This is for our own icons, so I can't see why it would necessarily be
bad. It may not look the greatest, but we're doing it because
currently it doesn't look that good. So, I'm not staunchly opposed to
it. If UX is happy, fine by me. Also, I would think you only want to
do this under some conditions. For example, we shouldn't change the
colors with the default theme at all, right?

  -Scott

On Fri, Jun 24, 2016 at 8:55 AM,  <estade@chromium.org> wrote:
> On 2016/06/23 22:51:02, sky wrote:
>> You should attach screenshots of what your change looks like to the
>> bug for comparison and ping Sebastien. IMO web site favicons aren't
>> going to change with different themes, so it seems to me we shouldn't
>> have to change ours either. That said, we should definitely make sure
>> our icons look good with incognito.
>>
>> -Scott
>
> right. I guess the feedback I'm looking for at this point is whether this is
> too
> hacky to even consider. I'd have to work on getting the right HSL but I
> don't
> want to spend that time if others are staunchly opposed to this approach.
>
> The color should match the other icons in the UI (for the default theme
> that's
> #5a5a5a) and I am pretty confident Sebastien would be happy with the
> screenshots
> if I were able to achieve that.
>
>>
>> On Thu, Jun 23, 2016 at 2:35 PM, <mailto:estade@chromium.org> wrote:
>> > On 2016/06/23 20:53:54, sky wrote:
>> >> What do other browsers do in similar scenarios? Can we instead design
>> >> our
>> > icons
>> >> to work better with dark schemes?
>> >
>> > see the bug for screenshots --- Sebastien tried that but the outcome was
>> > suboptimal imo.
>> >
>> > https://codereview.chromium.org/2091053002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> https://codereview.chromium.org/2091053002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-06-24 16:09:48]

On 2016/06/24 16:05:18, sky wrote:
> do this under some conditions. For example, we shouldn't change the
> colors with the default theme at all, right?

We wouldn't *need* to but ideally the code would handle that just fine (i.e.
essentially no-op) which would be nice because then we wouldn't need to special
case anything and any future changes to icon coloration would be picked up
automatically.

[Evan Stade, 2016-06-24 20:14:36]

attached screenshot of latest code to bug.

Updating the bookmarks bar in a similar way would be a good follow up.

[sky, 2016-06-24 20:21:48]

What does Sebastien think?

On Fri, Jun 24, 2016 at 1:14 PM,  <estade@chromium.org> wrote:
> attached screenshot of latest code to bug.
>
> Updating the bookmarks bar in a similar way would be a good follow up.
>
> https://codereview.chromium.org/2091053002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-06-24 20:29:12]

> What does Sebastien think?

"This is awesome :) thank you!"

[Evan Stade, 2016-06-24 20:29:38]

Description was changed from

==========
Change chrome:// favicons in tabstrip based on theming.

This is a WIP, a proof-of-concept hack - the exact color it outputs is not
correct for normal mode and seems slightly too faint in themed or incognito
mode.

This hack is cool because it works for incognito and themes, but it's not
cool because it will have to be copied over for bookmark buttons and normal
websites that have the same issue with their icons can't address it in the same
way.

BUG=526663
==========

to

==========
Change chrome:// favicons in tabstrip based on theming.

This hack is cool because it works for incognito and themes, but it's not
cool because it will have to be copied over for bookmark buttons and normal
websites that have the same issue with their icons can't address it in the same
way.

BUG=526663
==========

[Evan Stade, 2016-06-24 21:26:00]

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

[commit-bot: I haz the power, 2016-06-24 21:26:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 21:29:08]

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

[commit-bot: I haz the power, 2016-06-24 21:29:09]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[sky, 2016-06-24 22:24:16]

You'll also need to merge with the tab change you reviewed earlier.

https://codereview.chromium.org/2091053002/diff/40001/ui/base/theme_provider.h
File ui/base/theme_provider.h (right):

https://codereview.chromium.org/2091053002/diff/40001/ui/base/theme_provider....
ui/base/theme_provider.h:62: virtual color_utils::HSL GetTint(int id) const = 0;
This name is pretty generic. Should it be more specific,
GetTintForInternalIcons?

[Evan Stade, 2016-06-27 14:56:59]

merged and added the change to the bookmark bar as well as the default favicon.

https://codereview.chromium.org/2091053002/diff/40001/ui/base/theme_provider.h
File ui/base/theme_provider.h (right):

https://codereview.chromium.org/2091053002/diff/40001/ui/base/theme_provider....
ui/base/theme_provider.h:62: virtual color_utils::HSL GetTint(int id) const = 0;
On 2016/06/24 22:24:16, sky wrote:
> This name is pretty generic. Should it be more specific,
> GetTintForInternalIcons?

GetTintForInternalIcons only makes sense when the ThemeProvider is ThemeService
and the id is TINT_BUTTONS. Other tints like TINT_FRAME aren't used for icons.

[Evan Stade, 2016-06-27 14:57:05]

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

[commit-bot: I haz the power, 2016-06-27 14:57:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 15:23:56]

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

[commit-bot: I haz the power, 2016-06-27 15:23:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2016-06-27 16:09:58]

Ok, LGTM

[Evan Stade, 2016-06-27 16:36:05]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-06-27 16:36:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 17:04:39]

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

[commit-bot: I haz the power, 2016-06-27 17:04:40]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Evan Stade, 2016-06-28 00:09:40]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-06-28 00:09:41]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2091053002/#ps80001
(title: "fix mac")

[commit-bot: I haz the power, 2016-06-28 00:10:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 00:26:00]

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

[commit-bot: I haz the power, 2016-06-28 00:26:01]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Kasting, 2016-06-29 07:31:55]

Please give me time to look at this before you land it

[Evan Stade, 2016-06-29 20:47:43]

On 2016/06/29 07:31:55, Peter Kasting (OOO) wrote:
> Please give me time to look at this before you land it

I'd like to land it before the branch point. Do you think you'll have time to
review it before then (with enough time left over for me to make any necessary
adjustments?)

[Peter Kasting, 2016-06-30 00:07:04]

LGTM

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1789: // Themify
chrome:// favicons and the default one.
Nit: Might want to add "This ensures they're visible over the tab background" or
some other motivator.

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1168: favicon_ = gfx::ImageSkia();
Do we have to clear on every change?  Are there changes that wouldn't change the
favicon, which we can avoid clearing for?

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1485: // Themify the icon if it's a
chrome:// page or if it's the sadtab favicon.
Should this share code with the bookmark bar?  Should the two note the existence
of the other?

[Evan Stade, 2016-06-30 15:39:45]

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1789: // Themify
chrome:// favicons and the default one.
On 2016/06/30 00:07:04, Peter Kasting (slow) wrote:
> Nit: Might want to add "This ensures they're visible over the tab background"
or
> some other motivator.

Done.

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1168: favicon_ = gfx::ImageSkia();
On 2016/06/30 00:07:04, Peter Kasting (slow) wrote:
> Do we have to clear on every change?  Are there changes that wouldn't change
the
> favicon, which we can avoid clearing for?

I think the answer is yes, but I'm not sure if it's easy to know with certainty
whether we can avoid clearing the cache. In the normal case (non-chrome://
pages) clearing the cache is harmless as there's no computation done during
paint.

In any case, I've changed this check such that it should catch some more cases
where we don't need to reset the cache, which also has the nice side effect of
eliminating this reference to about:crash.

https://codereview.chromium.org/2091053002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1485: // Themify the icon if it's a
chrome:// page or if it's the sadtab favicon.
On 2016/06/30 00:07:04, Peter Kasting (slow) wrote:
> Should this share code with the bookmark bar?  Should the two note the
existence
> of the other?

I didn't share code because I didn't see a clear and useful way to do so. The
only shared bits are url.SchemeIs(content::kChromeUIScheme) and the
CreateHSLShiftedImage invocation.

I can add cross-references in comments.

https://codereview.chromium.org/2091053002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091053002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:147: url.host() !=
chrome::kChromeUIUberHost;
note this slight inconvenience --- two chrome:// pages use the product logo icon
and shouldn't be colorized.

[Evan Stade, 2016-06-30 15:39:51]

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

[commit-bot: I haz the power, 2016-06-30 15:40:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-06-30 15:40:45]

https://codereview.chromium.org/2091053002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2091053002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:147: url.host() !=
chrome::kChromeUIUberHost;
On 2016/06/30 15:39:45, Evan Stade wrote:
> note this slight inconvenience --- two chrome:// pages use the product logo
icon
> and shouldn't be colorized.

Also, you may wonder why this isn't necessary for the bookmarks bar ---
crbug.com/624821 explains it.

[commit-bot: I haz the power, 2016-06-30 16:01:59]

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

[commit-bot: I haz the power, 2016-06-30 16:02:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2016-06-30 21:04:30]

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

[commit-bot: I haz the power, 2016-06-30 21:05:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 21:31:10]

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

[commit-bot: I haz the power, 2016-06-30 21:31:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2016-07-01 14:18:17]

Description was changed from

==========
Change chrome:// favicons in tabstrip based on theming.

This hack is cool because it works for incognito and themes, but it's not
cool because it will have to be copied over for bookmark buttons and normal
websites that have the same issue with their icons can't address it in the same
way.

BUG=526663
==========

to

==========
Change chrome:// favicons in tabstrip based on theming.

This hack is cool because it works for incognito and themes, but it's not
cool because it will have to be copied over for bookmark buttons and normal
websites that have the same issue with their icons can't address it in the same
way.

BUG=526663
TBR=groby@chromium.org
==========

[Evan Stade, 2016-07-01 14:18:17]

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

[Evan Stade, 2016-07-01 14:18:36]

TBR groby for (mechanical) changes to mac

[Evan Stade, 2016-07-01 14:18:47]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-07-01 14:18:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2091053002/#ps160001
(title: "mac includes")

[commit-bot: I haz the power, 2016-07-01 14:18:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 15:33:35]

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

[commit-bot: I haz the power, 2016-07-01 15:33:37]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-07-01 16:38:46]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-07-01 16:38:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 18:02:05]

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

[commit-bot: I haz the power, 2016-07-01 18:02:06]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[groby-ooo-7-16, 2016-07-01 19:36:37]

LGTM post hoc

[Evan Stade, 2016-07-01 19:49:02]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-07-01 19:49:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 21:28:21]

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

[commit-bot: I haz the power, 2016-07-01 21:28:23]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-07-01 23:42:25]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-07-01 23:42:26]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org,
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2091053002/#ps180001
(title: "turns out that's a real test failure on gtk (add TODO)")

[commit-bot: I haz the power, 2016-07-01 23:42:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-02 00:42:29]

Description was changed from

==========
Change chrome:// favicons in tabstrip based on theming.

This hack is cool because it works for incognito and themes, but it's not
cool because it will have to be copied over for bookmark buttons and normal
websites that have the same issue with their icons can't address it in the same
way.

BUG=526663
TBR=groby@chromium.org
==========

to

==========
Change chrome:// favicons in tabstrip based on theming.

This hack is cool because it works for incognito and themes, but it's not
cool because it will have to be copied over for bookmark buttons and normal
websites that have the same issue with their icons can't address it in the same
way.

BUG=526663
TBR=groby@chromium.org
==========

[commit-bot: I haz the power, 2016-07-02 00:42:32]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-02 00:42:40]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-02 00:45:47]

Description was changed from

==========
Change chrome:// favicons in tabstrip based on theming.

This hack is cool because it works for incognito and themes, but it's not
cool because it will have to be copied over for bookmark buttons and normal
websites that have the same issue with their icons can't address it in the same
way.

BUG=526663
TBR=groby@chromium.org
==========

to

==========
Change chrome:// favicons in tabstrip based on theming.

This hack is cool because it works for incognito and themes, but it's not
cool because it will have to be copied over for bookmark buttons and normal
websites that have the same issue with their icons can't address it in the same
way.

BUG=526663
TBR=groby@chromium.org

Committed: https://crrev.com/80c47667a3be9304f80c055bd5611527e67fcbe5
Cr-Commit-Position: refs/heads/master@{#403581}
==========

[commit-bot: I haz the power, 2016-07-02 00:45:48]

Patchset 10 (id:??) landed as
https://crrev.com/80c47667a3be9304f80c055bd5611527e67fcbe5
Cr-Commit-Position: refs/heads/master@{#403581}