Files.app: Stop to use drive thumbnail for cached files.

Files.app: Stop to use drive thumbnail for cached files.

Previously ThumbnailLoader refers both metadata.thumbnail.url and
meatadata.external.thumbnailUrl. Thus it loads external thumbnail even if the
file is cached locally.

The CL fixes the logic and stop to use drive thumbnail for image files that are
cached. Note that if the file is not an image, use drive thumbnail always
because Files.app does not have logic to generate thumbnail from non-image
files.

BUG=446827
TEST=Added

Committed: https://crrev.com/d102ac3f7aa2d7759f1226ef65744d589bf4f676
Cr-Commit-Position: refs/heads/master@{#310729}

[hirono, 2015-01-08 10:59:15]

hirono@chromium.org changed reviewers:
+ mtomasz@chromium.org

[hirono, 2015-01-08 10:59:15]

@mtomasz - PTAL the CL?
@fukino, @yawano - I added you to CC. If you found the problem around metadata
logic, please let me know.

Thanks!

[yawano, 2015-01-09 00:50:53]

On 2015/01/08 10:59:15, hirono wrote:
> @mtomasz - PTAL the CL?
> @fukino, @yawano - I added you to CC. If you found the problem around metadata
> logic, please let me know.
> 
> Thanks!

@hirono: SGTM. Thank you.

[mtomasz, 2015-01-09 01:20:40]

lgtm with one question. Doesn't it affect performance? If jpeg files do not have
thumbnails, then generating them for mosaic view will become very heavy.

https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right):

https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:122: * Returns
whether the metadata have the thumbnail information or not.
nit: Please update the comment.

https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
(right):

https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js:5: 
nit: Redundant \n

[hirono, 2015-01-09 01:38:22]

On 2015/01/09 01:20:40, mtomasz wrote:
> lgtm with one question. Doesn't it affect performance? If jpeg files do not
have
> thumbnails, then generating them for mosaic view will become very heavy.
> 
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right):
> 
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:122: * Returns
> whether the metadata have the thumbnail information or not.
> nit: Please update the comment.
> 
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> File ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
> (right):
> 
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js:5: 
> nit: Redundant \n

There is a trade off of "the latest thumbnail" vs "performance".
In Gallery, we should use the latest thumbnail because users have more chances
to update images in the app.
In Files.app, I'm thinking following options. WDYT?

a. Pass flag to allow load thumbnail from the drive even if the file is cached.
b. Add assumption the drive thumbnails are updated in N minutes after the file
is updated. It means we can use drive thumbnail if the last modified time of the
file is earlier than N minutes ago. This is not a complete logic if the device
is offline because we don't record "last synced time". e.g. wrong thumbnail can
be used for the situation: the image is updated (offline) -> N minutes passed ->
the image is synced.
c. Do nothing. Anyway we generates thumbnails from files for local volumes (e.g.
SD cards). It is just as slow as local volumes.

[mtomasz, 2015-01-09 02:07:16]

On 2015/01/09 01:38:22, hirono wrote:
> On 2015/01/09 01:20:40, mtomasz wrote:
> > lgtm with one question. Doesn't it affect performance? If jpeg files do not
> have
> > thumbnails, then generating them for mosaic view will become very heavy.
> > 
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right):
> > 
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:122: *
Returns
> > whether the metadata have the thumbnail information or not.
> > nit: Please update the comment.
> > 
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > File ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js:5: 
> > nit: Redundant \n
> 
> There is a trade off of "the latest thumbnail" vs "performance".
> In Gallery, we should use the latest thumbnail because users have more chances
> to update images in the app.
> In Files.app, I'm thinking following options. WDYT?
> 
> a. Pass flag to allow load thumbnail from the drive even if the file is
cached.
> b. Add assumption the drive thumbnails are updated in N minutes after the file
> is updated. It means we can use drive thumbnail if the last modified time of
the
> file is earlier than N minutes ago. This is not a complete logic if the device
> is offline because we don't record "last synced time". e.g. wrong thumbnail
can
> be used for the situation: the image is updated (offline) -> N minutes passed
->
> the image is synced.
> c. Do nothing. Anyway we generates thumbnails from files for local volumes
(e.g.
> SD cards). It is just as slow as local volumes.

Got it. Let's go with (c) for now as in your CL. Note, that jpeg decoding is
going to be much faster soon (@posciak is working on it).

[mtomasz, 2015-01-09 02:07:25]

On 2015/01/09 02:07:16, mtomasz wrote:
> On 2015/01/09 01:38:22, hirono wrote:
> > On 2015/01/09 01:20:40, mtomasz wrote:
> > > lgtm with one question. Doesn't it affect performance? If jpeg files do
not
> > have
> > > thumbnails, then generating them for mosaic view will become very heavy.
> > > 
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:122: *
> Returns
> > > whether the metadata have the thumbnail information or not.
> > > nit: Please update the comment.
> > > 
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > File
ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js:5:

> > > nit: Redundant \n
> > 
> > There is a trade off of "the latest thumbnail" vs "performance".
> > In Gallery, we should use the latest thumbnail because users have more
chances
> > to update images in the app.
> > In Files.app, I'm thinking following options. WDYT?
> > 
> > a. Pass flag to allow load thumbnail from the drive even if the file is
> cached.
> > b. Add assumption the drive thumbnails are updated in N minutes after the
file
> > is updated. It means we can use drive thumbnail if the last modified time of
> the
> > file is earlier than N minutes ago. This is not a complete logic if the
device
> > is offline because we don't record "last synced time". e.g. wrong thumbnail
> can
> > be used for the situation: the image is updated (offline) -> N minutes
passed
> ->
> > the image is synced.
> > c. Do nothing. Anyway we generates thumbnails from files for local volumes
> (e.g.
> > SD cards). It is just as slow as local volumes.
> 
> Got it. Let's go with (c) for now as in your CL. Note, that jpeg decoding is
> going to be much faster soon (@posciak is working on it).

lgtm

[mtomasz, 2015-01-09 02:09:30]

On 2015/01/09 02:07:25, mtomasz wrote:
> On 2015/01/09 02:07:16, mtomasz wrote:
> > On 2015/01/09 01:38:22, hirono wrote:
> > > On 2015/01/09 01:20:40, mtomasz wrote:
> > > > lgtm with one question. Doesn't it affect performance? If jpeg files do
> not
> > > have
> > > > thumbnails, then generating them for mosaic view will become very heavy.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:122: *
> > Returns
> > > > whether the metadata have the thumbnail information or not.
> > > > nit: Please update the comment.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > File
> ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > >
ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js:5:
> 
> > > > nit: Redundant \n
> > > 
> > > There is a trade off of "the latest thumbnail" vs "performance".
> > > In Gallery, we should use the latest thumbnail because users have more
> chances
> > > to update images in the app.
> > > In Files.app, I'm thinking following options. WDYT?
> > > 
> > > a. Pass flag to allow load thumbnail from the drive even if the file is
> > cached.
> > > b. Add assumption the drive thumbnails are updated in N minutes after the
> file
> > > is updated. It means we can use drive thumbnail if the last modified time
of
> > the
> > > file is earlier than N minutes ago. This is not a complete logic if the
> device
> > > is offline because we don't record "last synced time". e.g. wrong
thumbnail
> > can
> > > be used for the situation: the image is updated (offline) -> N minutes
> passed
> > ->
> > > the image is synced.
> > > c. Do nothing. Anyway we generates thumbnails from files for local volumes
> > (e.g.
> > > SD cards). It is just as slow as local volumes.
> > 
> > Got it. Let's go with (c) for now as in your CL. Note, that jpeg decoding is
> > going to be much faster soon (@posciak is working on it).
> 
> lgtm

One thing. Could you check all users of USE_EMBEDDED, especially
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/ga...
if they are still correct after this patch and the comments there are still
correct? Thanks.

[fukino, 2015-01-09 02:27:55]

On 2015/01/09 02:07:16, mtomasz wrote:
> On 2015/01/09 01:38:22, hirono wrote:
> > On 2015/01/09 01:20:40, mtomasz wrote:
> > > lgtm with one question. Doesn't it affect performance? If jpeg files do
not
> > have
> > > thumbnails, then generating them for mosaic view will become very heavy.
> > > 
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:122: *
> Returns
> > > whether the metadata have the thumbnail information or not.
> > > nit: Please update the comment.
> > > 
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > File
ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js:5:

> > > nit: Redundant \n
> > 
> > There is a trade off of "the latest thumbnail" vs "performance".
> > In Gallery, we should use the latest thumbnail because users have more
chances
> > to update images in the app.
> > In Files.app, I'm thinking following options. WDYT?
> > 
> > a. Pass flag to allow load thumbnail from the drive even if the file is
> cached.
> > b. Add assumption the drive thumbnails are updated in N minutes after the
file
> > is updated. It means we can use drive thumbnail if the last modified time of
> the
> > file is earlier than N minutes ago. This is not a complete logic if the
device
> > is offline because we don't record "last synced time". e.g. wrong thumbnail
> can
> > be used for the situation: the image is updated (offline) -> N minutes
passed
> ->
> > the image is synced.
> > c. Do nothing. Anyway we generates thumbnails from files for local volumes
> (e.g.
> > SD cards). It is just as slow as local volumes.
> 
> Got it. Let's go with (c) for now as in your CL. Note, that jpeg decoding is
> going to be much faster soon (@posciak is working on it).

I agree with (c).
Let's go with simpler solution for now. Thanks!

[hirono, 2015-01-09 02:30:06]

On 2015/01/09 02:27:55, fukino wrote:
> On 2015/01/09 02:07:16, mtomasz wrote:
> > On 2015/01/09 01:38:22, hirono wrote:
> > > On 2015/01/09 01:20:40, mtomasz wrote:
> > > > lgtm with one question. Doesn't it affect performance? If jpeg files do
> not
> > > have
> > > > thumbnails, then generating them for mosaic view will become very heavy.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:122: *
> > Returns
> > > > whether the metadata have the thumbnail information or not.
> > > > nit: Please update the comment.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > File
> ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > >
ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js:5:
> 
> > > > nit: Redundant \n
> > > 
> > > There is a trade off of "the latest thumbnail" vs "performance".
> > > In Gallery, we should use the latest thumbnail because users have more
> chances
> > > to update images in the app.
> > > In Files.app, I'm thinking following options. WDYT?
> > > 
> > > a. Pass flag to allow load thumbnail from the drive even if the file is
> > cached.
> > > b. Add assumption the drive thumbnails are updated in N minutes after the
> file
> > > is updated. It means we can use drive thumbnail if the last modified time
of
> > the
> > > file is earlier than N minutes ago. This is not a complete logic if the
> device
> > > is offline because we don't record "last synced time". e.g. wrong
thumbnail
> > can
> > > be used for the situation: the image is updated (offline) -> N minutes
> passed
> > ->
> > > the image is synced.
> > > c. Do nothing. Anyway we generates thumbnails from files for local volumes
> > (e.g.
> > > SD cards). It is just as slow as local volumes.
> > 
> > Got it. Let's go with (c) for now as in your CL. Note, that jpeg decoding is
> > going to be much faster soon (@posciak is working on it).
> 
> I agree with (c).
> Let's go with simpler solution for now. Thanks!

I just found the cloud import team introduced is_dirty flag to the metadata. So
we got the new option.

d. Don't use drive thumbnail if the file is dirty (not synced).

Let me try it.

[mtomasz, 2015-01-09 02:32:27]

On 2015/01/09 02:30:06, hirono wrote:
> On 2015/01/09 02:27:55, fukino wrote:
> > On 2015/01/09 02:07:16, mtomasz wrote:
> > > On 2015/01/09 01:38:22, hirono wrote:
> > > > On 2015/01/09 01:20:40, mtomasz wrote:
> > > > > lgtm with one question. Doesn't it affect performance? If jpeg files
do
> > not
> > > > have
> > > > > thumbnails, then generating them for mosaic view will become very
heavy.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > > File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js
> > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > > ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:122: *
> > > Returns
> > > > > whether the metadata have the thumbnail information or not.
> > > > > nit: Please update the comment.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > > File
> > ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
> > > > >
> ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js:5:
> > 
> > > > > nit: Redundant \n
> > > > 
> > > > There is a trade off of "the latest thumbnail" vs "performance".
> > > > In Gallery, we should use the latest thumbnail because users have more
> > chances
> > > > to update images in the app.
> > > > In Files.app, I'm thinking following options. WDYT?
> > > > 
> > > > a. Pass flag to allow load thumbnail from the drive even if the file is
> > > cached.
> > > > b. Add assumption the drive thumbnails are updated in N minutes after
the
> > file
> > > > is updated. It means we can use drive thumbnail if the last modified
time
> of
> > > the
> > > > file is earlier than N minutes ago. This is not a complete logic if the
> > device
> > > > is offline because we don't record "last synced time". e.g. wrong
> thumbnail
> > > can
> > > > be used for the situation: the image is updated (offline) -> N minutes
> > passed
> > > ->
> > > > the image is synced.
> > > > c. Do nothing. Anyway we generates thumbnails from files for local
volumes
> > > (e.g.
> > > > SD cards). It is just as slow as local volumes.
> > > 
> > > Got it. Let's go with (c) for now as in your CL. Note, that jpeg decoding
is
> > > going to be much faster soon (@posciak is working on it).
> > 
> > I agree with (c).
> > Let's go with simpler solution for now. Thanks!
> 
> I just found the cloud import team introduced is_dirty flag to the metadata.
So
> we got the new option.
> 
> d. Don't use drive thumbnail if the file is dirty (not synced).
> 
> Let me try it.

I'm wondering how fast are styx thumbnails updated. If it's instant, then it's
perfect, but if it's with a delay, then we may end up on getting an old
thumbnail. WDYT?

[hirono, 2015-01-09 05:50:49]

@mtomasz - I removed USE_EMBEDDED enum and introduced new type to show exact
load target. Thank you for catching this.
And it seems styx thumbnails are updated immediately. So I started to use dirty
flag to determine the load target.

Thanks!

https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right):

https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:122: * Returns
whether the metadata have the thumbnail information or not.
On 2015/01/09 01:20:40, mtomasz wrote:
> nit: Please update the comment.

Done.

https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
(right):

https://codereview.chromium.org/831833004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js:5: 
On 2015/01/09 01:20:40, mtomasz wrote:
> nit: Redundant \n

Done.

[mtomasz, 2015-01-09 05:58:50]

Nice cleanup! LGTM with nits.

https://codereview.chromium.org/831833004/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/831833004/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:243: // Drive
provides high quality thumbnails via USE_EMBEDDED, however local
nit: Please update the comment.

https://codereview.chromium.org/831833004/diff/40001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/mosaic_mode.js (right):

https://codereview.chromium.org/831833004/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/mosaic_mode.js:2063: if
(this.thumbnailLoader_.getLoadTarget() ==
nit: ===

[hirono, 2015-01-09 07:11:47]

Thanks!

https://codereview.chromium.org/831833004/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/831833004/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:243: // Drive
provides high quality thumbnails via USE_EMBEDDED, however local
On 2015/01/09 05:58:50, mtomasz wrote:
> nit: Please update the comment.

Done.

https://codereview.chromium.org/831833004/diff/40001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/mosaic_mode.js (right):

https://codereview.chromium.org/831833004/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/mosaic_mode.js:2063: if
(this.thumbnailLoader_.getLoadTarget() ==
On 2015/01/09 05:58:50, mtomasz wrote:
> nit: ===

Done.

[hirono, 2015-01-09 07:11:51]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2015-01-09 07:12:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/831833004/60001

[commit-bot: I haz the power, 2015-01-09 07:51:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-09 07:52:04]

Patchset 4 (id:??) landed as
https://crrev.com/d102ac3f7aa2d7759f1226ef65744d589bf4f676
Cr-Commit-Position: refs/heads/master@{#310729}