Gallery: Store image caches in Gallery items.

Previously the caches are stored in the UI class.
The CL moves the caches in Gallery items to:

 * Simplify UI class.
 * Share the cache different UI classes.
 * Move loading logic into the items in future patches.

BUG=391643
TEST=manually

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285890

[hirono, 2014-07-25 09:19:03]

Previously the caches are stored in the UI class.
The CL moves the caches in Gallery items to:

 * Simplify UI class.
 * Share the cache different UI classes.
 * Move loading logic into the items in future patches.

[hirono, 2014-07-25 09:19:46]

PTAL. Thanks!

[mtomasz, 2014-07-26 05:14:59]

On 2014/07/25 09:19:46, hirono wrote:
> PTAL. Thanks!

Do we need this cache? Image loader already caches. What do you think?

[hirono, 2014-07-28 04:50:58]

On 2014/07/26 05:14:59, mtomasz wrote:
> On 2014/07/25 09:19:46, hirono wrote:
> > PTAL. Thanks!
> 
> Do we need this cache? Image loader already caches. What do you think?

The slide images are not loaded by ImageLoader currently.
I did quick observation, and found that loading the slide images by using
ImageLoader takes more time.

https://docs.google.com/a/chromium.org/spreadsheets/d/12MIvHiZpPuqtScqMwPv0p5...

direct: Load directly
external: Load by using the external extension (ImageLoader)

[mtomasz, 2014-07-28 05:05:42]

On 2014/07/28 04:50:58, hirono wrote:
> On 2014/07/26 05:14:59, mtomasz wrote:
> > On 2014/07/25 09:19:46, hirono wrote:
> > > PTAL. Thanks!
> > 
> > Do we need this cache? Image loader already caches. What do you think?
> 
> The slide images are not loaded by ImageLoader currently.
> I did quick observation, and found that loading the slide images by using
> ImageLoader takes more time.
> 
>
https://docs.google.com/a/chromium.org/spreadsheets/d/12MIvHiZpPuqtScqMwPv0p5...
> 
> direct: Load directly
> external: Load by using the external extension (ImageLoader)

Thanks for the investigation. Image loader looks very bad on the comparison. Did
you perform any resizing? JPEG decoding + encoding is slow on Chrome OS.

[mtomasz, 2014-07-28 05:13:02]

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/gallery.js (right):

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery.js:151: var contentCacheCount = 2;
I think the code is too complicated. contentCacheCount becomes a negative number
in #155. What does it mean if count is negative? Can we simplify?

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery.js:159: sorted[i].contentImage.width = 0;
Please add a comment, that this is needed to force GC.

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/gallery_item.js (right):

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery_item.js:55: this.lastAccessed = Date.now();
This seems never updated? Is it OK?

[hirono, 2014-07-28 06:50:19]

> Did you perform any resizing? JPEG decoding + encoding is slow on Chrome OS.

No, this is time for loading full size images. I think we can do either to
improve the FPS of the CSS transition animation.

 * Move the resizing part into Image Loader, or
 * Split images into stripes, and then resize the stripes

[hirono, 2014-07-28 06:50:32]

Thanks!

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/gallery.js (right):

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery.js:151: var contentCacheCount = 2;
On 2014/07/28 05:13:01, mtomasz wrote:
> I think the code is too complicated. contentCacheCount becomes a negative
number
> in #155. What does it mean if count is negative? Can we simplify?

Done.

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery.js:159: sorted[i].contentImage.width = 0;
On 2014/07/28 05:13:01, mtomasz wrote:
> Please add a comment, that this is needed to force GC.

Done.

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/gallery_item.js (right):

https://codereview.chromium.org/420743002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery_item.js:55: this.lastAccessed = Date.now();
On 2014/07/28 05:13:01, mtomasz wrote:
> This seems never updated? Is it OK?

The property is updated in the evictCache function.

[mtomasz, 2014-07-28 07:51:15]

On 2014/07/28 06:50:19, hirono wrote:
> > Did you perform any resizing? JPEG decoding + encoding is slow on Chrome OS.
> 
> No, this is time for loading full size images. I think we can do either to
> improve the FPS of the CSS transition animation.
> 
>  * Move the resizing part into Image Loader, or
>  * Split images into stripes, and then resize the stripes

Hm, resizing in image loader for the screen size resolution should work. If only
jpeg encoding was causing that image loader's slowness, then it should be fine
(since the output after resizing will be small).

[mtomasz, 2014-07-28 07:51:21]

https://codereview.chromium.org/420743002/diff/80001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/gallery.js (right):

https://codereview.chromium.org/420743002/diff/80001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery.js:159: currentSelectedItem.lastAccessed =
Date.now();
I think updating lastAccessed time shouldn't be responsibility of evictCache(),
which should just evict cache.

How about adding a getter and setter for contentImage and screenImage, and
updating accessedTime there + calling evictCache in the setter? WDYT?

[hirono, 2014-07-28 09:10:33]

https://codereview.chromium.org/420743002/diff/80001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/gallery.js (right):

https://codereview.chromium.org/420743002/diff/80001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery.js:159: currentSelectedItem.lastAccessed =
Date.now();
On 2014/07/28 07:51:21, mtomasz wrote:
> I think updating lastAccessed time shouldn't be responsibility of
evictCache(),
> which should just evict cache.
> 
> How about adding a getter and setter for contentImage and screenImage, and
> updating accessedTime there + calling evictCache in the setter? WDYT?

How about adding touch method and calling it when the selection is changed?
Getters should work. But It may change the priority of cache unintentionally. I
would rather make it controllable a bit more.

[mtomasz, 2014-07-28 09:14:00]

https://codereview.chromium.org/420743002/diff/80001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/gallery.js (right):

https://codereview.chromium.org/420743002/diff/80001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery.js:159: currentSelectedItem.lastAccessed =
Date.now();
On 2014/07/28 09:10:33, hirono wrote:
> On 2014/07/28 07:51:21, mtomasz wrote:
> > I think updating lastAccessed time shouldn't be responsibility of
> evictCache(),
> > which should just evict cache.
> > 
> > How about adding a getter and setter for contentImage and screenImage, and
> > updating accessedTime there + calling evictCache in the setter? WDYT?
> 
> How about adding touch method and calling it when the selection is changed?
> Getters should work. But It may change the priority of cache unintentionally.
I
> would rather make it controllable a bit more.

SGTM!

[hirono, 2014-07-28 09:27:21]

https://codereview.chromium.org/420743002/diff/80001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/gallery.js (right):

https://codereview.chromium.org/420743002/diff/80001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery.js:159: currentSelectedItem.lastAccessed =
Date.now();
On 2014/07/28 09:14:00, mtomasz wrote:
> On 2014/07/28 09:10:33, hirono wrote:
> > On 2014/07/28 07:51:21, mtomasz wrote:
> > > I think updating lastAccessed time shouldn't be responsibility of
> > evictCache(),
> > > which should just evict cache.
> > > 
> > > How about adding a getter and setter for contentImage and screenImage, and
> > > updating accessedTime there + calling evictCache in the setter? WDYT?
> > 
> > How about adding touch method and calling it when the selection is changed?
> > Getters should work. But It may change the priority of cache
unintentionally.
> I
> > would rather make it controllable a bit more.
> 
> SGTM!

Thanks! Done.

[mtomasz, 2014-07-28 10:07:43]

lgtm with a nit.

https://codereview.chromium.org/420743002/diff/100001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/gallery_item.js (right):

https://codereview.chromium.org/420743002/diff/100001/ui/file_manager/gallery...
ui/file_manager/gallery/js/gallery_item.js:129: return this.lastAccessed_ =
Date.now();
nit: Do we need to return?

[hirono, 2014-07-28 10:31:30]

Thank you!

https://codereview.chromium.org/420743002/diff/100001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/gallery_item.js (right):

https://codereview.chromium.org/420743002/diff/100001/ui/file_manager/gallery...
ui/file_manager/gallery/js/gallery_item.js:129: return this.lastAccessed_ =
Date.now();
On 2014/07/28 10:07:43, mtomasz wrote:
> nit: Do we need to return?

Done.

[hirono, 2014-07-28 10:31:33]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-07-28 10:32:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hirono@chromium.org/420743002/120001

[commit-bot: I haz the power, 2014-07-28 11:44:07]

Change committed as 285890