Site specific secondary tiles for Windows 8.

Site specific secondary tiles for Windows 8.

When pages are pinned to the start screen, if there is a favicon available for the site it is used to generate the tile.

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

[benwells, 2012-10-19 01:42:12]

This change uses the history service, which only seems to have 16x16 favicons
available.

I'm not sure what the best approach is to get 32x32 icons, if available, but can
maybe improve once the basics land.

[sky, 2012-10-19 16:09:56]

https://chromiumcodereview.appspot.com/11198025/diff/5002/chrome/browser/ui/m...
File chrome/browser/ui/metro_pin_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/11198025/diff/5002/chrome/browser/ui/m...
chrome/browser/ui/metro_pin_tab_helper.cc:15: #if defined(OS_WIN)
Isn't this whole file windows specific?

https://chromiumcodereview.appspot.com/11198025/diff/5002/win8/metro_driver/s...
File win8/metro_driver/secondary_tile.cc (right):

https://chromiumcodereview.appspot.com/11198025/diff/5002/win8/metro_driver/s...
win8/metro_driver/secondary_tile.cc:44: static int k_logo_width = 120;
constants like this are kFooBar.

https://chromiumcodereview.appspot.com/11198025/diff/5002/win8/metro_driver/s...
win8/metro_driver/secondary_tile.cc:81: gfx::ImageSkia icon_image(*icon_bitmap);
Maybe this should take an ImageSkia rather than SkBitmap.

https://chromiumcodereview.appspot.com/11198025/diff/5002/win8/metro_driver/s...
win8/metro_driver/secondary_tile.cc:122: if
(!file_util::CopyFile(default_logo_path, logo_path))
What thread is this run on?

[cpu_(ooo_6.6-7.5), 2012-10-19 19:39:07]

http://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/chrome_ap...
File win8/metro_driver/chrome_app_view.cc (left):

http://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/chrome_ap...
win8/metro_driver/chrome_app_view.cc:29: 
I'll take care of this whitespace. Please remove from CL

http://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/secondary...
File win8/metro_driver/secondary_tile.cc (right):

http://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/secondary...
win8/metro_driver/secondary_tile.cc:49: static double k_box_fade = 0.75;
I don't see any metro specific calls here. Wouldn't it be better to do all this
skia plus file creation in chrome and just pass the paths to the metro driver?

[benwells, 2012-10-22 06:20:46]

https://codereview.chromium.org/11198025/diff/5002/chrome/browser/ui/metro_pi...
File chrome/browser/ui/metro_pin_tab_helper.cc (right):

https://codereview.chromium.org/11198025/diff/5002/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper.cc:15: #if defined(OS_WIN)
On 2012/10/19 16:09:56, sky wrote:
> Isn't this whole file windows specific?

It's only used on windows, yes. I've only put the minimal amount of code behind
#if's as it felt cleaner 

I can move more. E.g. the tab helper creation can go behind an #if in
tab_contents.cc, and this could become _win.

https://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/chrome_a...
File win8/metro_driver/chrome_app_view.cc (left):

https://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/chrome_a...
win8/metro_driver/chrome_app_view.cc:29: 
On 2012/10/19 19:39:08, cpu wrote:
> I'll take care of this whitespace. Please remove from CL

Done.

https://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/secondar...
File win8/metro_driver/secondary_tile.cc (right):

https://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/secondar...
win8/metro_driver/secondary_tile.cc:44: static int k_logo_width = 120;
On 2012/10/19 16:09:56, sky wrote:
> constants like this are kFooBar.

Done.

https://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/secondar...
win8/metro_driver/secondary_tile.cc:49: static double k_box_fade = 0.75;
On 2012/10/19 19:39:08, cpu wrote:
> I don't see any metro specific calls here. Wouldn't it be better to do all
this
> skia plus file creation in chrome and just pass the paths to the metro driver?

OK, yeah that sounds like a great idea! It will make development easier and get
all the file creation stuff happening on the FILE thread. I'll do that in the
next patch.

https://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/secondar...
win8/metro_driver/secondary_tile.cc:81: gfx::ImageSkia icon_image(*icon_bitmap);
On 2012/10/19 16:09:56, sky wrote:
> Maybe this should take an ImageSkia rather than SkBitmap.

Done.

https://codereview.chromium.org/11198025/diff/5002/win8/metro_driver/secondar...
win8/metro_driver/secondary_tile.cc:122: if
(!file_util::CopyFile(default_logo_path, logo_path))
On 2012/10/19 16:09:56, sky wrote:
> What thread is this run on?

This is all happening in the metro thread. I'll put up a patch with cpu's
suggestion, which will move this to the FILE thread.

[sky, 2012-10-22 15:51:20]

https://codereview.chromium.org/11198025/diff/5002/chrome/browser/ui/metro_pi...
File chrome/browser/ui/metro_pin_tab_helper.cc (right):

https://codereview.chromium.org/11198025/diff/5002/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper.cc:15: #if defined(OS_WIN)
On 2012/10/22 06:20:47, benwells wrote:
> On 2012/10/19 16:09:56, sky wrote:
> > Isn't this whole file windows specific?
> 
> It's only used on windows, yes. I've only put the minimal amount of code
behind
> #if's as it felt cleaner 
> 
> I can move more. E.g. the tab helper creation can go behind an #if in
> tab_contents.cc, and this could become _win.

Yes please. Additionally move this out of TabContents entirely. At this point
you should be using WebContentsUserData.

[benwells, 2012-10-22 20:27:01]

On 2012/10/22 15:51:20, sky wrote:
>
https://codereview.chromium.org/11198025/diff/5002/chrome/browser/ui/metro_pi...
> File chrome/browser/ui/metro_pin_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/11198025/diff/5002/chrome/browser/ui/metro_pi...
> chrome/browser/ui/metro_pin_tab_helper.cc:15: #if defined(OS_WIN)
> On 2012/10/22 06:20:47, benwells wrote:
> > On 2012/10/19 16:09:56, sky wrote:
> > > Isn't this whole file windows specific?
> > 
> > It's only used on windows, yes. I've only put the minimal amount of code
> behind
> > #if's as it felt cleaner 
> > 
> > I can move more. E.g. the tab helper creation can go behind an #if in
> > tab_contents.cc, and this could become _win.
> 
> Yes please. Additionally move this out of TabContents entirely. At this point
> you should be using WebContentsUserData.

Sure. I won't get another patch up for a little while (it's 7.30am here, just
woke up) and this should not impact on the rest of the change. Could you take a
look at that please? It might save an overnight round trip.

cpu: how does the metro driver look?

[sky, 2012-10-22 21:18:00]

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
File chrome/browser/ui/metro_pin_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
chrome/browser/ui/metro_pin_tab_helper.cc:40: string16 GenerateTileId(const
string16& url_str) {
Add descriptions.

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
chrome/browser/ui/metro_pin_tab_helper.cc:146: #if defined(OS_WIN)
It would be nice if you add some asserts around threads here.

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
chrome/browser/ui/metro_pin_tab_helper.cc:203: gfx::Image favicon =
favicon_tab_helper->GetFavicon();
I believe you want to DeepCopy the ImageSkia and pass that around.

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
File chrome/browser/ui/metro_pin_tab_helper.h (right):

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
chrome/browser/ui/metro_pin_tab_helper.h:18: // The TaskRunner handles running
tasks for this helper on the FILE thread.
Does this need to be public?

[benwells, 2012-10-23 10:34:25]

I'm not sure what you mean by moving it out of TabContents and use
WebContentsUserData. It does use WebContentsUserData - I think it was the first
one avi converted - but it is still in TabContents (like all the other ones from
what i can see).

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
File chrome/browser/ui/metro_pin_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
chrome/browser/ui/metro_pin_tab_helper.cc:40: string16 GenerateTileId(const
string16& url_str) {
On 2012/10/22 21:18:00, sky wrote:
> Add descriptions.

Done.

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
chrome/browser/ui/metro_pin_tab_helper.cc:146: #if defined(OS_WIN)
On 2012/10/22 21:18:00, sky wrote:
> It would be nice if you add some asserts around threads here.

Done.

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
chrome/browser/ui/metro_pin_tab_helper.cc:203: gfx::Image favicon =
favicon_tab_helper->GetFavicon();
On 2012/10/22 21:18:00, sky wrote:
> I believe you want to DeepCopy the ImageSkia and pass that around.

Done.

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
File chrome/browser/ui/metro_pin_tab_helper.h (right):

https://chromiumcodereview.appspot.com/11198025/diff/6007/chrome/browser/ui/m...
chrome/browser/ui/metro_pin_tab_helper.h:18: // The TaskRunner handles running
tasks for this helper on the FILE thread.
On 2012/10/22 21:18:00, sky wrote:
> Does this need to be public?

No, made private.

[sky, 2012-10-23 16:31:56]

My mistake, LGTM

[cpu_(ooo_6.6-7.5), 2012-10-23 20:17:01]

http://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_pi...
File chrome/browser/ui/metro_pin_tab_helper_win.cc (right):

http://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper_win.cc:48: static int kLogoWidth = 120;
you mean const int xxx = yy ?

http://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper_win.cc:71:
color_utils::CalculateKMeanColorOfPNG(icon_mem, 100, 665, sampler);
100 and 665 would be nice to have in constants above.

http://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper_win.cc:101: FilePath GetLogoPath(const
gfx::ImageSkia& image,
seems the name could be better, like MakeLogo, as is seems just a path
construction.

http://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper_win.cc:121: static const wchar_t
kDefaultLogoFileName[] = L"SecondaryTile.png";
don't need the static and its not used in most of our code.

http://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper_win.cc:128: return FilePath();
please extract 121-129 into a freestanding function.

http://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper_win.cc:155: 
you can fold the do nothing funtions like 152 and 154 in the declaration, line
139.

http://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper_win.cc:237: gfx::ImageSkia()));
I don't know how cheap is to make a gfx::ImageSkia here or to deep copy one in
line 227, consider instead allocating it in the heap or passing NULL here.

I guess I don't know if the posttask ends up copying all those bits again. Can
you check?

http://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper_win.cc:248: if (metro_module) {
rather do
if (!metro_module)
  return;

To have less nested blocks.

[benwells, 2012-10-24 00:48:17]

https://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_p...
File chrome/browser/ui/metro_pin_tab_helper_win.cc (right):

https://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_p...
chrome/browser/ui/metro_pin_tab_helper_win.cc:48: static int kLogoWidth = 120;
On 2012/10/23 20:17:01, cpu wrote:
> you mean const int xxx = yy ?

Done.

https://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_p...
chrome/browser/ui/metro_pin_tab_helper_win.cc:71:
color_utils::CalculateKMeanColorOfPNG(icon_mem, 100, 665, sampler);
On 2012/10/23 20:17:01, cpu wrote:
> 100 and 665 would be nice to have in constants above.

Done.

https://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_p...
chrome/browser/ui/metro_pin_tab_helper_win.cc:101: FilePath GetLogoPath(const
gfx::ImageSkia& image,
On 2012/10/23 20:17:01, cpu wrote:
> seems the name could be better, like MakeLogo, as is seems just a path
> construction.

Coming up with a name was hard as it did a variety of jobs. I've restructured a
bit which hopefully makes the flow more obvious (and the names more accurate).

https://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_p...
chrome/browser/ui/metro_pin_tab_helper_win.cc:121: static const wchar_t
kDefaultLogoFileName[] = L"SecondaryTile.png";
On 2012/10/23 20:17:01, cpu wrote:
> don't need the static and its not used in most of our code.

Done.

https://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_p...
chrome/browser/ui/metro_pin_tab_helper_win.cc:128: return FilePath();
On 2012/10/23 20:17:01, cpu wrote:
> please extract 121-129 into a freestanding function.

Done.

https://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_p...
chrome/browser/ui/metro_pin_tab_helper_win.cc:237: gfx::ImageSkia()));
On 2012/10/23 20:17:01, cpu wrote:
> I don't know how cheap is to make a gfx::ImageSkia here or to deep copy one in
> line 227, consider instead allocating it in the heap or passing NULL here.
> 
> I guess I don't know if the posttask ends up copying all those bits again. Can
> you check?

As discussed over chat, ImageSkia is cheap. The ImageSkia does get copied over
properly, I checked it during development.

I think the deep copy is needed, it was suggested by sky@.

https://codereview.chromium.org/11198025/diff/17006/chrome/browser/ui/metro_p...
chrome/browser/ui/metro_pin_tab_helper_win.cc:248: if (metro_module) {
On 2012/10/23 20:17:01, cpu wrote:
> rather do
> if (!metro_module)
>   return;
> 
> To have less nested blocks.

Done.

[cpu_(ooo_6.6-7.5), 2012-10-24 17:25:35]

lgtm

[commit-bot: I haz the power, 2012-10-24 20:56:18]

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

[commit-bot: I haz the power, 2012-10-24 22:51:58]

Change committed as 163943