Files.app: Extract cache part from NewMetadataProvider.

Files.app: Extract cache part from NewMetadataProvider.

Previously FileSystemMetadata has multiple providers, and it switches the
providers depends on Entry state. e.g. If the entry is dirty, it uses
ContentProvider instead of ExternalProvider.

Previously each provider cached its result but they shared single
MetadataProviderCache instance. Thus if ExternalProvider returns values that are
out of sync, the values are cached and ContentProvider is never called.

The CL extracts the caching part of NewMetadataProvider as
CachedMetadataProvider wrapper class. And let it switch providers before
caching.

BUG=410766
TEST=FileManagerJsTest.*Metadata*

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

[hirono, 2015-03-02 06:17:03]

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

[hirono, 2015-03-02 06:17:04]

Sorry for the huge CL. Please feel free to ask if you have questions about the
CL.
Thanks!

[mtomasz, 2015-03-02 07:17:38]

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

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/external_metadata_provider.js:14:
NewMetadataProvider.call(
nit: Will it fit in one line?

https://codereview.chromium.org/971653003/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/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:11:
function FileSystemMetadata(volumeManager) {
Do we need this class? It has no functionality. How about converting
MultiMetadataProvider to FileSystemMetadata instead?

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:14:
new MultiMetadataProvider(
nit: How about better naming? Currently we have the chain FileSystemMetadata -> 
MultiMetadataProvider -> FileSystemProvider.

Maybe we could rename FileSystemMetadata to something else, so there is no
confusion with FileSystemProvider?  FileSystemMetadata returns much more than
FileSystemProvider provides, but they share the same prefix. WDYT?

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

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:83:
CachedMetadataProvider.prototype.get = function(entries, names) {
CachedMetadataProvider and NewMetadataProvider sound like bthey are both
providers, but they do not share a common interface. Their get() is similar, but
not the same. Can we unify them, and share the same interface?

How about making NewMetadataProvider an interface, and later make
CachedMetadataProvider implement that interface?

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:156:
this.cache_.clear(util.entriesToURLs(entries));
Do we need to both expose this.cache_ as cacheModel if we have these methods?

[hirono, 2015-03-02 08:32:18]

Thanks!

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

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/external_metadata_provider.js:14:
NewMetadataProvider.call(
On 2015/03/02 07:17:38, mtomasz wrote:
> nit: Will it fit in one line?

Done.

https://codereview.chromium.org/971653003/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/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:11:
function FileSystemMetadata(volumeManager) {
On 2015/03/02 07:17:38, mtomasz wrote:
> Do we need this class? It has no functionality. How about converting
> MultiMetadataProvider to FileSystemMetadata instead?

Can I handle it at the same time with #14's fix?

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:14:
new MultiMetadataProvider(
On 2015/03/02 07:17:38, mtomasz wrote:
> nit: How about better naming? Currently we have the chain FileSystemMetadata
-> 
> MultiMetadataProvider -> FileSystemProvider.
> 
> Maybe we could rename FileSystemMetadata to something else, so there is no
> confusion with FileSystemProvider?  FileSystemMetadata returns much more than
> FileSystemProvider provides, but they share the same prefix. WDYT?

I also considered it. Maybe MetadataModel?
Can I handle it separated CL? The CL has already been huge and renaming will
affect much more files.

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

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:83:
CachedMetadataProvider.prototype.get = function(entries, names) {
On 2015/03/02 07:17:38, mtomasz wrote:
> CachedMetadataProvider and NewMetadataProvider sound like bthey are both
> providers, but they do not share a common interface. Their get() is similar,
but
> not the same. Can we unify them, and share the same interface?
> 
> How about making NewMetadataProvider an interface, and later make
> CachedMetadataProvider implement that interface?

CachedMetadataProvider#get is used by client code directly. So get(entries,
names) is good for convenient. On the other hand, NewMetadataProvider#get is
called from CachedMetadataProvider. it means some entry&property pairs might
have been cached, and others might not. I'd like to avoid get(entries, names)
here, otherwise we need to refetch all thumbnails even if one entry's thumbnail
is not cached.

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:156:
this.cache_.clear(util.entriesToURLs(entries));
On 2015/03/02 07:17:38, mtomasz wrote:
> Do we need to both expose this.cache_ as cacheModel if we have these methods?
Nope. I stopped to expose this.cache_. This has internal methods like raw
invalidate.

[mtomasz, 2015-03-02 08:55:27]

lgtm. Other things we can discuss/do in a follow up CL.

https://codereview.chromium.org/971653003/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/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:11:
function FileSystemMetadata(volumeManager) {
On 2015/03/02 08:32:18, hirono wrote:
> On 2015/03/02 07:17:38, mtomasz wrote:
> > Do we need this class? It has no functionality. How about converting
> > MultiMetadataProvider to FileSystemMetadata instead?
> 
> Can I handle it at the same time with #14's fix?

Sure. But maybe we actually want to keep it? See the comment about sharing the
interface.

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/file_system_metadata.js:14:
new MultiMetadataProvider(
On 2015/03/02 08:32:18, hirono wrote:
> On 2015/03/02 07:17:38, mtomasz wrote:
> > nit: How about better naming? Currently we have the chain FileSystemMetadata
> -> 
> > MultiMetadataProvider -> FileSystemProvider.
> > 
> > Maybe we could rename FileSystemMetadata to something else, so there is no
> > confusion with FileSystemProvider?  FileSystemMetadata returns much more
than
> > FileSystemProvider provides, but they share the same prefix. WDYT?
> 
> I also considered it. Maybe MetadataModel?
> Can I handle it separated CL? The CL has already been huge and renaming will
> affect much more files.

MetadataModel SGTM. Separate CL SGTM too.

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

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:83:
CachedMetadataProvider.prototype.get = function(entries, names) {
On 2015/03/02 08:32:18, hirono wrote:
> On 2015/03/02 07:17:38, mtomasz wrote:
> > CachedMetadataProvider and NewMetadataProvider sound like bthey are both
> > providers, but they do not share a common interface. Their get() is similar,
> but
> > not the same. Can we unify them, and share the same interface?
> > 
> > How about making NewMetadataProvider an interface, and later make
> > CachedMetadataProvider implement that interface?
> 
> CachedMetadataProvider#get is used by client code directly. So get(entries,
> names) is good for convenient. On the other hand, NewMetadataProvider#get is
> called from CachedMetadataProvider. it means some entry&property pairs might
> have been cached, and others might not. I'd like to avoid get(entries, names)
> here, otherwise we need to refetch all thumbnails even if one entry's
thumbnail
> is not cached.

Maybe we could add such convenience method to FileSystemMetadata? All providers
(cached and not cached) would share the same NewMetadataProvider interface
making the architecture simpler. WDYT?

[hirono, 2015-03-02 09:13:24]

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

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:83:
CachedMetadataProvider.prototype.get = function(entries, names) {
On 2015/03/02 08:55:27, mtomasz wrote:
> On 2015/03/02 08:32:18, hirono wrote:
> > On 2015/03/02 07:17:38, mtomasz wrote:
> > > CachedMetadataProvider and NewMetadataProvider sound like bthey are both
> > > providers, but they do not share a common interface. Their get() is
similar,
> > but
> > > not the same. Can we unify them, and share the same interface?
> > > 
> > > How about making NewMetadataProvider an interface, and later make
> > > CachedMetadataProvider implement that interface?
> > 
> > CachedMetadataProvider#get is used by client code directly. So get(entries,
> > names) is good for convenient. On the other hand, NewMetadataProvider#get is
> > called from CachedMetadataProvider. it means some entry&property pairs might
> > have been cached, and others might not. I'd like to avoid get(entries,
names)
> > here, otherwise we need to refetch all thumbnails even if one entry's
> thumbnail
> > is not cached.
> 
> Maybe we could add such convenience method to FileSystemMetadata? All
providers
> (cached and not cached) would share the same NewMetadataProvider interface
> making the architecture simpler. WDYT?

If we keep FileSystemMetadata, it provides the convenience get(entries, names)
and delegates all other methods of CachedMetadataProvider. So how about just
renaming CachedMetadataProvider with MetadataModel?

[mtomasz, 2015-03-02 09:21:01]

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

https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:83:
CachedMetadataProvider.prototype.get = function(entries, names) {
On 2015/03/02 09:13:24, hirono wrote:
> On 2015/03/02 08:55:27, mtomasz wrote:
> > On 2015/03/02 08:32:18, hirono wrote:
> > > On 2015/03/02 07:17:38, mtomasz wrote:
> > > > CachedMetadataProvider and NewMetadataProvider sound like bthey are both
> > > > providers, but they do not share a common interface. Their get() is
> similar,
> > > but
> > > > not the same. Can we unify them, and share the same interface?
> > > > 
> > > > How about making NewMetadataProvider an interface, and later make
> > > > CachedMetadataProvider implement that interface?
> > > 
> > > CachedMetadataProvider#get is used by client code directly. So
get(entries,
> > > names) is good for convenient. On the other hand, NewMetadataProvider#get
is
> > > called from CachedMetadataProvider. it means some entry&property pairs
might
> > > have been cached, and others might not. I'd like to avoid get(entries,
> names)
> > > here, otherwise we need to refetch all thumbnails even if one entry's
> > thumbnail
> > > is not cached.
> > 
> > Maybe we could add such convenience method to FileSystemMetadata? All
> providers
> > (cached and not cached) would share the same NewMetadataProvider interface
> > making the architecture simpler. WDYT?
> 
> If we keep FileSystemMetadata, it provides the convenience get(entries, names)
> and delegates all other methods of CachedMetadataProvider. So how about just
> renaming CachedMetadataProvider with MetadataModel?

If we do that, we can't share the same interface between CachedMetadataProvider
and other providers, right? What I'm concerned about that both
CachedMetadataProvider and other providers are all providers, and they all have
get() method, but the arguments are different depending on which provider it is,
which is quite confusing.

[hirono, 2015-03-02 09:29:09]

On 2015/03/02 09:21:01, mtomasz wrote:
>
https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
> File
> ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js
> (right):
> 
>
https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
>
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:83:
> CachedMetadataProvider.prototype.get = function(entries, names) {
> On 2015/03/02 09:13:24, hirono wrote:
> > On 2015/03/02 08:55:27, mtomasz wrote:
> > > On 2015/03/02 08:32:18, hirono wrote:
> > > > On 2015/03/02 07:17:38, mtomasz wrote:
> > > > > CachedMetadataProvider and NewMetadataProvider sound like bthey are
both
> > > > > providers, but they do not share a common interface. Their get() is
> > similar,
> > > > but
> > > > > not the same. Can we unify them, and share the same interface?
> > > > > 
> > > > > How about making NewMetadataProvider an interface, and later make
> > > > > CachedMetadataProvider implement that interface?
> > > > 
> > > > CachedMetadataProvider#get is used by client code directly. So
> get(entries,
> > > > names) is good for convenient. On the other hand,
NewMetadataProvider#get
> is
> > > > called from CachedMetadataProvider. it means some entry&property pairs
> might
> > > > have been cached, and others might not. I'd like to avoid get(entries,
> > names)
> > > > here, otherwise we need to refetch all thumbnails even if one entry's
> > > thumbnail
> > > > is not cached.
> > > 
> > > Maybe we could add such convenience method to FileSystemMetadata? All
> > providers
> > > (cached and not cached) would share the same NewMetadataProvider interface
> > > making the architecture simpler. WDYT?
> > 
> > If we keep FileSystemMetadata, it provides the convenience get(entries,
names)
> > and delegates all other methods of CachedMetadataProvider. So how about just
> > renaming CachedMetadataProvider with MetadataModel?
> 
> If we do that, we can't share the same interface between
CachedMetadataProvider
> and other providers, right? What I'm concerned about that both
> CachedMetadataProvider and other providers are all providers, and they all
have
> get() method, but the arguments are different depending on which provider it
is,
> which is quite confusing.

After renaming CachedMetadataProvider -> MetadataModel, CachedMetadataProvider
no longer looks like a provider.
After that, Provider is class that providing metadata to MetadataModel. So
having different signature in MetadataModel and other providers is not problem.

New structure looks like:

MetadataModel (renamed from CachedMetadataProvider) -> MultiMetadataProvider ->
XXXProvider

[mtomasz, 2015-03-02 09:35:30]

On 2015/03/02 09:29:09, hirono wrote:
> On 2015/03/02 09:21:01, mtomasz wrote:
> >
>
https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
> > File
> > ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
> >
>
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:83:
> > CachedMetadataProvider.prototype.get = function(entries, names) {
> > On 2015/03/02 09:13:24, hirono wrote:
> > > On 2015/03/02 08:55:27, mtomasz wrote:
> > > > On 2015/03/02 08:32:18, hirono wrote:
> > > > > On 2015/03/02 07:17:38, mtomasz wrote:
> > > > > > CachedMetadataProvider and NewMetadataProvider sound like bthey are
> both
> > > > > > providers, but they do not share a common interface. Their get() is
> > > similar,
> > > > > but
> > > > > > not the same. Can we unify them, and share the same interface?
> > > > > > 
> > > > > > How about making NewMetadataProvider an interface, and later make
> > > > > > CachedMetadataProvider implement that interface?
> > > > > 
> > > > > CachedMetadataProvider#get is used by client code directly. So
> > get(entries,
> > > > > names) is good for convenient. On the other hand,
> NewMetadataProvider#get
> > is
> > > > > called from CachedMetadataProvider. it means some entry&property pairs
> > might
> > > > > have been cached, and others might not. I'd like to avoid get(entries,
> > > names)
> > > > > here, otherwise we need to refetch all thumbnails even if one entry's
> > > > thumbnail
> > > > > is not cached.
> > > > 
> > > > Maybe we could add such convenience method to FileSystemMetadata? All
> > > providers
> > > > (cached and not cached) would share the same NewMetadataProvider
interface
> > > > making the architecture simpler. WDYT?
> > > 
> > > If we keep FileSystemMetadata, it provides the convenience get(entries,
> names)
> > > and delegates all other methods of CachedMetadataProvider. So how about
just
> > > renaming CachedMetadataProvider with MetadataModel?
> > 
> > If we do that, we can't share the same interface between
> CachedMetadataProvider
> > and other providers, right? What I'm concerned about that both
> > CachedMetadataProvider and other providers are all providers, and they all
> have
> > get() method, but the arguments are different depending on which provider it
> is,
> > which is quite confusing.
> 
> After renaming CachedMetadataProvider -> MetadataModel, CachedMetadataProvider
> no longer looks like a provider.
> After that, Provider is class that providing metadata to MetadataModel. So
> having different signature in MetadataModel and other providers is not
problem.
> 
> New structure looks like:
> 
> MetadataModel (renamed from CachedMetadataProvider) -> MultiMetadataProvider
->
> XXXProvider

Ah, you're right! SGTM!

[hirono, 2015-03-02 09:40:40]

The CQ bit was checked by hirono@chromium.org

[hirono, 2015-03-02 09:40:41]

The patchset sent to the CQ was uploaded after l-g-t-m from mtomasz@chromium.org
Link to the patchset: https://codereview.chromium.org/971653003/#ps40001 (title:
"Fix audio player.")

[hirono, 2015-03-02 09:40:59]

On 2015/03/02 09:35:30, mtomasz wrote:
> On 2015/03/02 09:29:09, hirono wrote:
> > On 2015/03/02 09:21:01, mtomasz wrote:
> > >
> >
>
https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
> > > File
> > >
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/971653003/diff/1/ui/file_manager/file_manager...
> > >
> >
>
ui/file_manager/file_manager/foreground/js/metadata/new_metadata_provider.js:83:
> > > CachedMetadataProvider.prototype.get = function(entries, names) {
> > > On 2015/03/02 09:13:24, hirono wrote:
> > > > On 2015/03/02 08:55:27, mtomasz wrote:
> > > > > On 2015/03/02 08:32:18, hirono wrote:
> > > > > > On 2015/03/02 07:17:38, mtomasz wrote:
> > > > > > > CachedMetadataProvider and NewMetadataProvider sound like bthey
are
> > both
> > > > > > > providers, but they do not share a common interface. Their get()
is
> > > > similar,
> > > > > > but
> > > > > > > not the same. Can we unify them, and share the same interface?
> > > > > > > 
> > > > > > > How about making NewMetadataProvider an interface, and later make
> > > > > > > CachedMetadataProvider implement that interface?
> > > > > > 
> > > > > > CachedMetadataProvider#get is used by client code directly. So
> > > get(entries,
> > > > > > names) is good for convenient. On the other hand,
> > NewMetadataProvider#get
> > > is
> > > > > > called from CachedMetadataProvider. it means some entry&property
pairs
> > > might
> > > > > > have been cached, and others might not. I'd like to avoid
get(entries,
> > > > names)
> > > > > > here, otherwise we need to refetch all thumbnails even if one
entry's
> > > > > thumbnail
> > > > > > is not cached.
> > > > > 
> > > > > Maybe we could add such convenience method to FileSystemMetadata? All
> > > > providers
> > > > > (cached and not cached) would share the same NewMetadataProvider
> interface
> > > > > making the architecture simpler. WDYT?
> > > > 
> > > > If we keep FileSystemMetadata, it provides the convenience get(entries,
> > names)
> > > > and delegates all other methods of CachedMetadataProvider. So how about
> just
> > > > renaming CachedMetadataProvider with MetadataModel?
> > > 
> > > If we do that, we can't share the same interface between
> > CachedMetadataProvider
> > > and other providers, right? What I'm concerned about that both
> > > CachedMetadataProvider and other providers are all providers, and they all
> > have
> > > get() method, but the arguments are different depending on which provider
it
> > is,
> > > which is quite confusing.
> > 
> > After renaming CachedMetadataProvider -> MetadataModel,
CachedMetadataProvider
> > no longer looks like a provider.
> > After that, Provider is class that providing metadata to MetadataModel. So
> > having different signature in MetadataModel and other providers is not
> problem.
> > 
> > New structure looks like:
> > 
> > MetadataModel (renamed from CachedMetadataProvider) -> MultiMetadataProvider
> ->
> > XXXProvider
> 
> Ah, you're right! SGTM!

Thanks! I'll create the next CL based on the discussion!

[commit-bot: I haz the power, 2015-03-02 09:41:05]

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

[commit-bot: I haz the power, 2015-03-02 10:55:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-02 10:55:31]

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