I think that default_favicon.icon gets processed to become accessible as kDefaultFaviconIcon and code-search shows its one use here:

https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm?rcl=2b73a386b986b099182e8cc28d59f479c1af7e6a&l=53

Are you testing on a Mac?  At least on my dev Linux machine, I am seeing the default favicon change from black to white when I switch from classic to a dark theme: it matches the text.  So...how to repro?  Maybe it's theme-specific?

(1) Nope, Windows 10.
(2) Set your system to color titlebars with some dark color and look at the color of the default favicon in background tabs.

Screenshot attached.

Deleted: favicon.png 2.1 KB

	favicon.png 2.1 KB View Download

Oh, I see -- changing the Windows 10 title bar accent color causes tab text color to adapt but not the icon.  I was just changing the chrome theme from dark to light - and in that case, the text/icon colors did change together.  This distinction may be a hint of what to look for in the logic, I will check it out after my current Pri-2 issues.

Digging into the code, I see why this is happening.  Default favicons will render a cached "themed" version of the icon produced by TabIcon::ThemeImage: 
gfx::ImageSkiaOperations::CreateHSLShiftedImage(source, GetThemeProvider()->GetTint(ThemeProperties::TINT_BUTTONS));

Even the vector icon renders to raster image, as that's the favicon representation.  This image generation/coloring happens in TabIcon::SetIcon once and then no update is applied when text color changes in Tab::OnButtonColorMaybeChanged.  Moving toward a fix, the color can be updated after title_->SetEnabledColor using this newly defined method:

void TabIcon::ColorThemedFavicon(SkColor color) {
  color_utils::HSL hsl;
  color_utils::SkColorToHSL(color, &hsl);
  themed_favicon_ = gfx::ImageSkiaOperations::CreateHSLShiftedImage(favicon_, hsl);
}

Some checks may be necessary, but I have this sketch driving themed favicon color when switching tabs.  I don't want to upload a CL until I can test this on Windows, though.  I'll see about setting that up tomorrow - if rewriting the image for text color changes is what we want to do.  Is the color change worth the extra image processing?

We should be able to get away with just creating two icons instead of one (for background tabs and foreground tabs) at the time the theme changes, and then drawing the appropriate one for each tab.  We could optimize by using just one icon in cases where the two would have the same color.  Either way, this lets us keep doing the processing only when the theme changes.

Also beware of bug 854738, since if we switched the tab title text color in that case we'd also want to switch which icon we drew.

(Although we'd also want to recompute if the frame color changes, which can happen without a theme change... we might need to compute foreground/background x active/inactive windows.)

Yeah, I can see the potential for various influences on color, and since the OS may change title bar accent at any time, adapting flexibly is ideal.  Caching more copies of the icon is an option but if we do that, I think it would be better to use a central store for the variants that all tabs can share instead of tab-owned clones - especially since TabIcon is written with built-in duplication already.  I know memory conservation is not one of the 4 S's of Chrome, but still, I can see this bulking up without offering a whole lot of value in return, so I'm hesitant about this approach.

We'd want to cache the icons either in some sort of profile-keyed static store on Tab, or maybe just in the TabStrip.  There's already some machinery for caching the appearance of tabs themselves -- look where that is and what sort of keys control it.

It's unclear whether the cost (via either image processing or caching) is worth the benefit here, and I have some higher priority tasks right now so moving myself to CC.  Happy to help as I can if anyone picks this up.

OK.  Bumping this to P2, it's one of the more obvious visual polish problems GM2 has right now, so we'll need someone to take this.  Maybe I can get to it.

I took a look at this and had some random thoughts (outside the concern of caching).

First, it appears that icons are a slightly different color than the text, in some themes (perhaps all, but it's hard to see). (I'm comparing Ocean Pacific to Classic.) I'm wondering why we don't just peg them together. When I simply pass 'title_color' to the icon, it looks really nice, and coding is trivial. When they're different, it looks odd, even buggy.

Additionally, there are actually 4 states here, not just 2: There's whether a tab is active or not, as well as whether the browser is focused or not. The approach below only handles activity. Pegging the colors would handle all states.

If we're only concerned with handling tab activity, I have this approach of shifting the color identically to the title:

     title_->SetEnabledColor(title_color);
+    if (IsActive())
+      icon_->OnThemeChanged();
+    else
+      icon_->MakeContrastingColor();

The latter would just do the 'BlendToOppositeLuma' thing. I don't see a huge difference. 0x2f just isn't that much. Let me know if this sounds worthy of review.

btw, I noticed that when we look up an HSL value, as opposed to a SkColor, and it fails, we get back a triple of -1, not red. This appears to render as black. I wonder how often this is happening and we're not noticing.

Pegging the icon color to the text color SGTM.

If 'pegging' means keeping the two colors synchronized, I agree that looks good and handles all the cases (provided text color is right in all cases).  I didn't proceed with such a change, though, because window and tab focus changes happen often on clicks, and processing icon images every time seemed like a nontrivial performance penalty incurred for the sake of edge cases.  It's not noticeable by itself, but as a game developer I always ask "is it worth it?" when processing weight is added.  Same goes for cached copies and memory footprint.

Asking "is it worth it" is good, but better-functioning UI is pretty much always worth it.  CPU is more important than memory, so caching these images is likely better than recreating them unless recreating is trivially cheap.  Caching is likely on the order of a few kilobytes, which isn't worth worrying about.

I'm leaning towards not caching here. We cache favicons because replacing requires a download, done during keystrokes. I'm skeptical that redrawing a 256 pixel 2 color icon while redrawing an entire page of content is noticeable.

Computing a vector icon requires creating vector paths, then rasterizing them.  Depending on how this is implemented, the worst case would be doing that for every one of several hundred tabs on every mouse move.  That wouldn't be acceptable, so I'd like to have a better understanding of when we'd be re-drawing these icons first.

By contrast just having a cache of up to four icons (for the four states you identified), with potentially fewer if we see that some colors are the same, sounds to me like it's pretty low-cost.

I don't think the color will change on mouse moves, just clicks that change window or tab focus, thereby changing text color.  And either approach, caching or re-processing with new color, will have very minor impact, unnoticeable by itself.  What I'm considering here is: do we degrade the snappy performance for a billion users, even by half a millisecond or a few kilobytes -- for the sake of a rare edge case with only aesthetic consequences?  Nearly all favicons remain colored as they are regardless of text changes, only the default vector icons are we trying to have match the text.  To me it just doesn't seem like a big deal, but if it's affecting usability or something, I'd say go ahead and take either approach.

Every mouse move? I'm only seeing this code called on a click or ^page (or alt-tab).

The most likely issue with moves is if we change the tab text color on hover, as proposed elsewhere.  That would get us to the point of redrawing this on hover changes.

The less likely issue with moves is if we end up naively re-drawing on all tab repaints; those in turn can get triggered pretty aggressively.

Regarding the tradeoff in comment 19: This is not rare (affects theme users + users with colored titlebars, which in total is probably around 100M people); the cost of a couple of kilobytes of memory is basically zero, and it's certainly worth paying that cost everywhere to make this not look glitchy.

Okay, I thought it was rare as I could only get it to happen on Windows when I changed system title-bar accent colors, but yeah, common enough has me convinced. :)  I think krb@ had a plan to knock this out pretty easily.

Don't stop asking the questions you're asking, though.  They're the right questions, and we should have answers to them when we make changes like this.

Disagree with M-X