Files.app: Let DirectoryContents controls metadata cache size by relative value

Files.app: Let DirectoryContents controls metadata cache size by relative value

Previously, DirectoryContents controlled the size of metadata cache by absolute value. But multiple of instances of DirectoryContents might be created at same time and it might set wrong cache size. 

This patch lets DirectoryContents controls metadata cache size by relative value, instead of absolute value, and solves that problem.

BUG=373629
TEST=manually

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275771

[yoshiki, 2014-06-05 17:52:10]

@mtoamsz: PTAL. Thanks.

[mtomasz, 2014-06-06 00:28:09]

On 2014/06/05 17:52:10, yoshiki wrote:
> @mtoamsz: PTAL. Thanks.

Why is it possible to create more than one DirectoryContents in the same time?
Shouldn't we fix it? Also, how about simplifying, and removing cache size limit
in metadata cache at all? Instead basically we could clear the cache once we
change directory. That would significantly simplify the code. WDYT?

[yoshiki, 2014-06-06 02:31:15]

On 2014/06/06 00:28:09, mtomasz wrote:
> On 2014/06/05 17:52:10, yoshiki wrote:
> > @mtoamsz: PTAL. Thanks.
> 
> Why is it possible to create more than one DirectoryContents in the same time?
> Shouldn't we fix it?

When the content in directory is changed, we use two DirectoryContents to
prevent flickering, like double-buffering. The instance of DirectoryContent is
cloned in DirecotyrMode.rescan(), and is replaced with cloned one in
DirecotyrMode.replaceDirectoryContents().

> Also, how about simplifying, and removing cache size limit
> in metadata cache at all? Instead basically we could clear the cache once we
> change directory. That would significantly simplify the code. WDYT?

The metadata cache is used not only in DirectoryContent but also NavBar and
DirTree and the cache saves some time during back and forth among directories.
Before we remove the cache size limit, we should stop sharing the same instance
of MetadataCache among all components in Files.app.

From long term view, I agree we need some improvement in metadata cache. But I
think there is no simple and good solution to do in a short time.

[mtomasz, 2014-06-06 03:34:44]

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

https://codereview.chromium.org/310633002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_contents.js:494: var delta
= size - this.currentMetadataCacheSize_;
CurrentMetadataCacheSize_ is always 0.

[yoshiki, 2014-06-06 04:24:26]

@mtomasz: PTAL

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

https://codereview.chromium.org/310633002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_contents.js:494: var delta
= size - this.currentMetadataCacheSize_;
On 2014/06/06 03:34:43, mtomasz wrote:
> CurrentMetadataCacheSize_ is always 0.

Oops, sorry. I missed it when creating the patch. Fixed.

[mtomasz, 2014-06-06 07:38:14]

https://codereview.chromium.org/310633002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_contents.js (right):

https://codereview.chromium.org/310633002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_contents.js:459:
this.currentMetadataCacheSize_ = 0;
Naming is confusing. This is not size of metadata cache, but number of elements
in the metadata cache related to this instance of DirectoryContents. Size of the
metadata cache may be bigger than currentMetadataCacheSize.

How about: this.lastFileListSize_? And then:

DirContents.prototype.dispose = function() {
  this.context_.metadataCache.resizeBy(-this.lastFileListSize_);
};

DirContents.prototype.makeSpaceInMetadataCache = function() {
  this.context_.metadataCache.resizeBy(
      this.fileList_.length - this.lastFileListSize_);
  this.lastFileListSize_ = this.fileList.length;
};

The advantage is that we avoid unclear naming (setMetadataCacheSize,
currentMetadataCacheSize, while they do not mean size of the metadata cache).

Just a suggestion.

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

https://codereview.chromium.org/310633002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js:171:
MetadataCache.prototype.changeCacheSize = function(delta) {
Naming is confusing here. How about resizeBy(delta)? (Like window.scrollBy,
window.resizeBy)?

[yoshiki, 2014-06-06 08:17:22]

@mtomasz: PTAL. Thanks.

https://codereview.chromium.org/310633002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_contents.js (right):

https://codereview.chromium.org/310633002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_contents.js:459:
this.currentMetadataCacheSize_ = 0;
On 2014/06/06 07:38:14, mtomasz wrote:
> Naming is confusing. This is not size of metadata cache, but number of
elements
> in the metadata cache related to this instance of DirectoryContents. Size of
the
> metadata cache may be bigger than currentMetadataCacheSize.
> 
> How about: this.lastFileListSize_? And then:
> 
> DirContents.prototype.dispose = function() {
>   this.context_.metadataCache.resizeBy(-this.lastFileListSize_);
> };
> 
> DirContents.prototype.makeSpaceInMetadataCache = function() {
>   this.context_.metadataCache.resizeBy(
>       this.fileList_.length - this.lastFileListSize_);
>   this.lastFileListSize_ = this.fileList.length;
> };
> 
> The advantage is that we avoid unclear naming (setMetadataCacheSize,
> currentMetadataCacheSize, while they do not mean size of the metadata cache).
> 
> Just a suggestion.

Thanks for good suggestion. I almost agreed but I'd like to keep an argument of
makeSpaceInMetadataCache(), because in my other patch
(https://codereview.chromium.org/301943002/) I'd like to allocate metadata size
in advance of flielist.

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

https://codereview.chromium.org/310633002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js:171:
MetadataCache.prototype.changeCacheSize = function(delta) {
On 2014/06/06 07:38:14, mtomasz wrote:
> Naming is confusing here. How about resizeBy(delta)? (Like window.scrollBy,
> window.resizeBy)?

Good idea! Done.

[mtomasz, 2014-06-06 09:20:05]

Thanks! LGTM!

[yoshiki, 2014-06-06 09:21:42]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-06-06 09:23:50]

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

[commit-bot: I haz the power, 2014-06-06 13:24:17]

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

[commit-bot: I haz the power, 2014-06-06 15:14:59]

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

[commit-bot: I haz the power, 2014-06-06 15:15:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[yoshiki, 2014-06-06 15:27:09]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-06-06 15:28:05]

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

[commit-bot: I haz the power, 2014-06-06 19:12:49]

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

[commit-bot: I haz the power, 2014-06-06 20:56:40]

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

[commit-bot: I haz the power, 2014-06-06 20:56:41]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[yoshiki, 2014-06-09 01:53:50]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2014-06-09 01:53:56]

The CQ bit was unchecked by yoshiki@chromium.org

[yoshiki, 2014-06-09 01:53:57]

The CQ bit was checked by yoshiki@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoshiki@chromium.org/310633002/120001

[commit-bot: I haz the power, 2014-06-09 03:47:24]

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

[commit-bot: I haz the power, 2014-06-09 05:08:49]

Change committed as 275771