Add a class to replace ImageLoadingTracker with a nicer API.

Add a class to replace ImageLoadingTracker with a nicer API.

This adds a new extensions::ImageLoader class, which mostly serves the same purpose as ImageLoadingTracker, but with some differences. It currently doesn't do any caching, and it uses callbacks instead of a delegate to pass the loaded image back to the caller. This should make this new class much nicer to use.
Also ported some ILT usage to the new ImageLoader class.

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

[Marijn Kruisselbrink, 2012-10-04 22:30:04]

This is sort of a continuation of mitchelwrosen's patch, although the
implementation is mostly rewritten to take into account some of Aaron's
comments.

There is no caching, and multiple sizes of the same image (LoadImagesAsync) are
loaded serially, not in seperate Tasks. With these changes there is no need for
a PendingLoadInfo type object anymore, which greatly simplifies things.
Also currently I didn't add the option of passing a MessageLoopProxy to the
LoadImage*Async methods; the usecase of getting the reply on a different
messageloop/thread than from where LoadImageAsync was called doesn't seem to be
that useful as far as I can tell.

I'm not entirely sure what to do about caching (and how is decided which places
use CACHE and which use DONT_CACHE). Most ILT users seem to use DONT_CACHE.
There are a couple that use CACHE (and all those places only seem to load one
resolution at the time). I don't know which of the DONT_CACHE using places do
expect the cache to be used for lookups though; at least ExtensionIconImage has
a lot of special-casing and behavior in its code for the case where an image is
retrieved from the cache, even though it gets images using DONT_CACHE.

[Finnur, 2012-10-05 14:41:33]

http://codereview.chromium.org/11027044/diff/1/chrome/browser/background/back...
File chrome/browser/background/background_application_list_model.cc (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/background/back...
chrome/browser/background/background_application_list_model.cc:159:
base::Bind(&Application::OnImageLoaded, AsWeakPtr()));
I suggest you add the people who used the CACHE parameter as reviewers for this
changelist to validate that not caching is OK.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image_unittest.cc (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image_unittest.cc:474: // TODO: use
caching api
TODO(owner), capitalize Use and end with period.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_install_prompt.h:45: public
base::SupportsWeakPtr<ExtensionInstallPrompt> {
If the second doesn't fit within 80 cols when aligned with the other public,
then I'd move the first public down one line so you can align them.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_install_prompt.h:243: void
OnImageLoaded(const gfx::Image& image);
Is there a reason to keep these public? Can you not bind against them if they
are private? I think the only reason they were public was because of an old
Chrome team rule: one shouldn't change the visibility of functions while
overriding them (inheriting a public function, the override should be public).

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_utils.cc (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils.cc:31: bool IsComponentExtensionResource(
This is copied from the ImageLoadingTracker. Did you svn copy the file to
preserve the commit history?

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils.cc:34: int& resource_id) {
You should use int* resource_id, not int&.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils.cc:132: continue;
We used to let the caller know when we were done, by sending a null bitmap. I
wonder if this will cause problems during conversion to this code. Just
something to keep in mind.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_utils_unittest.cc (left):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils_unittest.cc:146: EXPECT_EQ(1,
image_loaded_count());
Why remove this?

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils_unittest.cc:260: }
Same question here...

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_utils_unittest.cc (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils_unittest.cc:107: };
You writing code in invisible ink now? :)

[Aaron Boodman, 2012-10-05 16:46:03]

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_install_prompt.h:243: void
OnImageLoaded(const gfx::Image& image);
On 2012/10/05 14:41:33, Finnur wrote:
> Is there a reason to keep these public? Can you not bind against them if they
> are private? I think the only reason they were public was because of an old
> Chrome team rule: one shouldn't change the visibility of functions while
> overriding them (inheriting a public function, the override should be public).

I did not know about that rule. I very frequently do the opposite.

[Finnur, 2012-10-05 17:59:06]

I think this is one of those rules that someone instilled in me a long time ago
but wasn't actually a part of the style guide and probably doesn't make sense. I
asked Peter (who recently overhauled the style guide) and he said he often
instructed people to do the opposite. We should do the same.

[Marijn Kruisselbrink, 2012-10-05 18:00:08]

tbarzic/xiyuan: while trying to simplify ImageLoadingTracker it seems to me that
nearly every current user should probably instead be using
extensions::IconImage, is that true? If that is the case, would it make sense to
implement the caching that is currently done by ILT in IconImage instead?

http://codereview.chromium.org/11027044/diff/1/chrome/browser/background/back...
File chrome/browser/background/background_application_list_model.cc (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/background/back...
chrome/browser/background/background_application_list_model.cc:159:
base::Bind(&Application::OnImageLoaded, AsWeakPtr()));
On 2012/10/05 14:41:33, Finnur wrote:
> I suggest you add the people who used the CACHE parameter as reviewers for
this
> changelist to validate that not caching is OK.
It also seems that currently all the places that use CACHE should probably not
be using ILT/extension_image_utils directly, but use extensions::IconImage
(although the same can be said about practically every usage of ILT; I'm not
sure if there is any place where ILT is used for anything else but getting the
icon of an extension). If that's the case, maybe caching can be built into
IconImage instead. I'll add the people who wrote that as reviewers.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image_unittest.cc (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image_unittest.cc:474: // TODO: use
caching api
On 2012/10/05 14:41:33, Finnur wrote:
> TODO(owner), capitalize Use and end with period.

Done.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_install_prompt.h:45: public
base::SupportsWeakPtr<ExtensionInstallPrompt> {
On 2012/10/05 14:41:33, Finnur wrote:
> If the second doesn't fit within 80 cols when aligned with the other public,
> then I'd move the first public down one line so you can align them.

Done.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_install_prompt.h:243: void
OnImageLoaded(const gfx::Image& image);
On 2012/10/05 14:41:33, Finnur wrote:
> Is there a reason to keep these public? Can you not bind against them if they
> are private? I think the only reason they were public was because of an old
> Chrome team rule: one shouldn't change the visibility of functions while
> overriding them (inheriting a public function, the override should be public).

Done.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_utils.cc (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils.cc:31: bool IsComponentExtensionResource(
On 2012/10/05 14:41:33, Finnur wrote:
> This is copied from the ImageLoadingTracker. Did you svn copy the file to
> preserve the commit history?
I'm using the git workflow, so no svn copy; I could try passing a lower
--similarity to git cl upload to try to get it to detect this as a svn copy.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils.cc:34: int& resource_id) {
On 2012/10/05 14:41:33, Finnur wrote:
> You should use int* resource_id, not int&.
Ah yes, I fixed that in ILT in another CL but apparently copied this from an
older version (and I'll have to update this anyway to match the other changes to
IsComponentExtensionResource I made in that other CL).

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils.cc:132: continue;
On 2012/10/05 14:41:33, Finnur wrote:
> We used to let the caller know when we were done, by sending a null bitmap. I
> wonder if this will cause problems during conversion to this code. Just
> something to keep in mind.
ILT only internally needed to send itself a null bitmap to make sure the number
of bitmaps it received matched to number of representations it was waiting for,
but it never let the caller know of any of these internal steps; by loading all
representations as one task instead of individual tasks that part is no longer
needed.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_utils_unittest.cc (left):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils_unittest.cc:146: EXPECT_EQ(1,
image_loaded_count());
On 2012/10/05 14:41:33, Finnur wrote:
> Why remove this?
Because at the moment extension_image_utils doesn't do anything with the cache
(it not only doesn't save things in it, it also doesn't check if images already
are pre-cached). If it is desirable to always check if an image is in this cache
already, I could re-add this part of the test.

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_utils_unittest.cc (right):

http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_utils_unittest.cc:107: };
On 2012/10/05 14:41:33, Finnur wrote:
> You writing code in invisible ink now? :)

Done.

[xiyuan, 2012-10-05 18:35:33]

On 2012/10/05 18:00:08, Marijn Kruisselbrink wrote:
> tbarzic/xiyuan: while trying to simplify ImageLoadingTracker it seems to me
that
> nearly every current user should probably instead be using
> extensions::IconImage, is that true? If that is the case, would it make sense
to
> implement the caching that is currently done by ILT in IconImage instead?
> 

I agree that we should move cache out of ILT. But not so sure about putting it
into IconImage, e.g. in my http://codereview.chromium.org/11034027/, I am trying
to cache the icon after CrxInstaller installs it. It would be nice to have a
dedicated cache interface.

[Aaron Boodman, 2012-10-05 18:47:35]

I think we should remove the cache.

This is wrong layer to implement, and I can't think of any place where we
create so many copies of the same image that it would be critical.

I also agree with xiyuan, that if we need a cache we should make it a
standalone service that hangs off ExtensionSystem.

- a, via phone
On Oct 5, 2012 11:00 AM, <mek@chromium.org> wrote:

> tbarzic/xiyuan: while trying to simplify ImageLoadingTracker it seems to
> me that
> nearly every current user should probably instead be using
> extensions::IconImage, is that true? If that is the case, would it make
> sense to
> implement the caching that is currently done by ILT in IconImage instead?
>
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/background/background_**application_list_model.cc<http://codereview.chromium.org/11027044/diff/1/chrome/browser/background/background_application_list_model.cc>
> File chrome/browser/background/**background_application_list_**model.cc
> (right):
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/background/background_**application_list_model.cc#**newcode159<http://codereview.chromium.org/11027044/diff/1/chrome/browser/background/background_application_list_model.cc#newcode159>
> chrome/browser/background/**background_application_list_**model.cc:159:
> base::Bind(&Application::**OnImageLoaded, AsWeakPtr()));
> On 2012/10/05 14:41:33, Finnur wrote:
>
>> I suggest you add the people who used the CACHE parameter as reviewers
>>
> for this
>
>> changelist to validate that not caching is OK.
>>
> It also seems that currently all the places that use CACHE should
> probably not be using ILT/extension_image_utils directly, but use
> extensions::IconImage (although the same can be said about practically
> every usage of ILT; I'm not sure if there is any place where ILT is used
> for anything else but getting the icon of an extension). If that's the
> case, maybe caching can be built into IconImage instead. I'll add the
> people who wrote that as reviewers.
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/extension_**icon_image_unittest.cc<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/extension_icon_image_unittest.cc>
> File chrome/browser/extensions/**extension_icon_image_unittest.**cc
> (right):
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/extension_**icon_image_unittest.cc#**newcode474<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/extension_icon_image_unittest.cc#newcode474>
> chrome/browser/extensions/**extension_icon_image_unittest.**cc:474: //
> TODO:
> use caching api
> On 2012/10/05 14:41:33, Finnur wrote:
>
>> TODO(owner), capitalize Use and end with period.
>>
>
> Done.
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/extension_**install_prompt.h<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/extension_install_prompt.h>
> File chrome/browser/extensions/**extension_install_prompt.h (right):
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/extension_**install_prompt.h#newcode45<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/extension_install_prompt.h#newcode45>
> chrome/browser/extensions/**extension_install_prompt.h:45: public
> base::SupportsWeakPtr<**ExtensionInstallPrompt> {
> On 2012/10/05 14:41:33, Finnur wrote:
>
>> If the second doesn't fit within 80 cols when aligned with the other
>>
> public,
>
>> then I'd move the first public down one line so you can align them.
>>
>
> Done.
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/extension_**install_prompt.h#newcode243<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/extension_install_prompt.h#newcode243>
> chrome/browser/extensions/**extension_install_prompt.h:**243: void
> OnImageLoaded(const gfx::Image& image);
> On 2012/10/05 14:41:33, Finnur wrote:
>
>> Is there a reason to keep these public? Can you not bind against them
>>
> if they
>
>> are private? I think the only reason they were public was because of
>>
> an old
>
>> Chrome team rule: one shouldn't change the visibility of functions
>>
> while
>
>> overriding them (inheriting a public function, the override should be
>>
> public).
>
> Done.
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/image_**utils.cc<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/image_utils.cc>
> File chrome/browser/extensions/**image_utils.cc (right):
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/image_**utils.cc#newcode31<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/image_utils.cc#newcode31>
> chrome/browser/extensions/**image_utils.cc:31: bool
> IsComponentExtensionResource(
> On 2012/10/05 14:41:33, Finnur wrote:
>
>> This is copied from the ImageLoadingTracker. Did you svn copy the file
>>
> to
>
>> preserve the commit history?
>>
> I'm using the git workflow, so no svn copy; I could try passing a lower
> --similarity to git cl upload to try to get it to detect this as a svn
> copy.
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/image_**utils.cc#newcode34<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/image_utils.cc#newcode34>
> chrome/browser/extensions/**image_utils.cc:34: int& resource_id) {
> On 2012/10/05 14:41:33, Finnur wrote:
>
>> You should use int* resource_id, not int&.
>>
> Ah yes, I fixed that in ILT in another CL but apparently copied this
> from an older version (and I'll have to update this anyway to match the
> other changes to IsComponentExtensionResource I made in that other CL).
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/image_**utils.cc#newcode132<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/image_utils.cc#newcode132>
> chrome/browser/extensions/**image_utils.cc:132: continue;
> On 2012/10/05 14:41:33, Finnur wrote:
>
>> We used to let the caller know when we were done, by sending a null
>>
> bitmap. I
>
>> wonder if this will cause problems during conversion to this code.
>>
> Just
>
>> something to keep in mind.
>>
> ILT only internally needed to send itself a null bitmap to make sure the
> number of bitmaps it received matched to number of representations it
> was waiting for, but it never let the caller know of any of these
> internal steps; by loading all representations as one task instead of
> individual tasks that part is no longer needed.
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/image_**utils_unittest.cc<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/image_utils_unittest.cc>
> File chrome/browser/extensions/**image_utils_unittest.cc (left):
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/image_**utils_unittest.cc#oldcode146<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/image_utils_unittest.cc#oldcode146>
> chrome/browser/extensions/**image_utils_unittest.cc:146: EXPECT_EQ(1,
> image_loaded_count());
> On 2012/10/05 14:41:33, Finnur wrote:
>
>> Why remove this?
>>
> Because at the moment extension_image_utils doesn't do anything with the
> cache (it not only doesn't save things in it, it also doesn't check if
> images already are pre-cached). If it is desirable to always check if an
> image is in this cache already, I could re-add this part of the test.
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/image_**utils_unittest.cc<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/image_utils_unittest.cc>
> File chrome/browser/extensions/**image_utils_unittest.cc (right):
>
> http://codereview.chromium.**org/11027044/diff/1/chrome/**
>
browser/extensions/image_**utils_unittest.cc#newcode107<http://codereview.chromium.org/11027044/diff/1/chrome/browser/extensions/image_utils_unittest.cc#newcode107>
> chrome/browser/extensions/**image_utils_unittest.cc:107: };
> On 2012/10/05 14:41:33, Finnur wrote:
>
>> You writing code in invisible ink now? :)
>>
>
> Done.
>
>
http://codereview.chromium.**org/11027044/<http://codereview.chromium.org/110...
>

[Marijn Kruisselbrink, 2012-10-05 21:23:29]

On 2012/10/05 18:47:35, Aaron Boodman wrote:
> I think we should remove the cache.
> 
> This is wrong layer to implement, and I can't think of any place where we
> create so many copies of the same image that it would be critical.
> 
> I also agree with xiyuan, that if we need a cache we should make it a
> standalone service that hangs off ExtensionSystem.
The way I see it there are three parts to caching; there is the issue of when
something should be stored in a cache (which I think we all agree on should not
be in ILT or its replacement, but either completely separate or maybe in
something wrapping the image loading code), and then there is the question of
when should something be read from this cache. Do I understand it correctly that
the loading from the cache is also not something that should done by ILT/its
replacement (this would describe my current implementation of
extension_image_utils)?

If ILT/its replacement doesn't load images from this cache (wherever the cache
actually lives), presumably there would be some wrapper that does this
(otherwise it would need to be duplicated for every user of ILT), but then isn't
ILT/its replacement just an implementation detail for this wrapper? I can't
really think of a situation where you would want to load an image and always get
it from disk even if you already have it in memory (that's also no a usecase ILT
currently supports). So that would lead me to think ILT/its replacement should
just always try to get an image from this cache.

So then there is the third part, where the actual cache lives. Currently this is
in the actual Extension*, but I understand that we want to move this to a
separate class hanging of ExtensionSystem?

And then there is the other thing, that it would seem to me that all current
users of ILT should be using extensions::IconImage instead anyway, which would
kind of make ILT/extension_image_utils an implementation detail of that class
(although there are probably cases where you might want to use ILT to load
non-icon extension images, if I didn't miss anything there just doesn't seem to
be any such use yet).

So I guess what could work is: move the
HasCachedImage/SetCachedImage/GetCachedImage to a new class; keep the
extension_image_utils code as simple as possible, so no reading or writing from
cache; have extensions::IconImage try getting images from the cache, and if that
fails delegate to extension_image_utils to load the image.
The only thing I'm not sure about in this situation is when something would
actually get stored in this cache; on ChromeOS apparently on installation of an
extension, but are there any other cases?

[Aaron Boodman, 2012-10-05 21:42:32]

On Fri, Oct 5, 2012 at 2:23 PM,  <mek@chromium.org> wrote:
> On 2012/10/05 18:47:35, Aaron Boodman wrote:
>>
>> I think we should remove the cache.
>
>> This is wrong layer to implement, and I can't think of any place where we
>> create so many copies of the same image that it would be critical.
>
>
>> I also agree with xiyuan, that if we need a cache we should make it a
>> standalone service that hangs off ExtensionSystem.
>
> The way I see it there are three parts to caching; there is the issue of
> when
> something should be stored in a cache (which I think we all agree on should
> not
> be in ILT or its replacement, but either completely separate or maybe in
> something wrapping the image loading code), and then there is the question
> of
> when should something be read from this cache. Do I understand it correctly
> that
> the loading from the cache is also not something that should done by ILT/its
> replacement (this would describe my current implementation of
> extension_image_utils)?

I think we may be over-thinking this :).

The main problem with the current caching is that we rarely
have the same extension image being used in multiple places at the
same time in Chrome. The most common case that I can think of is if
you have multiple browser windows open, then any browser action icons
will be duplicated.

Even in those cases where duplication can occur, there is no single
component in the system that knows if it is likely to (the call sites
can't say whether the image ought to be cached or not - they don't
know if the user is going to create another browser window). So it
shouldn't be the call sites' job to tell the loader whether to cache or not.

This is the reason that I was advocating just removing this complexity.

That said... now that you got me thinking about it... it should be
easy to make your new image_utils into a stateful object and give it its
own internal cache. The new object would be owned by
ExtensionSystem and there'd be just one per profile. The cache would
be a set of weak ptrs. So if we happen to hit the case where the same
image is needed in multiple places at the same time, then we will
share it. But we won't waste memory in the typical case when that
doesn't occur.

I don't think there would be a need for a separate, simpler layer
below this stateful object, do you?

- a

[Marijn Kruisselbrink, 2012-10-05 22:50:04]

On 2012/10/05 21:42:32, Aaron Boodman wrote:
> On Fri, Oct 5, 2012 at 2:23 PM,  <mailto:mek@chromium.org> wrote:
> > On 2012/10/05 18:47:35, Aaron Boodman wrote:
> >>
> >> I think we should remove the cache.
> >
> >> This is wrong layer to implement, and I can't think of any place where we
> >> create so many copies of the same image that it would be critical.
> >
> >
> >> I also agree with xiyuan, that if we need a cache we should make it a
> >> standalone service that hangs off ExtensionSystem.
> >
> > The way I see it there are three parts to caching; there is the issue of
> > when
> > something should be stored in a cache (which I think we all agree on should
> > not
> > be in ILT or its replacement, but either completely separate or maybe in
> > something wrapping the image loading code), and then there is the question
> > of
> > when should something be read from this cache. Do I understand it correctly
> > that
> > the loading from the cache is also not something that should done by ILT/its
> > replacement (this would describe my current implementation of
> > extension_image_utils)?
> 
> I think we may be over-thinking this :).
> 
> The main problem with the current caching is that we rarely
> have the same extension image being used in multiple places at the
> same time in Chrome. The most common case that I can think of is if
> you have multiple browser windows open, then any browser action icons
> will be duplicated.
I'm not sure if at the same time matters? And there certainly are places where
the same images (especially for apps) are loaded over and over; every time the
new-tab-page is opened (if it isn't already open) all the app icons get loaded
from ILT, and similarly on ChromeOS every time the launcher is opened all the
icons for apps there get loaded from ILT. I don't think you want to have to load
every icon from disk every time the launcher is opened, so some caching is
needed at some point in the chain (evidenced by the point that
http://codereview.chromium.org/11034027/ even pre-caches these icons so that
even the first time the launcher is shown hopefully all IO is already done).

> Even in those cases where duplication can occur, there is no single
> component in the system that knows if it is likely to (the call sites
> can't say whether the image ought to be cached or not - they don't
> know if the user is going to create another browser window). So it
> shouldn't be the call sites' job to tell the loader whether to cache or not.
> 
> This is the reason that I was advocating just removing this complexity.
> 
> That said... now that you got me thinking about it... it should be
> easy to make your new image_utils into a stateful object and give it its
> own internal cache. The new object would be owned by
> ExtensionSystem and there'd be just one per profile. The cache would
> be a set of weak ptrs. So if we happen to hit the case where the same
> image is needed in multiple places at the same time, then we will
> share it. But we won't waste memory in the typical case when that
> doesn't occur.
> 
> I don't think there would be a need for a separate, simpler layer
> below this stateful object, do you?
The weak_ptr solution wouldn't solve the problem where you want to keep images
(like app icons) always cached in memory though (also I'm not sure what these
weak_ptrs would be pointing to; would you make ImageSkiaRep or SkBitmap inherit
base::SupportsWeakPtr or something like that?)

[Marijn Kruisselbrink, 2012-10-08 17:30:34]

In the latest patch set I moved loading of resources from the blocking pool back
to the UI thread (matching the same change that was made for ILT to fix some
race conditions).
Also since I now had to pass bitmaps from the UI thread to the blocking pool
task anyway, adding back in the loading from cache was pretty trivial (although
I realize we might or might not want to have a cache at all).

[xiyuan, 2012-10-08 17:35:44]

On 2012/10/08 17:30:34, Marijn Kruisselbrink wrote:
> In the latest patch set I moved loading of resources from the blocking pool
back
> to the UI thread (matching the same change that was made for ILT to fix some
> race conditions).
> Also since I now had to pass bitmaps from the UI thread to the blocking pool
> task anyway, adding back in the loading from cache was pretty trivial
(although
> I realize we might or might not want to have a cache at all).

Thank you for updating the change. I promise I'll leave ILT alone now. :)

I would say let's add the old cache logic and we could always change it later
when we have new plans. The cache is still useful (at least for the preload case
I have).

[Aaron Boodman, 2012-10-08 18:36:11]

On 2012/10/05 22:50:04, Marijn Kruisselbrink wrote:
> On 2012/10/05 21:42:32, Aaron Boodman wrote:
> > On Fri, Oct 5, 2012 at 2:23 PM,  <mailto:mek@chromium.org> wrote:
> > > On 2012/10/05 18:47:35, Aaron Boodman wrote:
> > >>
> > >> I think we should remove the cache.
> > >
> > >> This is wrong layer to implement, and I can't think of any place where we
> > >> create so many copies of the same image that it would be critical.
> > >
> > >
> > >> I also agree with xiyuan, that if we need a cache we should make it a
> > >> standalone service that hangs off ExtensionSystem.
> > >
> > > The way I see it there are three parts to caching; there is the issue of
> > > when
> > > something should be stored in a cache (which I think we all agree on
should
> > > not
> > > be in ILT or its replacement, but either completely separate or maybe in
> > > something wrapping the image loading code), and then there is the question
> > > of
> > > when should something be read from this cache. Do I understand it
correctly
> > > that
> > > the loading from the cache is also not something that should done by
ILT/its
> > > replacement (this would describe my current implementation of
> > > extension_image_utils)?
> > 
> > I think we may be over-thinking this :).
> > 
> > The main problem with the current caching is that we rarely
> > have the same extension image being used in multiple places at the
> > same time in Chrome. The most common case that I can think of is if
> > you have multiple browser windows open, then any browser action icons
> > will be duplicated.
> I'm not sure if at the same time matters? And there certainly are places where
> the same images (especially for apps) are loaded over and over; every time the
> new-tab-page is opened (if it isn't already open) all the app icons get loaded
> from ILT, and similarly on ChromeOS every time the launcher is opened all the
> icons for apps there get loaded from ILT. I don't think you want to have to
load
> every icon from disk every time the launcher is opened, so some caching is
> needed at some point in the chain (evidenced by the point that
> http://codereview.chromium.org/11034027/ even pre-caches these icons so that
> even the first time the launcher is shown hopefully all IO is already done).

Ah, I thought we were optimizing for memory usage.

I talked to Xiyuan about this privately. He says the only use case he has for
caching right now is his external installation case.

If we only have a few really compelling use cases, I think what I'd like to do
for now is to factor those out separately.

So, we'd remove the cache from Extension, and this library would not do any
caching. Xiyuan's code would observe EXTENSION_INSTALLED and we'd change that
notification to include the few icons that we currently need. His code can grab
them and cache them from that message.

- a

[Aaron Boodman, 2012-10-08 19:00:31]

After look at Xiyuan's code again, I'm worried that there will be no obvious
place in his change to listen to the EXTENSION_INSTALLED notification and catch
the loaded image.

So I change my mind again (sorry): I think that this utility should have
built-in caching, which means it should be an object and be owned by Profile or
ExtensionSystem or someone like that.

We can just hard-code for now the types of icons that we will cache, and over
time, perhaps we will come up with some more principled story.

http://codereview.chromium.org/11027044/diff/13001/chrome/browser/extensions/...
File chrome/browser/extensions/image_utils.cc (right):

http://codereview.chromium.org/11027044/diff/13001/chrome/browser/extensions/...
chrome/browser/extensions/image_utils.cc:155: if (extension->location() !=
Extension::COMPONENT)
If the location is component, then we should never go to the disk to try and
find this icon. But some of the branches below return false, so we will.

http://codereview.chromium.org/11027044/diff/13001/chrome/browser/extensions/...
chrome/browser/extensions/image_utils.cc:249: }
If we're done at this point, it would be nice to call the callback. You should
document that it could be re-entrant though.

[Marijn Kruisselbrink, 2012-10-08 20:24:51]

On 2012/10/08 19:00:31, Aaron Boodman wrote:
> So I change my mind again (sorry): I think that this utility should have
> built-in caching, which means it should be an object and be owned by Profile
or
> ExtensionSystem or someone like that.
I'm sorry, I'm not entirely sure what you want; is this an object that is just
responsible for keeping track of the cached image (so replacing
Extension::HasCachedImage/GetCachedImage/SetCachedImage) which is then used by
extension_image_utils to get cached images from? Or will this class actually
replace extension_image_utils (so essentially going back to most of what ILT
was, with the difference that it uses closures for callbacks and there is only
one instance per Profile).
Another option still is to keep extension_image_utils simple and add the storing
to cache behavior to a layer on top of that (although that would require the
caller to decide between using extension_image_utils directly or going through
this wrapper, which I guess we want to avoid).

> We can just hard-code for now the types of icons that we will cache, and over
> time, perhaps we will come up with some more principled story.
So instead of currently with ILT where the caller decides between CACHE and
NOCACHE the ILT replacement would have some heuristics to determine if something
should be stored in the cache?
>

http://codereview.chromium.org/11027044/diff/13001/chrome/browser/extensions/...
File chrome/browser/extensions/image_utils.cc (right):

http://codereview.chromium.org/11027044/diff/13001/chrome/browser/extensions/...
chrome/browser/extensions/image_utils.cc:155: if (extension->location() !=
Extension::COMPONENT)
On 2012/10/08 19:00:31, Aaron Boodman wrote:
> If the location is component, then we should never go to the disk to try and
> find this icon. But some of the branches below return false, so we will.
So should all the branches that return false have a NOTREACHED() to hopefully
check that this method is only called when a resource actually exists? Although
since this method can potentially be called with arbitrary chrome-extension://
urls that might be a bad idea. So this would either need some other output
("this is a component extension resource but I don't have a resource id") or
whatever place calls this method should do the location() check and deal with
the case of no resource id (even if IsComponentExtensionResource would do the
check, all the places that call it would still need to special case the
situation where it would return true but not return a valid resource id).

http://codereview.chromium.org/11027044/diff/13001/chrome/browser/extensions/...
chrome/browser/extensions/image_utils.cc:249: }
On 2012/10/08 19:00:31, Aaron Boodman wrote:
> If we're done at this point, it would be nice to call the callback. You should
> document that it could be re-entrant though.

Done. I wasn't sure if it made sense to complicate code for this particular
case/if it made sense to expose this sort of detail to users of LoadImage, but I
can see how it is desirable to not have to wait for the message loop to get the
image if it is available immediately anyway.

[Marijn Kruisselbrink, 2012-10-10 18:08:33]

okay, in my latest patchset I changed it from an extension_image_utils namespace
to an extensions::ImageLoader class. Currently this is a singleton, but I'm
guessing this should probably be accessed through ExtensionSystem instead? (the
slight complication there is that not all current ILT using places have direct
access to a Profile*, which makes porting those places slightly more involved,
but shouldn't be too big of a problem I think).

Also changed it such that on chromeos certain sizes of images will be cached.

[xiyuan, 2012-10-10 20:30:22]

I am not sure caching the images based on its size is a good idea. For example,
on ChromeOS, user could play with many apps during one session. Those apps
probably only show up once or twice but their icons are automatically cached
forever. This does not sound right to me.

Now I am a bit prefer to have a separate cache. ImageLoader checks the cache to
speed up loading but never auto set images to the cache. It's up to the caller
to decide whether to put the image into the cache. What do you guys think?

http://codereview.chromium.org/11027044/diff/24023/chrome/browser/extensions/...
File chrome/browser/extensions/image_loader.cc (right):

http://codereview.chromium.org/11027044/diff/24023/chrome/browser/extensions/...
chrome/browser/extensions/image_loader.cc:261: bitmaps[i] = new
SkBitmap(*bitmap);
Would using a SkBitmap* gives extra benefits? If not, suggest to just use
SkBitmap. Copying/Assigning a SkBitmap is cheap as it mostly just add ref to its
internal pixel buffer.

http://codereview.chromium.org/11027044/diff/24023/chrome/browser/extensions/...
File chrome/browser/extensions/image_loader.h (right):

http://codereview.chromium.org/11027044/diff/24023/chrome/browser/extensions/...
chrome/browser/extensions/image_loader.h:170: //
nit: delete it or finish it? :)

[Marijn Kruisselbrink, 2012-10-10 22:33:03]

On 2012/10/10 20:30:22, xiyuan wrote:
> I am not sure caching the images based on its size is a good idea. For
example,
> on ChromeOS, user could play with many apps during one session. Those apps
> probably only show up once or twice but their icons are automatically cached
> forever. This does not sound right to me.
Yeah, I'm not entirely sure it makes sense this way either.

> Now I am a bit prefer to have a separate cache. ImageLoader checks the cache
to
> speed up loading but never auto set images to the cache. It's up to the caller
> to decide whether to put the image into the cache. What do you guys think?
I'm not sure when which caller would decide to actually put something in the
cache, but if there is some clear place for that, I could definitely live with
that. It would significantly simplify ImageLoader since there would no longer be
the need to track pending loads and uninstalled extensions.

http://codereview.chromium.org/11027044/diff/24023/chrome/browser/extensions/...
File chrome/browser/extensions/image_loader.cc (right):

http://codereview.chromium.org/11027044/diff/24023/chrome/browser/extensions/...
chrome/browser/extensions/image_loader.cc:261: bitmaps[i] = new
SkBitmap(*bitmap);
On 2012/10/10 20:30:22, xiyuan wrote:
> Would using a SkBitmap* gives extra benefits? If not, suggest to just use
> SkBitmap. Copying/Assigning a SkBitmap is cheap as it mostly just add ref to
its
> internal pixel buffer.
I think initially I thought I needed to be able to distinguish having no bitmap
from having a null bitmap, but actually I don't think I actually need that. So
changed this to get rid of the icky pointers.

http://codereview.chromium.org/11027044/diff/24023/chrome/browser/extensions/...
File chrome/browser/extensions/image_loader.h (right):

http://codereview.chromium.org/11027044/diff/24023/chrome/browser/extensions/...
chrome/browser/extensions/image_loader.h:170: //
On 2012/10/10 20:30:22, xiyuan wrote:
> nit: delete it or finish it? :)

Done.

[xiyuan, 2012-10-11 00:41:08]

On 2012/10/10 22:33:03, Marijn Kruisselbrink wrote:
> I'm not sure when which caller would decide to actually put something in the
> cache, but if there is some clear place for that, I could definitely live with
> that. It would significantly simplify ImageLoader since there would no longer
be
> the need to track pending loads and uninstalled extensions.

I think that's why aa suggested to get rid of the cache entirely in the
beginning. It's not quite useful yet makes code too complicated. So far, the
only use case I have for the cache is to preload the default app icons. And I
could do it without the cache by making ChromeLauncherController to observe
EXTENSION_INSTALLED and store the preloaded images in LauncherAppIconLoader
somehow.

[Aaron Boodman, 2012-10-11 02:36:10]

On 2012/10/10 20:30:22, xiyuan wrote:
> I am not sure caching the images based on its size is a good idea. For
example,
> on ChromeOS, user could play with many apps during one session. Those apps
> probably only show up once or twice but their icons are automatically cached
> forever. This does not sound right to me.
> 
> Now I am a bit prefer to have a separate cache. ImageLoader checks the cache
to
> speed up loading but never auto set images to the cache. It's up to the caller
> to decide whether to put the image into the cache. What do you guys think?

There are a number of ways to optimize this:

- We could only cache when IsExternalLocation(). (this would still
over-exuberantly cache after first install, though)
- We could have public API on ImageLoader to mark certain extensions as
cacheable. CRXInstaller would call this at the appropriate time.

I realize these both feel icky, but it's a weird edge case that I can't think of
how to handle any other way without over complicating everything else.

[Marijn Kruisselbrink, 2012-10-17 16:52:19]

Since apparently every alternative way to do caching compared to the current
situation has its own set of problems, would it be such a bad thing to keep
caching working the way it works in ILT, where the caller of LoadImage decides
if something should be cached? Then a separate bug/CL could be made to figure
out ways to improve the caching; even without the caching changes, I think this
would already be a worthwhile change?

[xiyuan, 2012-10-17 17:01:10]

On 2012/10/17 16:52:19, Marijn Kruisselbrink wrote:
> Since apparently every alternative way to do caching compared to the current
> situation has its own set of problems, would it be such a bad thing to keep
> caching working the way it works in ILT, where the caller of LoadImage decides
> if something should be cached? Then a separate bug/CL could be made to figure
> out ways to improve the caching; even without the caching changes, I think
this
> would already be a worthwhile change?

I'd say, let's get rid of cache from ImageLoader completely and see how bad it
would be. Then we could find out whether we need a cache and spend more time to
decide where the cache should live and how it should be used.

[Aaron Boodman, 2012-10-17 17:15:13]

I'm fine removing caching entirely :). But, I also think my earlier proposal
about having an API to mark a particular extension's image(s) as cacheable at
runtime would be a fine solution to the one case we have where caching is
currently needed (xiyuan's case).

http://codereview.chromium.org/11027044/diff/15009/chrome/browser/extensions/...
File chrome/browser/extensions/image_loader.h (right):

http://codereview.chromium.org/11027044/diff/15009/chrome/browser/extensions/...
chrome/browser/extensions/image_loader.h:42: // returns the callback has been
notified.
I think "reentrant" is the right word here?

[xiyuan, 2012-10-17 17:33:19]

Let's forget about my preloading CL for now. Moving icon loading to blocking
pool might be enough to solve the slow icon loading on first sign-in issue.

If we need to do more work, then we could either do caching certain extensions
in ImageLoader or passing preloaded icons via EXTENSION_INSTALLED notification
(both are aa's suggestions. :).

[Marijn Kruisselbrink, 2012-10-25 21:13:54]

Okay, I now removed caching entirely again, but keeping ImageLoader as a class
instead of a namespace to keep it possible to add caching back in later without
having to change API too much.
Also changed ImageLoader from being a singleton to hanging of from
ExtensionSystem.

[xiyuan, 2012-10-26 01:11:57]

LGTM

Thank you for making the changes.

http://codereview.chromium.org/11027044/diff/34001/chrome/browser/extensions/...
File chrome/browser/extensions/image_loader.cc (right):

http://codereview.chromium.org/11027044/diff/34001/chrome/browser/extensions/...
chrome/browser/extensions/image_loader.cc:28: std::string SizeToString(const
gfx::Size& max_size) {
nit: seems no longer needed.

[Marijn Kruisselbrink, 2012-10-30 23:11:07]

aa: what do you think of the current patch (ImageLoader class without caching)?

http://codereview.chromium.org/11027044/diff/34001/chrome/browser/extensions/...
File chrome/browser/extensions/image_loader.cc (right):

http://codereview.chromium.org/11027044/diff/34001/chrome/browser/extensions/...
chrome/browser/extensions/image_loader.cc:28: std::string SizeToString(const
gfx::Size& max_size) {
On 2012/10/26 01:11:57, xiyuan wrote:
> nit: seems no longer needed.

Done.

[Aaron Boodman, 2012-10-31 07:38:22]

I think it's fantastic. When can ILT be deleted? :)

http://codereview.chromium.org/11027044/diff/42001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/11027044/diff/42001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:555:
extensions::ExtensionSystem::Get(profile_)->image_loader()->LoadImageAsync(
I think ImageLoader should be its own PKS, not owned by ExtensionSystem.

[Marijn Kruisselbrink, 2012-10-31 21:22:59]

http://codereview.chromium.org/11027044/diff/42001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/11027044/diff/42001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:555:
extensions::ExtensionSystem::Get(profile_)->image_loader()->LoadImageAsync(
On 2012/10/31 07:38:22, Aaron Boodman wrote:
> I think ImageLoader should be its own PKS, not owned by ExtensionSystem.

Done.

[Yoyo Zhou, 2012-11-01 17:53:40]

On 2012/10/31 21:22:59, Marijn Kruisselbrink wrote:
>
http://codereview.chromium.org/11027044/diff/42001/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_install_prompt.cc (right):
> 
>
http://codereview.chromium.org/11027044/diff/42001/chrome/browser/extensions/...
> chrome/browser/extensions/extension_install_prompt.cc:555:
> extensions::ExtensionSystem::Get(profile_)->image_loader()->LoadImageAsync(
> On 2012/10/31 07:38:22, Aaron Boodman wrote:
> > I think ImageLoader should be its own PKS, not owned by ExtensionSystem.
> 
> Done.

image_loader_factory looks missing from your CL.

[Marijn Kruisselbrink, 2012-11-01 18:01:12]

On 2012/11/01 17:53:40, Yoyo Zhou wrote:
> image_loader_factory looks missing from your CL.
Oops, added.

[Aaron Boodman, 2012-11-08 21:07:22]

lgtm

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/extensions/...
File chrome/browser/extensions/image_loader_factory.h (right):

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/extensions/...
chrome/browser/extensions/image_loader_factory.h:15: class ImageLoader;
Can you please create a class template for this in like chrome/browser/profiles?
It pains me to see so many copies of this pattern.

[Marijn Kruisselbrink, 2012-11-08 23:59:35]

On 2012/11/08 21:07:22, Aaron Boodman wrote:
> lgtm
> 
>
http://codereview.chromium.org/11027044/diff/49024/chrome/browser/extensions/...
> File chrome/browser/extensions/image_loader_factory.h (right):
> 
>
http://codereview.chromium.org/11027044/diff/49024/chrome/browser/extensions/...
> chrome/browser/extensions/image_loader_factory.h:15: class ImageLoader;
> Can you please create a class template for this in like
chrome/browser/profiles?
> It pains me to see so many copies of this pattern.
How much of the factory implementation would such a class template provide?
Should it have bool (or enum) template parameters for the various
IsCreatedWithProfile, IsNULLWhileTesting and Incognito related bits, or are
factories still expected to just override these virtuals themselves if they need
non-default behavior?
I assume at least dependencies between factories is definitely not something
that a class template should attempt to provide.
And the last complication: ProfiledKeyedBaseFactory's constructor has a const
char* name (only used for debugging). Should this be set to something like
typeid(T).name(), or would the class template have a char* template argument
(which would have the problem that you can't directly pass string literals as
template arguments).

[Yoyo Zhou, 2012-11-09 00:14:24]

On 2012/11/08 23:59:35, Marijn Kruisselbrink wrote:
> On 2012/11/08 21:07:22, Aaron Boodman wrote:
> > lgtm
> > 
> >
>
http://codereview.chromium.org/11027044/diff/49024/chrome/browser/extensions/...
> > File chrome/browser/extensions/image_loader_factory.h (right):
> > 
> >
>
http://codereview.chromium.org/11027044/diff/49024/chrome/browser/extensions/...
> > chrome/browser/extensions/image_loader_factory.h:15: class ImageLoader;
> > Can you please create a class template for this in like
> chrome/browser/profiles?
> > It pains me to see so many copies of this pattern.
> How much of the factory implementation would such a class template provide?
> Should it have bool (or enum) template parameters for the various
> IsCreatedWithProfile, IsNULLWhileTesting and Incognito related bits, or are
> factories still expected to just override these virtuals themselves if they
need
> non-default behavior?
> I assume at least dependencies between factories is definitely not something
> that a class template should attempt to provide.
> And the last complication: ProfiledKeyedBaseFactory's constructor has a const
> char* name (only used for debugging). Should this be set to something like
> typeid(T).name(), or would the class template have a char* template argument
> (which would have the problem that you can't directly pass string literals as
> template arguments).

By the way, I've started doing the factory template for crbug.com/159265, so you
are welcome to not do it in this CL.

[Marijn Kruisselbrink, 2012-11-09 17:39:55]

for OWNERS reviews:
atwilson -> chrome/browser/background/
xians -> chrome/browser/media/
jhawkins -> chrome/browser/ui/webui/extensions/

[James Hawkins, 2012-11-09 17:42:21]

webui lgtm

[no longer working on chromium, 2012-11-09 19:07:10]

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:129:
icon_tracker_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
Without looking the details, just want to double check if this is an issue:
MediaStreamCaptureIndicator is created on the main thread, and it will destruct
the icon_tracker_ in UI thread, will this be a problem?

You can try it out by go to https://apprtc.appspot.com, click allow, then close
the browser.

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:278:
Profile::FromBrowserContext(web_contents->GetBrowserContext());
Check the web_contents before using it, in some cases it can be NULL.

[Marijn Kruisselbrink, 2012-11-10 00:19:19]

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:129:
icon_tracker_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
On 2012/11/09 19:07:10, xians1 wrote:
> Without looking the details, just want to double check if this is an issue:
> MediaStreamCaptureIndicator is created on the main thread, and it will
destruct
> the icon_tracker_ in UI thread, will this be a problem?
From trying this, it seems creation/usage is on the UI thread, but destruction
is on some other thread. And that can indeed be a problem. I guess I'll have to
come up with a solution without weak pointers then. I'm also not sure how
important it is that the DisplayBalloon in OnImageLoaded doesn't get called if
Hide() has been called before OnImageLoaded is invoked? If this happening is not
a problem I can just remove all the weak pointer stuff, otherwise I'll have to
add some "invalidated" flag (which should be thread safe, since all places where
this flag could be used already ensure they are on the UI thread).

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:278:
Profile::FromBrowserContext(web_contents->GetBrowserContext());
On 2012/11/09 19:07:10, xians1 wrote:
> Check the web_contents before using it, in some cases it can be NULL.
If the web_contents would be NULL, GetExtension would already have returned NULL
so we wouldn't even get to this code. But you're right that it would be good to
make that more explicit.

[Andrew T Wilson (Slow), 2012-11-13 08:41:20]

browser/background LGTM

[no longer working on chromium, 2012-11-13 15:17:29]

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:129:
icon_tracker_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
On 2012/11/10 00:19:19, Marijn Kruisselbrink wrote:
> On 2012/11/09 19:07:10, xians1 wrote:
> > Without looking the details, just want to double check if this is an issue:
> > MediaStreamCaptureIndicator is created on the main thread, and it will
> destruct
> > the icon_tracker_ in UI thread, will this be a problem?
> From trying this, it seems creation/usage is on the UI thread, but destruction
> is on some other thread. And that can indeed be a problem. I guess I'll have
to
> come up with a solution without weak pointers then. I'm also not sure how
> important it is that the DisplayBalloon in OnImageLoaded doesn't get called if
> Hide() has been called before OnImageLoaded is invoked? If this happening is
not
> a problem I can just remove all the weak pointer stuff, otherwise I'll have to
> add some "invalidated" flag (which should be thread safe, since all places
where
> this flag could be used already ensure they are on the UI thread).

After Hide() is called, it means no tab is using the capture devices, so it
should not show any UI indicators like balloon or image.

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:278:
Profile::FromBrowserContext(web_contents->GetBrowserContext());
On 2012/11/10 00:19:19, Marijn Kruisselbrink wrote:
> On 2012/11/09 19:07:10, xians1 wrote:
> > Check the web_contents before using it, in some cases it can be NULL.
> If the web_contents would be NULL, GetExtension would already have returned
NULL
> so we wouldn't even get to this code. But you're right that it would be good
to
> make that more explicit.

True, extension should be enough to safe guard it, I just missed it. Please just
ignore my comment.

[Marijn Kruisselbrink, 2012-11-14 17:35:27]

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/11027044/diff/49024/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:129:
icon_tracker_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
On 2012/11/13 15:17:29, xians1 wrote:
> On 2012/11/10 00:19:19, Marijn Kruisselbrink wrote:
> > On 2012/11/09 19:07:10, xians1 wrote:
> > > Without looking the details, just want to double check if this is an
issue:
> > > MediaStreamCaptureIndicator is created on the main thread, and it will
> > destruct
> > > the icon_tracker_ in UI thread, will this be a problem?
> > From trying this, it seems creation/usage is on the UI thread, but
destruction
> > is on some other thread. And that can indeed be a problem. I guess I'll have
> to
> > come up with a solution without weak pointers then. I'm also not sure how
> > important it is that the DisplayBalloon in OnImageLoaded doesn't get called
if
> > Hide() has been called before OnImageLoaded is invoked? If this happening is
> not
> > a problem I can just remove all the weak pointer stuff, otherwise I'll have
to
> > add some "invalidated" flag (which should be thread safe, since all places
> where
> > this flag could be used already ensure they are on the UI thread).
> 
> After Hide() is called, it means no tab is using the capture devices, so it
> should not show any UI indicators like balloon or image.
Okay, replaced the weak_ptr_factory (which would have had threading issues) with
a simple flag (which is safe since every method accessing it is always executed
on the UI thread).

[no longer working on chromium, 2012-11-15 16:12:55]

chrome/browser/media lgtm

[commit-bot: I haz the power, 2012-11-15 16:14:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/11027044/59001

[commit-bot: I haz the power, 2012-11-15 16:14:35]

Failed to apply patch for chrome/browser/extensions/extension_install_prompt.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_install_prompt.h
  Hunk #1 FAILED at 13.
  1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_install_prompt.h.rej

Patch:       chrome/browser/extensions/extension_install_prompt.h
Index: chrome/browser/extensions/extension_install_prompt.h
diff --git a/chrome/browser/extensions/extension_install_prompt.h
b/chrome/browser/extensions/extension_install_prompt.h
index
4cf1cb2bbbfac7f157d4465a7880b33add2aed2b..562d7d1ec222368d9346b525290f6caf299382d2
100644
--- a/chrome/browser/extensions/extension_install_prompt.h
+++ b/chrome/browser/extensions/extension_install_prompt.h
@@ -13,7 +13,6 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
 #include "chrome/browser/extensions/crx_installer_error.h"
-#include "chrome/browser/extensions/image_loading_tracker.h"
 #include "chrome/common/extensions/url_pattern.h"
 #include "google_apis/gaia/oauth2_mint_token_flow.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -43,8 +42,9 @@ class PermissionSet;
 }  // namespace extensions
 
 // Displays all the UI around extension installation.
-class ExtensionInstallPrompt : public ImageLoadingTracker::Observer,
-                               public OAuth2MintTokenFlow::Delegate {
+class ExtensionInstallPrompt
+    : public OAuth2MintTokenFlow::Delegate,
+      public base::SupportsWeakPtr<ExtensionInstallPrompt> {
  public:
   enum PromptType {
     UNSET_PROMPT_TYPE = -1,
@@ -260,11 +260,6 @@ class ExtensionInstallPrompt : public
ImageLoadingTracker::Observer,
   // Installation failed. This is declared virtual for testing.
   virtual void OnInstallFailure(const extensions::CrxInstallerError& error);
 
-  // ImageLoadingTracker::Observer:
-  virtual void OnImageLoaded(const gfx::Image& image,
-                             const std::string& extension_id,
-                             int index) OVERRIDE;
-
  protected:
   friend class extensions::ExtensionWebstorePrivateApiTest;
   friend class WebstoreStandaloneInstallUnpackFailureTest;
@@ -280,6 +275,9 @@ class ExtensionInstallPrompt : public
ImageLoadingTracker::Observer,
   // an empty bitmap, then a default icon will be used instead.
   void SetIcon(const SkBitmap* icon);
 
+  // extension_image_utils callback.
+  void OnImageLoaded(const gfx::Image& image);
+
   // Starts the process of showing a confirmation UI, which is split into two.
   // 1) Set off a 'load icon' task.
   // 2) Handle the load icon response and show the UI (OnImageLoaded).
@@ -319,6 +317,8 @@ class ExtensionInstallPrompt : public
ImageLoadingTracker::Observer,
   // The delegate we will call Proceed/Abort on after confirmation UI.
   Delegate* delegate_;
 
+  Profile* profile_;
+
   // A pre-filled prompt.
   Prompt prompt_;
 
@@ -327,10 +327,6 @@ class ExtensionInstallPrompt : public
ImageLoadingTracker::Observer,
 
   scoped_ptr<OAuth2MintTokenFlow> token_flow_;
 
-  // Keeps track of extension images being loaded on the File thread for the
-  // purpose of showing the install UI.
-  ImageLoadingTracker tracker_;
-
   // Used to show the confirm dialog.
   ShowDialogCallback show_dialog_callback_;
 };

[commit-bot: I haz the power, 2012-11-15 18:52:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/11027044/65001

[commit-bot: I haz the power, 2012-11-15 19:52:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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, 2012-11-15 19:52:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/11027044/39002

[commit-bot: I haz the power, 2012-11-15 19:52:37]

Failed to apply patch for chrome/browser/extensions/image_loading_tracker.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/image_loading_tracker.cc
  Hunk #1 FAILED at 9.
  Hunk #2 succeeded at 309 (offset 1 line).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/image_loading_tracker.cc.rej

Patch:       chrome/browser/extensions/image_loading_tracker.cc
Index: chrome/browser/extensions/image_loading_tracker.cc
diff --git a/chrome/browser/extensions/image_loading_tracker.cc
b/chrome/browser/extensions/image_loading_tracker.cc
index
f8885ab7840a20a8c0a9ba73dd8b0a5cbb8228b8..cfd920998cdda93ca8f41e65e4b5ca6735621561
100644
--- a/chrome/browser/extensions/image_loading_tracker.cc
+++ b/chrome/browser/extensions/image_loading_tracker.cc
@@ -9,19 +9,16 @@
 
 #include "base/bind.h"
 #include "base/file_util.h"
-#include "base/path_service.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "chrome/browser/extensions/image_loader.h"
 #include "chrome/browser/ui/webui/extensions/extension_icon_source.h"
 #include "chrome/common/chrome_notification_types.h"
-#include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_file_util.h"
 #include "chrome/common/extensions/extension_resource.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
-#include "grit/component_extension_resources_map.h"
-#include "grit/theme_resources.h"
 #include "skia/ext/image_operations.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -308,68 +305,14 @@ void ImageLoadingTracker::LoadImages(
       loader_ = new ImageLoader(this);
 
     int resource_id = -1;
-    if (IsComponentExtensionResource(extension, it->resource.relative_path(),
-                                     &resource_id))
+    if (extensions::ImageLoader::IsComponentExtensionResource(
+        extension, it->resource.relative_path(), &resource_id))
       loader_->LoadResource(*it, id, resource_id);
     else
       loader_->LoadImage(*it, id);
   }
 }
 
-bool ImageLoadingTracker::IsComponentExtensionResource(
-    const Extension* extension,
-    const FilePath& resource_path,
-    int* resource_id) {
-  static const GritResourceMap kExtraComponentExtensionResources[] = {
-    {"web_store/webstore_icon_128.png", IDR_WEBSTORE_ICON},
-    {"web_store/webstore_icon_16.png", IDR_WEBSTORE_ICON_16},
-    {"chrome_app/product_logo_128.png", IDR_PRODUCT_LOGO_128},
-    {"chrome_app/product_logo_16.png", IDR_PRODUCT_LOGO_16},
-    {"settings_app/settings_app_icon_128.png", IDR_SETTINGS_APP_ICON_128},
-    {"settings_app/settings_app_icon_16.png", IDR_SETTINGS_APP_ICON_16},
-  };
-  static const size_t kExtraComponentExtensionResourcesSize =
-      arraysize(kExtraComponentExtensionResources);
-
-  if (extension->location() != Extension::COMPONENT)
-    return false;
-
-  FilePath directory_path = extension->path();
-  FilePath resources_dir;
-  FilePath relative_path;
-  if (!PathService::Get(chrome::DIR_RESOURCES, &resources_dir) ||
-      !resources_dir.AppendRelativePath(directory_path, &relative_path)) {
-    return false;
-  }
-  relative_path = relative_path.Append(resource_path);
-  relative_path = relative_path.NormalizePathSeparators();
-
-  // TODO(tc): Make a map of FilePath -> resource ids so we don't have to
-  // covert to FilePaths all the time.  This will be more useful as we add
-  // more resources.
-  for (size_t i = 0; i < kComponentExtensionResourcesSize; ++i) {
-    FilePath resource_path =
-        FilePath().AppendASCII(kComponentExtensionResources[i].name);
-    resource_path = resource_path.NormalizePathSeparators();
-
-    if (relative_path == resource_path) {
-      *resource_id = kComponentExtensionResources[i].value;
-      return true;
-    }
-  }
-  for (size_t i = 0; i < kExtraComponentExtensionResourcesSize; ++i) {
-    FilePath resource_path =
-        FilePath().AppendASCII(kExtraComponentExtensionResources[i].name);
-    resource_path = resource_path.NormalizePathSeparators();
-
-    if (relative_path == resource_path) {
-      *resource_id = kExtraComponentExtensionResources[i].value;
-      return true;
-    }
-  }
-  return false;
-}
-
 void ImageLoadingTracker::OnBitmapLoaded(
     const SkBitmap* bitmap,
     const ImageRepresentation& image_info,

[commit-bot: I haz the power, 2012-11-15 20:09:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/11027044/43048

[commit-bot: I haz the power, 2012-11-15 20:09:50]

Presubmit check for 11027044-43048 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      chrome/browser/extensions/image_loader.cc:17

Presubmit checks took 3.8s to calculate.

[commit-bot: I haz the power, 2012-11-15 20:43:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/11027044/43049

[commit-bot: I haz the power, 2012-11-16 04:25:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/11027044/43049

[commit-bot: I haz the power, 2012-11-16 07:40:50]

Change committed as 168155

[Aaron Boodman, 2012-11-20 21:46:13]

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
File chrome/browser/extensions/image_loader.cc (right):

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
chrome/browser/extensions/image_loader.cc:251:
base::Bind(&ImageLoader::LoadImagesOnBlockingPool, base::Unretained(this),
What prevents this callback from happening after the Profile is destroyed?
Shouldn't these be weak pointers?

[Marijn Kruisselbrink, 2012-11-20 21:51:03]

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
File chrome/browser/extensions/image_loader.cc (right):

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
chrome/browser/extensions/image_loader.cc:251:
base::Bind(&ImageLoader::LoadImagesOnBlockingPool, base::Unretained(this),
On 2012/11/20 21:46:14, Aaron Boodman wrote:
> What prevents this callback from happening after the Profile is destroyed?
> Shouldn't these be weak pointers?
I don't think weak pointers would work for cross-thread tasks, but yes, probably
something should be done. Changing ImageLoader to be a
RefcountedProfileKeyedService might work?

[Aaron Boodman, 2012-11-21 00:57:21]

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
File chrome/browser/extensions/image_loader.cc (right):

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
chrome/browser/extensions/image_loader.cc:251:
base::Bind(&ImageLoader::LoadImagesOnBlockingPool, base::Unretained(this),
On 2012/11/20 21:51:03, Marijn Kruisselbrink wrote:
> On 2012/11/20 21:46:14, Aaron Boodman wrote:
> > What prevents this callback from happening after the Profile is destroyed?
> > Shouldn't these be weak pointers?
> I don't think weak pointers would work for cross-thread tasks, but yes,
probably
> something should be done. Changing ImageLoader to be a
> RefcountedProfileKeyedService might work?

You can pass weakptrs to other threads, you just can't deref them there. So
you'd use a weakptr to ensure the reply never happens if the ImageLoader is
destroyed.

You can use PostTaskAndReply I believe, and make the reply closure weak.

If that doesn't work for some reason, just passing a reply closure through to
the file thread, where that closure uses a weakptr should work.

[Marijn Kruisselbrink, 2012-11-21 01:17:55]

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
File chrome/browser/extensions/image_loader.cc (right):

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
chrome/browser/extensions/image_loader.cc:251:
base::Bind(&ImageLoader::LoadImagesOnBlockingPool, base::Unretained(this),
On 2012/11/21 00:57:21, Aaron Boodman wrote:
> On 2012/11/20 21:51:03, Marijn Kruisselbrink wrote:
> > On 2012/11/20 21:46:14, Aaron Boodman wrote:
> > > What prevents this callback from happening after the Profile is destroyed?
> > > Shouldn't these be weak pointers?
> > I don't think weak pointers would work for cross-thread tasks, but yes,
> probably
> > something should be done. Changing ImageLoader to be a
> > RefcountedProfileKeyedService might work?
> 
> You can pass weakptrs to other threads, you just can't deref them there. So
> you'd use a weakptr to ensure the reply never happens if the ImageLoader is
> destroyed.
> 
> You can use PostTaskAndReply I believe, and make the reply closure weak.
> 
> If that doesn't work for some reason, just passing a reply closure through to
> the file thread, where that closure uses a weakptr should work.
Ah yes, you're right (of course). If I make LoadImagesOnBlockingPool a
non-member I can easily use a weakptr to prevent the ReplyBack part from being
executed after ImageLoader is destroyed. I'll do that tomorrow.

[kaiwang, 2012-11-29 03:57:37]

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
File chrome/browser/extensions/image_loader.h (right):

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
chrome/browser/extensions/image_loader.h:83: // if the image was found in the
cache.
Looking at the code, I think the callback is always run asynchronously?

[Marijn Kruisselbrink, 2012-11-29 15:51:39]

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
File chrome/browser/extensions/image_loader.h (right):

https://chromiumcodereview.appspot.com/11027044/diff/43049/chrome/browser/ext...
chrome/browser/extensions/image_loader.h:83: // if the image was found in the
cache.
On 2012/11/29 03:57:37, kaiwang wrote:
> Looking at the code, I think the callback is always run asynchronously?
Yeah, that's true; at the moment the callback is always run asynchronously. If
caching gets re-added in the future we'll at least keep the option of calling
the callback synchronously though. So I don't see a compelling reason to remove
this restriction evn though currently it is not being used.