Download large icon if available when creating secondary tiles on Windows 8.

Download large icon if available when creating secondary tiles on Windows 8.

This also fixes a couple of bugs with the background color calculation, where the lightness limit was invalid and the color of transparent pixels were being taken into account.

BUG=140942
TEST=Test sites with large favicons available (e.g. google.com, youtube.com) use them in their secondary tile.

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

[sky, 2012-10-26 20:01:15]

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
File chrome/browser/ui/metro_pin_tab_helper_win.cc (right):

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:158: class
MetroPinTabHelper::PagePinner
How come this is ref counted?

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:169: void ClearCurrentPageInfo();
Why do we need this?

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:181: // will be stored in
best_candidate_. If this is the last URL that was being
|s around references to parameters and fields.

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:238: if
(!in_progress_urls_.empty())
Should we create a pinner per url? Seems like it would simplify things in so far
as not having to worry about a new request coming in while processing one.

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:269:
FaviconUtil::DownloadFavicon(host, iter->icon_url, image_size);
You need to cache the return value and compare that in the IPC messages you get.

[benwells, 2012-10-29 06:38:41]

As discussed on chat I've separated things out. It feels nicer like this and
there is less code :)

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
File chrome/browser/ui/metro_pin_tab_helper_win.cc (right):

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:158: class
MetroPinTabHelper::PagePinner
On 2012/10/26 20:01:15, sky wrote:
> How come this is ref counted?

This class is now split into two. One half is not ref counted as it lives
totally on the UI thread. The other half is ref counted as it is created on the
UI thread and then runs tasks on the FILE thread.

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:169: void ClearCurrentPageInfo();
On 2012/10/26 20:01:15, sky wrote:
> Why do we need this?

This still exists, kind of, but is now in
MetroPinTabHelper::DidNavigateMainFrame. I've added a comment there, hopefully
it is clear (and correct).

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:181: // will be stored in
best_candidate_. If this is the last URL that was being
On 2012/10/26 20:01:15, sky wrote:
> |s around references to parameters and fields.

Done.

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:238: if
(!in_progress_urls_.empty())
On 2012/10/26 20:01:15, sky wrote:
> Should we create a pinner per url? Seems like it would simplify things in so
far
> as not having to worry about a new request coming in while processing one.

I've separated out the logic into a MetroPinTabHelper::FaviconDownloader class,
which is created as needed. There is still only one active but the
responsibilities are better encapsulated now, with nice clear roles.

http://codereview.chromium.org/11306005/diff/2001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:269:
FaviconUtil::DownloadFavicon(host, iter->icon_url, image_size);
On 2012/10/26 20:01:15, sky wrote:
> You need to cache the return value and compare that in the IPC messages you
get.

Done. Yeah I was checking the URLs when they come back but that's not totally
robust - some other thing might be downloading the same URL but with a different
size.

[sky, 2012-10-29 15:06:41]

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
File chrome/browser/ui/metro_pin_tab_helper_win.cc (right):

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:189: : title_(title), url_(url),
favicon_(favicon) {}
Since you wrapped the constructor each param on its own line.

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:217: HMODULE metro_module =
base::win::GetMetroModule();
I'm not familiar with the metro module, is is thread safe?

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:262: string16 title_;
const title_ and url_

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:331: if (best_candidate_.isNull())
{
Might this result in an image bigger than kMaxIconSize?

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:401: favicon_downloader_.reset();
Does this mean if the user pins something and the favicons are slow to download
the pin will fail? Is that expected?

[benwells, 2012-10-29 23:20:44]

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
File chrome/browser/ui/metro_pin_tab_helper_win.cc (right):

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:189: : title_(title), url_(url),
favicon_(favicon) {}
On 2012/10/29 15:06:41, sky wrote:
> Since you wrapped the constructor each param on its own line.

Done.

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:217: HMODULE metro_module =
base::win::GetMetroModule();
On 2012/10/29 15:06:41, sky wrote:
> I'm not familiar with the metro module, is is thread safe?

Yes, this part is. This code just calls a metro function, which sends a request
to the metro thread and returns straight away.

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:262: string16 title_;
On 2012/10/29 15:06:41, sky wrote:
> const title_ and url_

Done.

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:331: if (best_candidate_.isNull())
{
On 2012/10/29 15:06:41, sky wrote:
> Might this result in an image bigger than kMaxIconSize?

Oh yes, good catch.

http://codereview.chromium.org/11306005/diff/9001/chrome/browser/ui/metro_pin...
chrome/browser/ui/metro_pin_tab_helper_win.cc:401: favicon_downloader_.reset();
On 2012/10/29 15:06:41, sky wrote:
> Does this mean if the user pins something and the favicons are slow to
download
> the pin will fail? Is that expected?

I think it makes the most sense. The pin action comes up in a windows dialog
which feels quite separate from Chrome. Getting this coming up for a page you
were previously browsing would feel pretty strange.

I think this is an edge case, if it is flagged as a problem it should be easy to
change with this structure.

[sky, 2012-10-29 23:52:31]

LGTM - but could you wait on http://codereview.chromium.org/11232068/ . It
should simplify your code.

[benwells, 2012-10-30 00:37:19]

On 2012/10/29 23:52:31, sky wrote:
> LGTM - but could you wait on http://codereview.chromium.org/11232068/ . It
> should simplify your code.

That change is all in the renderer code and doesn't appear to change the IPC
messages sent through.

[commit-bot: I haz the power, 2012-10-30 01:23:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11306005/7002

[commit-bot: I haz the power, 2012-10-30 06:33:12]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-30 06:40:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11306005/7002

[commit-bot: I haz the power, 2012-10-30 08:36:19]

Change committed as 164878