Extract ContentScanner from DirectoryContent.

Extract ContentScanner from DirectoryContent.

DirectoryContent had basically two functions.
1) The list of entries.
2) The scanning entries from backend.
This CL extracts 2) part from DirectoryContent and create ContentScanner to
simplify the code path. At the same time, some events sent from
DirectoryContent was resorted, and their handlers in FileManager is fixed at
the same time.

BUG=279614
TEST=Ran browser_tests --gtest_filter="*FileSystemExtensionApiTest*:*FileManagerBrowserTest*:*FileBrowserPrivateApiTest*" and tested manually.

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

[hidehiko, 2013-09-30 14:37:00]

Thank you for your review in advance.
(cc: yoshiki@ just in case for the conflicting).
I'll send another CL to clean up DirectoryContents more.

- hidehiko

[mtomasz, 2013-10-01 02:13:43]

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/async_util.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/async_util.js:93:
AsyncUtil.Queue.prototype.cancel = function() {
FYI: A more complete cancellation logic is planned. Experimental CL:
https://codereview.chromium.org/17212002/.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/async_util.js:94:
this.closures_.splice(0, this.closures_.length);
nit: How about just this.closures_ = []?

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/directory_contents.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:8: * Scanner of
the entries.
@constructor is missing

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:16: * directory,
or starts to search with some query on a file system.
nit: Please add a new line after the description in log jsdocs for better
readability.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:40: * @extends
{ContentScanner}
@constructor is missing

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:59: * @param
{function(FileError)} errorCallback Called an error occurs.
@override is missing

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:97: * @extends
{ContentScanner}
ditto (@constructor)

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:111: * server
requests while user is typeing the query.
nit: typeing -> typing
nit: in milliseconds (we always specify units)

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:133: * @param
{function(FileError)} errorCallback Called an error occurs.
nit: ditto (@override)

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:191: * @param
{string} query The query of the search.
ditto

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:211: * @param
{function(FileError)} errorCallback Called an error occurs.
ditto here and in other places

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:288: * The search
types on the Drive File System.
@enum {string} missing

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:306:
DriveMetadataSearchContentScanner.prototype.startScan = function(
optional: how about just scan() instead of startScan()?

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:2713: if
(this.scanUpdatedTimer_) {
Can you please briefly explain this change? Thanks!

[mtomasz, 2013-10-01 02:25:23]

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:2791: if
(this.scanInProgress_) {
This will always return true because of #2771, so we can remove.

[mtomasz, 2013-10-01 02:36:39]

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/directory_contents.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:695:
this.scanner_ = new DirectoryContentScanner(this.entry_);
Do we need to create a new instance of scanner every time? If so, then how about
old instances? Shall we cancel them before creating a new one?

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:787:
this.scanner_ = new DriveSearchContentScanner(this.query_);
Same here.

[hidehiko, 2013-10-01 04:12:23]

Thank you for your review. PTAL?

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/async_util.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/async_util.js:93:
AsyncUtil.Queue.prototype.cancel = function() {
On 2013/10/01 02:13:44, mtomasz wrote:
> FYI: A more complete cancellation logic is planned. Experimental CL:
> https://codereview.chromium.org/17212002/.

Good to know. As we talked offline, please let me go with the current version.
The complete one should be almost compatible with this one.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/async_util.js:94:
this.closures_.splice(0, this.closures_.length);
On 2013/10/01 02:13:44, mtomasz wrote:
> nit: How about just this.closures_ = []?

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/directory_contents.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:8: * Scanner of
the entries.
On 2013/10/01 02:13:44, mtomasz wrote:
> @constructor is missing

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:16: * directory,
or starts to search with some query on a file system.
On 2013/10/01 02:13:44, mtomasz wrote:
> nit: Please add a new line after the description in log jsdocs for better
> readability.

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:40: * @extends
{ContentScanner}
On 2013/10/01 02:13:44, mtomasz wrote:
> @constructor is missing

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:59: * @param
{function(FileError)} errorCallback Called an error occurs.
On 2013/10/01 02:13:44, mtomasz wrote:
> @override is missing

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:97: * @extends
{ContentScanner}
On 2013/10/01 02:13:44, mtomasz wrote:
> ditto (@constructor)

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:111: * server
requests while user is typeing the query.
On 2013/10/01 02:13:44, mtomasz wrote:
> nit: typeing -> typing
> nit: in milliseconds (we always specify units)

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:133: * @param
{function(FileError)} errorCallback Called an error occurs.
On 2013/10/01 02:13:44, mtomasz wrote:
> nit: ditto (@override)

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:191: * @param
{string} query The query of the search.
On 2013/10/01 02:13:44, mtomasz wrote:
> ditto

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:211: * @param
{function(FileError)} errorCallback Called an error occurs.
On 2013/10/01 02:13:44, mtomasz wrote:
> ditto here and in other places

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:288: * The search
types on the Drive File System.
On 2013/10/01 02:13:44, mtomasz wrote:
> @enum {string} missing

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:306:
DriveMetadataSearchContentScanner.prototype.startScan = function(
On 2013/10/01 02:13:44, mtomasz wrote:
> optional: how about just scan() instead of startScan()?

Done.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:695:
this.scanner_ = new DirectoryContentScanner(this.entry_);
On 2013/10/01 02:36:39, mtomasz wrote:
> Do we need to create a new instance of scanner every time? If so, then how
about
> old instances? Shall we cancel them before creating a new one?

Good point.
In the existing code, scan must be called at most once. To rescan, it is
necessary
to clone() the instance, to call cancel() for the older instance, and to call
scan() for the new instance. It is done in DirectoryModel.

It should be clean up, but this CL is already huge, so I'd like to split it from
this CL. WDYT?

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:787:
this.scanner_ = new DriveSearchContentScanner(this.query_);
On 2013/10/01 02:36:39, mtomasz wrote:
> Same here.

Ack.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:2713: if
(this.scanUpdatedTimer_) {
On 2013/10/01 02:13:44, mtomasz wrote:
> Can you please briefly explain this change? Thanks!

This patch fixes some weird event firing in DirectoryContents. Along with it, we
should fix the handlers for correct callback implementation.

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:2791: if
(this.scanInProgress_) {
On 2013/10/01 02:25:23, mtomasz wrote:
> This will always return true because of #2771, so we can remove.

Done.

[mtomasz, 2013-10-01 04:39:55]

lgtm with one comment

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/directory_contents.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:695:
this.scanner_ = new DirectoryContentScanner(this.entry_);
On 2013/10/01 04:12:23, hidehiko wrote:
> On 2013/10/01 02:36:39, mtomasz wrote:
> > Do we need to create a new instance of scanner every time? If so, then how
> about
> > old instances? Shall we cancel them before creating a new one?
> 
> Good point.
> In the existing code, scan must be called at most once. To rescan, it is
> necessary
> to clone() the instance, to call cancel() for the older instance, and to call
> scan() for the new instance. It is done in DirectoryModel.
> 
> It should be clean up, but this CL is already huge, so I'd like to split it
from
> this CL. WDYT?
> 

Can you please add either (1) a TODO line or (2): if (this.scanner_) throw new
Error('Scanner already exists.')? Thanks!

[hidehiko, 2013-10-01 05:28:16]

Thank you for your review!

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/directory_contents.js (right):

https://codereview.chromium.org/25278002/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/directory_contents.js:695:
this.scanner_ = new DirectoryContentScanner(this.entry_);
On 2013/10/01 04:39:55, mtomasz wrote:
> On 2013/10/01 04:12:23, hidehiko wrote:
> > On 2013/10/01 02:36:39, mtomasz wrote:
> > > Do we need to create a new instance of scanner every time? If so, then how
> > about
> > > old instances? Shall we cancel them before creating a new one?
> > 
> > Good point.
> > In the existing code, scan must be called at most once. To rescan, it is
> > necessary
> > to clone() the instance, to call cancel() for the older instance, and to
call
> > scan() for the new instance. It is done in DirectoryModel.
> > 
> > It should be clean up, but this CL is already huge, so I'd like to split it
> from
> > this CL. WDYT?
> > 
> 
> Can you please add either (1) a TODO line or (2): if (this.scanner_) throw new
> Error('Scanner already exists.')? Thanks!

Done.

[mtomasz, 2013-10-01 05:39:37]

lgtm with a nit

https://codereview.chromium.org/25278002/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/directory_contents.js (right):

https://codereview.chromium.org/25278002/diff/10001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/directory_contents.js:707: // TODO:
this scan method must be called at most once. Remove such a
nit: We use syntax: TODO(login): Sentence from upper case.
Feel free to add me, if you will not have time to fix this.

[hidehiko, 2013-10-01 05:54:03]

Thank you for your review.

https://codereview.chromium.org/25278002/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/directory_contents.js (right):

https://codereview.chromium.org/25278002/diff/10001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/directory_contents.js:707: // TODO:
this scan method must be called at most once. Remove such a
On 2013/10/01 05:39:38, mtomasz wrote:
> nit: We use syntax: TODO(login): Sentence from upper case.
> Feel free to add me, if you will not have time to fix this.

Thank you for your offer. Added your and my names.

[mtomasz, 2013-10-01 06:37:05]

lgtm

[commit-bot: I haz the power, 2013-10-02 03:25:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/25278002/25001

[commit-bot: I haz the power, 2013-10-02 07:41:29]

Change committed as 226420