Retry r88137:

Retry r88137:

Add code to calculate the dominant color for a favicon.

Currently we calculate the dominant/representative color only for those favicons we need it for (i.e. the ones shown on the NTP). We don't do any caching either in memory or in the favicon db but that can be tacked on later if deemed suitable.

Code in color_analysis.* authored by dtrainor

BUG=none
TEST=trybots

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88600

[Evan Stade, 2011-06-09 20:27:43]

csilv is out, arv is out, stuartmorgan is out, so review lands (arbitrarily) on
Tony. The key to this change (as compared to the last version of this CL) is the
added value passed in most_visited_handler.cc. I had to fix the case where the
favicon is chrome://theme/IDR_FOO (i.e. for the stock most visited pages),
otherwise we'd try to look up chrome://theme/IDR_FOO in the favicon database
(clearly it's not there). Compared to hooking up the code necessary to calculate
the dominant favicon color for these resources, hard-coding the dominant color
here is (1) easier to program (2) faster to execute (3) better looking, since we
can tweak it.

[tony, 2011-06-09 20:34:38]

LGTM

[tony, 2011-06-09 20:37:23]

http://codereview.chromium.org/7031078/diff/2001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

http://codereview.chromium.org/7031078/diff/2001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:292:
page_value->SetString("faviconDominantColor", "rgb(0, 147, 60)");
Nit: Might be worth commenting what these colors are.  Or you could do something
like const char* kGreen = "rgb(...)";

[Evan Stade, 2011-06-09 20:51:06]

http://codereview.chromium.org/7031078/diff/2001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

http://codereview.chromium.org/7031078/diff/2001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:292:
page_value->SetString("faviconDominantColor", "rgb(0, 147, 60)");
On 2011/06/09 20:37:23, tony wrote:
> Nit: Might be worth commenting what these colors are.  Or you could do
something
> like const char* kGreen = "rgb(...)";

I did once ask someone in a code review to do this, but have since decided it's
not worth it. The problem is I wouldn't know what to call it (this color is
different from what you get if you were to type just "green" in css). Seems the
best way to get an accurate idea of the color is to use some graphic tool. And
FWIW, we don't usually do this when colors are defined in css files, which is
pretty much the same situation.

[tony, 2011-06-09 21:05:25]

I agree.  I don't feel that strongly about this.

On 2011/06/09 20:51:06, Evan Stade wrote:
>
http://codereview.chromium.org/7031078/diff/2001/chrome/browser/ui/webui/ntp/...
> File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):
> 
>
http://codereview.chromium.org/7031078/diff/2001/chrome/browser/ui/webui/ntp/...
> chrome/browser/ui/webui/ntp/most_visited_handler.cc:292:
> page_value->SetString("faviconDominantColor", "rgb(0, 147, 60)");
> On 2011/06/09 20:37:23, tony wrote:
> > Nit: Might be worth commenting what these colors are.  Or you could do
> something
> > like const char* kGreen = "rgb(...)";
> 
> I did once ask someone in a code review to do this, but have since decided
it's
> not worth it. The problem is I wouldn't know what to call it (this color is
> different from what you get if you were to type just "green" in css). Seems
the
> best way to get an accurate idea of the color is to use some graphic tool. And
> FWIW, we don't usually do this when colors are defined in css files, which is
> pretty much the same situation.