Files.app: Start to use new metadata models in DirectoryContents.

Files.app: Start to use new metadata models in DirectoryContents.

 * Prefetch metadata before entries are added to file list.
 * Notify the cache of entry changes.

BUG=410766
TEST=None

Committed: https://crrev.com/205597244898a72cc95ce414bac6c190d613a46f
Cr-Commit-Position: refs/heads/master@{#315284}

[hirono, 2015-02-06 07:02:48]

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

[hirono, 2015-02-06 07:02:48]

PTAL, thanks!

[mtomasz, 2015-02-06 09:00:58]

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js
(right):

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:68:
for (var i = 0; i < names.length; i++) {
Shall it be responsibility of providers or models to perform filtering?

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:70:
fileSystemNames.push(names[i]);
nit: Maybe we can use Array::filter?

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:74:
this.fileSystemMetadataProvider_.get(localEntries, fileSystemNames),
nit: Can we use jsdoc to ensure that we don't pass incorrect properties to
providers? Currently we allow any strings. WDYT?

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:110:
* @param {!Array<string>} urls
Could you document why urls are used instead of entries in some methods?

[hirono, 2015-02-06 09:28:46]

Thanks!

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js
(right):

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:68:
for (var i = 0; i < names.length; i++) {
On 2015/02/06 09:00:58, mtomasz wrote:
> Shall it be responsibility of providers or models to perform filtering?

Providers have assertion for invalid strings.
The model integrates two providers, and some properties are available only for
ExternalMetadataProvider, so the model filters out properties for
FileSystemMetadataProvider.

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:70:
fileSystemNames.push(names[i]);
On 2015/02/06 09:00:57, mtomasz wrote:
> nit: Maybe we can use Array::filter?

In metadata related code, I tries to avoid Array::filter or Array::map, because
it increases the number of function call.

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:74:
this.fileSystemMetadataProvider_.get(localEntries, fileSystemNames),
On 2015/02/06 09:00:58, mtomasz wrote:
> nit: Can we use jsdoc to ensure that we don't pass incorrect properties to
> providers? Currently we allow any strings. WDYT?

Actually no. The names specified as array, and closure compiler cannot detect
invalid values in Array.

https://jscomp-debug.googleplex.com/#input0%3D%252F**%250A%2520*%2520%2540enu...

Instead I added assertions in the providers.

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:110:
* @param {!Array<string>} urls
On 2015/02/06 09:00:58, mtomasz wrote:
> Could you document why urls are used instead of entries in some methods?

Done.

[mtomasz, 2015-02-09 02:39:11]

lgtm

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js
(right):

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:68:
for (var i = 0; i < names.length; i++) {
On 2015/02/06 09:28:46, hirono wrote:
> On 2015/02/06 09:00:58, mtomasz wrote:
> > Shall it be responsibility of providers or models to perform filtering?
> 
> Providers have assertion for invalid strings.
> The model integrates two providers, and some properties are available only for
> ExternalMetadataProvider, so the model filters out properties for
> FileSystemMetadataProvider.
> 

Asserts are OK, but another idea would be to put responsibility for filtering on
providers. They would return whatever they can, and ignore properties they don't
support. Similarly (a little bit) querySelector('a, b, c') would return only
things which match. But either is fine, so up to you.

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:70:
fileSystemNames.push(names[i]);
On 2015/02/06 09:28:46, hirono wrote:
> On 2015/02/06 09:00:57, mtomasz wrote:
> > nit: Maybe we can use Array::filter?
> 
> In metadata related code, I tries to avoid Array::filter or Array::map,
because
> it increases the number of function call.

Hm, I think it's an over-optimization. Size of fileSystemNames is small, and
cost of Array.filter ignorable to cost of IPCs. But I don't have strong feelings
here, so it's up to you.

[hirono, 2015-02-09 08:44:09]

New patchsets have been uploaded after l-g-t-m from mtomasz@chromium.org

[hirono, 2015-02-09 08:44:30]

Thanks!

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js
(right):

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:68:
for (var i = 0; i < names.length; i++) {
On 2015/02/09 02:39:11, mtomasz wrote:
> On 2015/02/06 09:28:46, hirono wrote:
> > On 2015/02/06 09:00:58, mtomasz wrote:
> > > Shall it be responsibility of providers or models to perform filtering?
> > 
> > Providers have assertion for invalid strings.
> > The model integrates two providers, and some properties are available only
for
> > ExternalMetadataProvider, so the model filters out properties for
> > FileSystemMetadataProvider.
> > 
> 
> Asserts are OK, but another idea would be to put responsibility for filtering
on
> providers. They would return whatever they can, and ignore properties they
don't
> support. Similarly (a little bit) querySelector('a, b, c') would return only
> things which match. But either is fine, so up to you.

If we ignore invalid property names, it is unnoticeable if we specify property
names in wrong spell. So let me keep assert here.

https://codereview.chromium.org/904003002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:70:
fileSystemNames.push(names[i]);
On 2015/02/09 02:39:11, mtomasz wrote:
> On 2015/02/06 09:28:46, hirono wrote:
> > On 2015/02/06 09:00:57, mtomasz wrote:
> > > nit: Maybe we can use Array::filter?
> > 
> > In metadata related code, I tries to avoid Array::filter or Array::map,
> because
> > it increases the number of function call.
> 
> Hm, I think it's an over-optimization. Size of fileSystemNames is small, and
> cost of Array.filter ignorable to cost of IPCs. But I don't have strong
feelings
> here, so it's up to you.

Agree with you. Turns it to filter. Thanks!

[hirono, 2015-02-09 08:44:33]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2015-02-09 08:45:34]

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

[commit-bot: I haz the power, 2015-02-09 10:44:37]

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

[commit-bot: I haz the power, 2015-02-09 10:44:38]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  ios_rel_device_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[hirono, 2015-02-09 11:01:25]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2015-02-09 11:01:38]

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

[commit-bot: I haz the power, 2015-02-09 11:03:59]

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

[commit-bot: I haz the power, 2015-02-09 11:04:00]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[hirono, 2015-02-09 11:15:52]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2015-02-09 11:16:38]

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

[commit-bot: I haz the power, 2015-02-09 13:08:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-09 13:09:00]

Patchset 3 (id:??) landed as
https://crrev.com/205597244898a72cc95ce414bac6c190d613a46f
Cr-Commit-Position: refs/heads/master@{#315284}