Refactored loading of applications / extensions icons.

Refactored loading of applications / extensions icons.
Added ImageLoader::LoadExtensionIconAsync() and ImageLoader::LoadExtensionIconDataURLAsync().
Moved all apps / extension loading code to ImageLoader.

BUG=297298, 297301, 304180
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227477

[dvh, 2013-10-06 06:44:11]

finnur@,

https://codereview.chromium.org/25050005 has been reverted because of
crbug/304180.
Fixing it using copy constructor to pass gfx::Image and GURL between threads.

PTAL.

Thanks!

[Finnur, 2013-10-06 12:53:41]

Couple of questions...

1) Why is there a data race there? Do you feel you have a good handle on that?
... and ...
2) I forget, is copying a gfx::Image an expensive operation or is it cheap? As I
recall, we were using a structure (may have been SkBitmap) where copying this
data was cheap because it kept an internal ref count and didn't actually copy
all the bits. Is that the case here with gfx::Image? As I recall also, the GURL
does not do the same trick, so we'd be copying potentially a lot of information
with the URL based operation. Right?

[dvh, 2013-10-06 16:39:57]

On 2013/10/06 12:53:41, Finnur wrote:
> Couple of questions...
> 
> 1) Why is there a data race there? Do you feel you have a good handle on that?
> ... and ...
> 2) I forget, is copying a gfx::Image an expensive operation or is it cheap? As
I
> recall, we were using a structure (may have been SkBitmap) where copying this
> data was cheap because it kept an internal ref count and didn't actually copy
> all the bits. Is that the case here with gfx::Image? As I recall also, the
GURL
> does not do the same trick, so we'd be copying potentially a lot of
information
> with the URL based operation. Right?

1) given the address, I think gfx::Image is still on the stack when the data is
passed from a thread to an other (0x7d1000065e40 is probably within the stack).
2.1) gfx::Image has a cheap copy operation
2.2) GURL does not have a cheap copy. I'm working on it: I'll avoid copies of
GURL.

[dvh, 2013-10-06 19:14:59]

On 2013/10/06 16:39:57, dvh wrote:
> On 2013/10/06 12:53:41, Finnur wrote:
> > Couple of questions...
> > 
> > 1) Why is there a data race there? Do you feel you have a good handle on
that?
> > ... and ...
> > 2) I forget, is copying a gfx::Image an expensive operation or is it cheap?
As
> I
> > recall, we were using a structure (may have been SkBitmap) where copying
this
> > data was cheap because it kept an internal ref count and didn't actually
copy
> > all the bits. Is that the case here with gfx::Image? As I recall also, the
> GURL
> > does not do the same trick, so we'd be copying potentially a lot of
> information
> > with the URL based operation. Right?
> 
> 1) given the address, I think gfx::Image is still on the stack when the data
is
> passed from a thread to an other (0x7d1000065e40 is probably within the
stack).
> 2.1) gfx::Image has a cheap copy operation
> 2.2) GURL does not have a cheap copy. I'm working on it: I'll avoid copies of
> GURL.

After I had a look at the code generating the GURL. It already suffers of
several duplications.
- base::Base64Encode(std::string(reinterpret_cast<const char*>(mem->front()),
mem->size()), &contents_base64))
- GURL(std::string(chrome::kDataScheme) + kDataURLPrefix + contents_base64);
- GURL url = GetImageDataURL(image);

I'm wondering if it's worth optimizing that, given the use case (Apps Developer
Tools, which is an improve developer mode for chrome://extensions).
I feel that duplicating it good enough for that use case and won't introduce
complex code.

in_reply_to:
ahdzfmNocm9taXVtY29kZXJldmlldy1ocnIiCxIFSXNzdWUY6t68DAwLEgdNZXNzYWdlGICAgICA9I4KDA
send_mail: 1
subject: Refactored loading of applications / extensions icons.

[Finnur, 2013-10-06 22:15:55]

OK, LGTM. 

I'm wondering if we should add a comment somewhere to note that this is not
optimal, but I'll let you decide what the best way forward is.

[commit-bot: I haz the power, 2013-10-07 06:08:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dvh@chromium.org/26161002/20001

[commit-bot: I haz the power, 2013-10-07 06:48:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-07 16:15:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dvh@chromium.org/26161002/20001

[commit-bot: I haz the power, 2013-10-07 16:40:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-07 19:53:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dvh@chromium.org/26161002/20001

[commit-bot: I haz the power, 2013-10-07 21:03:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-08 04:37:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dvh@chromium.org/26161002/20001

[commit-bot: I haz the power, 2013-10-08 06:51:06]

Change committed as 227477