Consolidate icon loading on ChromeOS

Consolidate icon loading on ChromeOS.

BUG=110919, 109677
TEST=unit_tests --gtest_filter=FileIconSourceTest.* && browser_tests --gtest_filter=DownloadExtensionTest.DownloadsApi_FileIcon_*


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

[ilja, 2012-01-22 02:31:43]

In general it looks very good. Just two points:
1) CrOS doesn't really have 32x32 icons right now. It has 16x16 and 128x128. The
latter are sometimes reused for "normal". Also sometimes 16x16 have to be
substituted for 32x32 because we did not get designs yet for 32x32. I am not
sure what happens with your code and "large".
2) Windows and Linux etc. should translate an extension into a MIME type and
then do the lookup in their system to find the right theme icons. CrOS doesn't
have this, that's why the table is there. I am slightly worried that if one is
going to use fileicon: on Lin/Win/Mac they might pick up the icon from CrOS,
which would be bad. I don't see where the switch happens. Mind pointing me to
it?

http://codereview.chromium.org/9167019/diff/13001/chrome/browser/icon_loader_...
File chrome/browser/icon_loader_chromeos.cc (right):

http://codereview.chromium.org/9167019/diff/13001/chrome/browser/icon_loader_...
chrome/browser/icon_loader_chromeos.cc:198: case IconLoader::LARGE:  //
fallthrough
What happens when LARGE is requested? CrOS file manager uses some icons that are
128 large I believe. Not sure they would go through this path right now.

http://codereview.chromium.org/9167019/diff/13001/chrome/browser/ui/webui/fil...
File chrome/browser/ui/webui/fileicon_source.cc (right):

http://codereview.chromium.org/9167019/diff/13001/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source.cc:88: gfx::Image* icon =
im->LookupIcon(path, icon_size);
Shouldn't that still be named "escaped_filepath"?

[asanka, 2012-01-23 19:16:47]

http://codereview.chromium.org/9167019/diff/13001/chrome/browser/icon_loader_...
File chrome/browser/icon_loader_chromeos.cc (right):

http://codereview.chromium.org/9167019/diff/13001/chrome/browser/icon_loader_...
chrome/browser/icon_loader_chromeos.cc:198: case IconLoader::LARGE:  //
fallthrough
On 2012/01/22 02:31:43, ilja wrote:
> What happens when LARGE is requested? CrOS file manager uses some icons that
are
> 128 large I believe. Not sure they would go through this path right now.

The pixel size determined here is used to decide whether we need to resize the
image before sending it out. If an icon size of LARGE is requested (also the
case for ALL),  we return the image as-is without resizing.  If the size of the
resource was 128x128, then the resulting image would be 128x128.

Consequently, if we only have a 16px image, then:
  SMALL = 16px
  NORMAL = 16px resized to 32px
  LARGE = 16px

If we have a 16px small icon, and a 128px large icon,
  SMALL = 16px
  NORMAL = 128px resized to 32px
  LARGE = 128px

http://codereview.chromium.org/9167019/diff/13001/chrome/browser/ui/webui/fil...
File chrome/browser/ui/webui/fileicon_source.cc (right):

http://codereview.chromium.org/9167019/diff/13001/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source.cc:88: gfx::Image* icon =
im->LookupIcon(path, icon_size);
On 2012/01/22 02:31:43, ilja wrote:
> Shouldn't that still be named "escaped_filepath"?

The FilePath argument here shouldn't contain any escaping at this point.

[asanka, 2012-01-23 19:21:17]

Thanks for looking!

On 2012/01/22 02:31:43, ilja wrote:
> In general it looks very good. Just two points:
> 1) CrOS doesn't really have 32x32 icons right now. It has 16x16 and 128x128.
The
> latter are sometimes reused for "normal". Also sometimes 16x16 have to be
> substituted for 32x32 because we did not get designs yet for 32x32. I am not
> sure what happens with your code and "large".

Let me know if the explanation in the previous message didn't help clarifying
this.

> 2) Windows and Linux etc. should translate an extension into a MIME type and
> then do the lookup in their system to find the right theme icons. CrOS doesn't
> have this, that's why the table is there. I am slightly worried that if one is
> going to use fileicon: on Lin/Win/Mac they might pick up the icon from CrOS,
> which would be bad. I don't see where the switch happens. Mind pointing me to
> it?

icon_{manager,loader}_chromeos.cc should only be built on CrOS. That's done by
the chrome_browser.gypi change. On other platforms, we build the corresponding
icon_{manager,loader}_*.cc code.

> 
>
http://codereview.chromium.org/9167019/diff/13001/chrome/browser/icon_loader_...
> File chrome/browser/icon_loader_chromeos.cc (right):
> 
>
http://codereview.chromium.org/9167019/diff/13001/chrome/browser/icon_loader_...
> chrome/browser/icon_loader_chromeos.cc:198: case IconLoader::LARGE:  //
> fallthrough
> What happens when LARGE is requested? CrOS file manager uses some icons that
are
> 128 large I believe. Not sure they would go through this path right now.
> 
>
http://codereview.chromium.org/9167019/diff/13001/chrome/browser/ui/webui/fil...
> File chrome/browser/ui/webui/fileicon_source.cc (right):
> 
>
http://codereview.chromium.org/9167019/diff/13001/chrome/browser/ui/webui/fil...
> chrome/browser/ui/webui/fileicon_source.cc:88: gfx::Image* icon =
> im->LookupIcon(path, icon_size);
> Shouldn't that still be named "escaped_filepath"?

[achuithb, 2012-01-23 19:25:20]

Is this patch ready for review?

[asanka, 2012-01-23 19:51:16]

On 2012/01/23 19:25:20, achuith.bhandarkar wrote:
> Is this patch ready for review?

Yes. PTAL.

[achuithb, 2012-01-23 20:37:23]

On 2012/01/23 19:51:16, asanka wrote:
> On 2012/01/23 19:25:20, achuith.bhandarkar wrote:
> > Is this patch ready for review?
> 
> Yes. PTAL.

Should the BUG= line include 109677 as well?

[asanka, 2012-01-23 20:43:19]

On 2012/01/23 20:37:23, achuith.bhandarkar wrote:
> Should the BUG= line include 109677 as well?

I added that bug as well. I used a separate bug because the content of the CL
has deviated significantly from just fixing a failing test. :)

[achuithb, 2012-01-23 20:59:27]

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/icon_loader_...
File chrome/browser/icon_loader_chromeos.cc (right):

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/icon_loader_...
chrome/browser/icon_loader_chromeos.cc:7: #include <string>
alphabetical order please

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/resources/do...
chrome/browser/resources/downloads.js:398: encodeURIComponent(this.filePath_));
please make sure this is tested on a chromeos device...

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/ui/webui/fil...
File chrome/browser/ui/webui/fileicon_source.cc (right):

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source.cc:35: void GetFilePathAndQuery(const
std::string& url,
I assume GetFilePathAndQuery and QueryToIconSize are exercised by
fileicon_source_unittest?

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source.cc:70: CR_DEFINE_STATIC_LOCAL(
Do you think a comment would be useful here? I had to look up
CR_DEFINE_STATIC_LOCAL to figure out why you needed BuildQueryIconSizeMap(). I
realize we also do this in other places, but it's uncommon enough that a comment
wouldn't hurt.

[achuithb, 2012-01-23 21:01:10]

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/resources/do...
chrome/browser/resources/downloads.js:398: encodeURIComponent(this.filePath_));
On 2012/01/23 20:59:27, achuith.bhandarkar wrote:
> please make sure this is tested on a chromeos device...

I should've phrased this better - we should make sure this is tested on a
chromeos device before landing it. Do you have a chromeos device handy?

[asanka, 2012-01-23 21:15:15]

On 2012/01/23 21:01:10, achuith.bhandarkar wrote:
> On 2012/01/23 20:59:27, achuith.bhandarkar wrote:
> > please make sure this is tested on a chromeos device...
> 
> I should've phrased this better - we should make sure this is tested on a
> chromeos device before landing it. Do you have a chromeos device handy?

I tested on Linux ChromeOS and Linux ChromeOS Aura builds, but not on a ChromeOS
device.  I'll do that now and will ping you offline for help.

[achuithb, 2012-01-23 21:30:12]

On 2012/01/23 21:15:15, asanka wrote:
> On 2012/01/23 21:01:10, achuith.bhandarkar wrote:
> > On 2012/01/23 20:59:27, achuith.bhandarkar wrote:
> > > please make sure this is tested on a chromeos device...
> > 
> > I should've phrased this better - we should make sure this is tested on a
> > chromeos device before landing it. Do you have a chromeos device handy?
> 
> I tested on Linux ChromeOS and Linux ChromeOS Aura builds, but not on a
ChromeOS
> device.  I'll do that now and will ping you offline for help.

So my concern here is that we need to properly handle filenames with non-lower
ASCII chars. http://www.javascripter.net/faq/escape.htm has the list of chars
that are encoded differently between escape and encodeURIComponent. We should
test with a filename that has all of these characters and make sure that the
file icon shows up correctly. The filemanager may forbid some characters. 

You can try to download
http://achuithz600/www/files/o@%23$%25&*()+=%3f,:%3b%22.jpg

[achuithb, 2012-01-23 21:32:33]

On 2012/01/23 21:30:12, achuith.bhandarkar wrote:
> On 2012/01/23 21:15:15, asanka wrote:
> > On 2012/01/23 21:01:10, achuith.bhandarkar wrote:
> > > On 2012/01/23 20:59:27, achuith.bhandarkar wrote:
> > > > please make sure this is tested on a chromeos device...
> > > 
> > > I should've phrased this better - we should make sure this is tested on a
> > > chromeos device before landing it. Do you have a chromeos device handy?
> > 
> > I tested on Linux ChromeOS and Linux ChromeOS Aura builds, but not on a
> ChromeOS
> > device.  I'll do that now and will ping you offline for help.
> 
> So my concern here is that we need to properly handle filenames with non-lower
> ASCII chars. http://www.javascripter.net/faq/escape.htm has the list of chars
> that are encoded differently between escape and encodeURIComponent. We should
> test with a filename that has all of these characters and make sure that the
> file icon shows up correctly. The filemanager may forbid some characters. 
> 
> You can try to download
>
http://achuithz600/www/files/o%40%2523%24%2525&*%28%29+=%253f%2C:%253b%2522.jpg

Actually go to http://achuithz600/www/files/, right click on
o@#$%&*()+=?,:;".jpg and use save link as.

Asanka: Ilja has kindly agreed to help test this if you get stuck.

[asanka, 2012-01-26 00:49:34]

I tested this on an Alex device.  The code changed a bit in the meantime as well
as a result of some more cleanup.

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/icon_loader_...
File chrome/browser/icon_loader_chromeos.cc (right):

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/icon_loader_...
chrome/browser/icon_loader_chromeos.cc:7: #include <string>
On 2012/01/23 20:59:27, achuith.bhandarkar wrote:
> alphabetical order please

Done.

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/resources/do...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/resources/do...
chrome/browser/resources/downloads.js:398: encodeURIComponent(this.filePath_));
On 2012/01/23 21:01:10, achuith.bhandarkar wrote:
> On 2012/01/23 20:59:27, achuith.bhandarkar wrote:
> > please make sure this is tested on a chromeos device...
> 
> I should've phrased this better - we should make sure this is tested on a
> chromeos device before landing it. Do you have a chromeos device handy?

Done.

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/ui/webui/fil...
File chrome/browser/ui/webui/fileicon_source.cc (right):

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source.cc:35: void GetFilePathAndQuery(const
std::string& url,
On 2012/01/23 20:59:27, achuith.bhandarkar wrote:
> I assume GetFilePathAndQuery and QueryToIconSize are exercised by
> fileicon_source_unittest?

Yes.

http://codereview.chromium.org/9167019/diff/21001/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source.cc:70: CR_DEFINE_STATIC_LOCAL(
On 2012/01/23 20:59:27, achuith.bhandarkar wrote:
> Do you think a comment would be useful here? I had to look up
> CR_DEFINE_STATIC_LOCAL to figure out why you needed BuildQueryIconSizeMap(). I
> realize we also do this in other places, but it's uncommon enough that a
comment
> wouldn't hurt.

Done. This code actually changed a bit, and I'm not using this macro anymore.

[ilja, 2012-01-26 01:34:42]

Thanks for testing on Alex! I can't judge the escaping changes, but the rest
looks good to me. I defer to Achuith for the final call.

[achuithb, 2012-01-27 19:26:18]

lgtm

[asanka, 2012-01-27 23:06:18]

Thanks guys!

Evan: Adding you for OWNERS chrome/browser/ui/webui/ and
chrome/browser/resources/. Can you take a look?

[Evan Stade, 2012-01-31 04:42:46]

lgtm

http://codereview.chromium.org/9167019/diff/39001/chrome/browser/ui/webui/fil...
File chrome/browser/ui/webui/fileicon_source.h (right):

http://codereview.chromium.org/9167019/diff/39001/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source.h:44: // function is called to perform
the actual fetch.
nit: mention that it's virtual just for testing

[asanka, 2012-01-31 17:04:30]

Thanks!

http://codereview.chromium.org/9167019/diff/39001/chrome/browser/ui/webui/fil...
File chrome/browser/ui/webui/fileicon_source.h (right):

http://codereview.chromium.org/9167019/diff/39001/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source.h:44: // function is called to perform
the actual fetch.
On 2012/01/31 04:42:47, Evan Stade wrote:
> nit: mention that it's virtual just for testing

Done.

[commit-bot: I haz the power, 2012-01-31 17:56:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/9167019/45001

[commit-bot: I haz the power, 2012-01-31 19:33:57]

Change committed as 119935