Files.app: Show thumbnail of non-image file even when the file cache is present

Files.app: Show thumbnail of non-image file even when the file cache is present

Previously, thumbnails of offline-available non-image files were not shown. This was because we tried to show self-generated thumbnails when the file cache was present. But we can't generate thumbnails of non-image so the thumbnails of video files were not shown.

With this patch, if thumbnail can't be generated, the thumbnail from the external provider shows instead.

BUG=415048
TEST=manual tested

Committed: https://crrev.com/19d31bfb5de5acba66036f5acb2a5a140d1d378d
Cr-Commit-Position: refs/heads/master@{#295893}

[yoshiki, 2014-09-17 09:07:23]

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

[yoshiki, 2014-09-17 09:08:41]

@mtomasz: PTAL. Thanks.

[yoshiki, 2014-09-17 09:11:15]

Updated the title.

[mtomasz, 2014-09-17 09:29:34]

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

[mtomasz, 2014-09-17 09:29:48]

+hirono@ for the stale thumbnail related code.

https://codereview.chromium.org/574293002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc
(right):

https://codereview.chromium.org/574293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:80:
properties->thumbnail_url.reset(
AFAIR, the check for is_present() was done by @hirono in order to overcome an
issue with stale thumbnails when the file is modified locally but not synced
yet. I'm worried that removing it will reintroduce the regression which was
recently fixed.

+@hirono to confirm.

https://codereview.chromium.org/574293002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js
(right):

https://codereview.chromium.org/574293002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js:877:
thumbnailUrl: data.thumbnailUrl  // Thumbnail passed from external server.
ExternalProvider used to provide a thumbnail field if the file is not cached.
See #804. However, after this change it's not. Calling
metadataCache.get([entry], 'thumbnail') would skip the ExternalProvider and
cause downloading the entire file to get a thumbnail from file if it's not
cached, what may kill performance of the grid view and mosaic view.

https://codereview.chromium.org/574293002/diff/40001/ui/file_manager/video_pl...
File ui/file_manager/video_player/js/video_player.js (right):

https://codereview.chromium.org/574293002/diff/40001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/video_player.js:325: 'url(' + url +
'&access_token=' + token + ')';
Can we append the access token in C++? I'm working on moving Drive specific
logic from JS to C++, since thumbnailUrl is now available for provided file
systems too.

[yoshiki, 2014-09-17 09:57:10]

https://codereview.chromium.org/574293002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc
(right):

https://codereview.chromium.org/574293002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:80:
properties->thumbnail_url.reset(
On 2014/09/17 09:29:48, mtomasz wrote:
> AFAIR, the check for is_present() was done by @hirono in order to overcome an
> issue with stale thumbnails when the file is modified locally but not synced
> yet. I'm worried that removing it will reintroduce the regression which was
> recently fixed.
> 
> +@hirono to confirm.

The generated thumbnail has higher priority and the generated one is always
shown when available. so the problem should not be occurred, I think.

https://codereview.chromium.org/574293002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js
(right):

https://codereview.chromium.org/574293002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js:877:
thumbnailUrl: data.thumbnailUrl  // Thumbnail passed from external server.
On 2014/09/17 09:29:48, mtomasz wrote:
> ExternalProvider used to provide a thumbnail field if the file is not cached.
> See #804. However, after this change it's not. Calling
> metadataCache.get([entry], 'thumbnail') would skip the ExternalProvider and
> cause downloading the entire file to get a thumbnail from file if it's not
> cached, what may kill performance of the grid view and mosaic view.

I got it. I add the code to fill thumbnail field when the file is present below.

https://codereview.chromium.org/574293002/diff/40001/ui/file_manager/video_pl...
File ui/file_manager/video_player/js/video_player.js (right):

https://codereview.chromium.org/574293002/diff/40001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/video_player.js:325: 'url(' + url +
'&access_token=' + token + ')';
On 2014/09/17 09:29:48, mtomasz wrote:
> Can we append the access token in C++? I'm working on moving Drive specific
> logic from JS to C++, since thumbnailUrl is now available for provided file
> systems too.

I'll do it. But please leave it as it is at this time. This code is run only for
drive files and the problem shouldn't be occurred.

[hirono, 2014-09-18 03:22:18]

https://codereview.chromium.org/574293002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js
(right):

https://codereview.chromium.org/574293002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js:887: //
generate it by next providers.
How about PDF files?
ContentProvider can generate only thumbnails of images.

[hirono, 2014-09-18 03:26:31]

Sorry I missed the change in thumbnail loader.
lgtm!

[mtomasz, 2014-09-18 03:44:10]

lgtm

https://codereview.chromium.org/574293002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js
(right):

https://codereview.chromium.org/574293002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js:889: }
else if ('thumbnailUrl' in data) {
This logic became very complicated. We are now returning thumbnails in two
different places. To show a thumbnail we always have to check both fields.

I'm fine with this change, but we should rewrite the entire metadata cache soon.
It became a big ball of mud.

[yoshiki, 2014-09-18 04:18:34]

On 2014/09/18 03:44:10, mtomasz wrote:
> lgtm
> 
>
https://codereview.chromium.org/574293002/diff/60001/ui/file_manager/file_man...
> File ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js
> (right):
> 
>
https://codereview.chromium.org/574293002/diff/60001/ui/file_manager/file_man...
> ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js:889: }
> else if ('thumbnailUrl' in data) {
> This logic became very complicated. We are now returning thumbnails in two
> different places. To show a thumbnail we always have to check both fields.

I know the complex point. But the problem is that the 2 providers return
different metadata. I think we need to introduce a concept of "priority".

> I'm fine with this change, but we should rewrite the entire metadata cache
soon.
> It became a big ball of mud.

Got it. I'll commit this patch (with adding some comment) just for now temporary
solution.

[yoshiki, 2014-09-18 04:23:38]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-09-18 04:24:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/574293002/80001

[commit-bot: I haz the power, 2014-09-18 04:27:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-18 04:27:43]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/57527)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[yoshiki, 2014-09-18 11:28:53]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-09-18 11:29:41]

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

[commit-bot: I haz the power, 2014-09-18 11:32:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-18 11:32:49]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/57633)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/62436)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[yoshiki, 2014-09-20 21:00:09]

Patchset #6 (id:100001) has been deleted

[yoshiki, 2014-09-20 21:00:37]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-09-20 21:01:26]

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

[commit-bot: I haz the power, 2014-09-20 21:55:54]

Committed patchset #6 (id:120001) as 62bad196290cac8341748ee49ba085d1dff523f4

[commit-bot: I haz the power, 2014-09-20 21:56:29]

Patchset 6 (id:??) landed as
https://crrev.com/19d31bfb5de5acba66036f5acb2a5a140d1d378d
Cr-Commit-Position: refs/heads/master@{#295893}