[fsp] Add support for providing thumbnails.

[fsp] Add support for providing thumbnails.

Previously, thumbnails were generated from full size images, which in case of
cloud providers would invoke downloading the entire file.

This CL adds an ability to provide a thumbnail as a data URI, so:
(1) We can have thumbnails for other file types than images.
(2) Thumbnails are shown fast.
(3) Resource consumption is significantly reduced.

Note, that the thumbnails are not wired to Files app yet. That will be done in
a separate patch.

TEST=unit_tests: *FileSystemProvider*GetMetadata*
BUG=407954
Committed: https://crrev.com/47501847827d29b4ba148a161a521eb3cd5b0730
Cr-Commit-Position: refs/heads/master@{#292840}

[mtomasz, 2014-08-27 08:40:56]

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

[mtomasz, 2014-08-27 08:41:04]

@hirono: PTAL. Thanks.

[hirono, 2014-08-27 09:20:08]

https://codereview.chromium.org/513683002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/operations/get_metadata.cc
(right):

https://codereview.chromium.org/513683002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/get_metadata.cc:36: if
(!params->metadata.modification_time.additional_properties.GetString(
Should update thumbnail here?

https://codereview.chromium.org/513683002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h
(right):

https://codereview.chromium.org/513683002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h:43:
std::string thumbnail;
Is the structure designed to be copied multiple times? (e.g.
base::Binding(callback, this, metadata))
If so, the thumbnail should be linked_ptr?

https://codereview.chromium.org/513683002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/513683002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/file_system_provider.idl:54: // Thumbnail image as
a data URI. Optional, but can be provided only when
How about describing about image types of the thumbnail here?

https://codereview.chromium.org/513683002/diff/20001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/513683002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:149:
if (!options.thumbnail && metadata.thumbnail) {
How about checking format of thumbnail?
The string may not start with 'data:' and valid image types.

[mtomasz, 2014-08-28 04:57:55]

https://codereview.chromium.org/513683002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/operations/get_metadata.cc
(right):

https://codereview.chromium.org/513683002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/get_metadata.cc:36: if
(!params->metadata.modification_time.additional_properties.GetString(
On 2014/08/27 09:20:07, hirono wrote:
> Should update thumbnail here?

Oops. Fortunately the tests would catch it.

https://codereview.chromium.org/513683002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h
(right):

https://codereview.chromium.org/513683002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h:43:
std::string thumbnail;
On 2014/08/27 09:20:07, hirono wrote:
> Is the structure designed to be copied multiple times? (e.g.
> base::Binding(callback, this, metadata))
> If so, the thumbnail should be linked_ptr?

How about passing EntryMetadata as a scoped_ptr? We would avoid the linked_ptr
drawbacks.

https://codereview.chromium.org/513683002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/513683002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/file_system_provider.idl:54: // Thumbnail image as
a data URI. Optional, but can be provided only when
On 2014/08/27 09:20:07, hirono wrote:
> How about describing about image types of the thumbnail here?

Done.

https://codereview.chromium.org/513683002/diff/20001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/513683002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:149:
if (!options.thumbnail && metadata.thumbnail) {
On 2014/08/27 09:20:08, hirono wrote:
> How about checking format of thumbnail?
> The string may not start with 'data:' and valid image types.

Good idea. If users passed something invalid (like http://... for urls allowed
by our csp) then it could affect security. Added here and a sanity check in
get_metadata.cc. Done.

[hirono, 2014-08-29 03:53:14]

> How about passing EntryMetadata as a scoped_ptr? We would avoid the linked_ptr
> drawbacks.

In this case, can EntryMetadata be marked as DISALLOW_COPY_AND_ASSIGN?

[mtomasz, 2014-08-29 03:53:57]

On 2014/08/29 03:53:14, hirono wrote:
> > How about passing EntryMetadata as a scoped_ptr? We would avoid the
linked_ptr
> > drawbacks.
> 
> In this case, can EntryMetadata be marked as DISALLOW_COPY_AND_ASSIGN?

Of course!

[hirono, 2014-08-29 03:58:32]

https://codereview.chromium.org/513683002/diff/40001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/513683002/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:43:
var uriLowerCase = uri.toLowerCase();
uri.substr(0, 20).toLowerCase() may be good for performance.

[mtomasz, 2014-08-29 05:20:27]

The scoped_ptr change made the patch big. Sorry for that. PTAL.

https://codereview.chromium.org/513683002/diff/40001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/513683002/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:43:
var uriLowerCase = uri.toLowerCase();
On 2014/08/29 03:58:32, hirono wrote:
> uri.substr(0, 20).toLowerCase() may be good for performance.

Good point. I optimized it in C++, but forgot about JS. Done.

[hirono, 2014-08-29 06:02:41]

https://codereview.chromium.org/513683002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/513683002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:128:
scoped_ptr<EntryMetadata>& metadata = it->second->metadata;
const scoped_ptr<EntryMetadta>& ?
or just EntryMetadata* ?

https://codereview.chromium.org/513683002/diff/60001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/513683002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:44:
var uriLowerCase = uri.substr(0, 32).toLowerCase();
Sorry I re-thought of this. Using regexp is better here. The function may be
called multiple times. It does not generate copy of strings like substr and
toLowerCase.

[mtomasz, 2014-08-29 06:17:48]

https://codereview.chromium.org/513683002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/513683002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:128:
scoped_ptr<EntryMetadata>& metadata = it->second->metadata;
On 2014/08/29 06:02:41, hirono wrote:
> const scoped_ptr<EntryMetadta>& ?
> or just EntryMetadata* ?

Done.

https://codereview.chromium.org/513683002/diff/60001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/513683002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:44:
var uriLowerCase = uri.substr(0, 32).toLowerCase();
On 2014/08/29 06:02:41, hirono wrote:
> Sorry I re-thought of this. Using regexp is better here. The function may be
> called multiple times. It does not generate copy of strings like substr and
> toLowerCase.

Done.

[hirono, 2014-08-29 11:14:26]

lgtm! Thank you!

[mtomasz, 2014-09-01 00:16:42]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2014-09-01 00:17:09]

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

[mtomasz, 2014-09-01 00:17:11]

@benwells: PTAL at IDL.

[commit-bot: I haz the power, 2014-09-01 00:17:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/513683002/80001

[commit-bot: I haz the power, 2014-09-01 01:06:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-09-01 01:10:10]

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

[commit-bot: I haz the power, 2014-09-01 01:10:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[benwells, 2014-09-01 01:31:59]

lgtm

[mtomasz, 2014-09-01 04:37:51]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-09-01 04:38:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/513683002/80001

[commit-bot: I haz the power, 2014-09-01 05:14:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-09-01 05:49:38]

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

[commit-bot: I haz the power, 2014-09-01 05:49:39]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mtomasz, 2014-09-01 06:42:36]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-09-01 06:43:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/513683002/100001

[commit-bot: I haz the power, 2014-09-01 07:57:28]

Committed patchset #6 (id:100001) as 0a021992e1be21e2b2ad0ed0d70be11ecf9cb58e

[commit-bot: I haz the power, 2014-09-10 03:15:49]

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