Files.app: Provide detailed change information on onDirectoryChanged event

Files.app: Provide detailed change information on onDirectoryChanged event

This patch adds the detailed change information into onDirectoryChanged event to Files.app. The detailed information is not handled on Files.app, hence this patch itself should not change any functionality.

BUG=131198
TEST=browser_tests/unit_tests passes

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

[yoshiki, 2014-06-20 04:08:41]

@kinaba-san, Could you take a look? Sorry for long patch. If you have any
question, please feel free to ask me. Thanks.

[kinaba, 2014-06-23 05:37:43]

General direction looks ok. Added first round of comments.

One quick question: how the change affects performance?
The only case I worry is what happens when a large change from
ChangeListProcessor (typically at the end of the initial loading) came.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_change.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_change.cc:125:
entry.file_info().is_directory() ? FILE_TYPE_DIRECTORY : FILE_TYPE_FILE;
As far as I remember, in entry.deleted() case, the type is undetermined from
|entry|. Probably you need to handle the case and assign FILE_TYPE_UNKNOWN.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system.cc:849: 
What about moving this dispatch-by-directory code to EventRouter and keep
drive::FileSystem to simply pass FileChange as is?

Since EventRouter is the place currently caring which path is watched, I guess
it will be more natural to have path-dependent screening in that layer, and
besides it'll be probably better for Bug 243634, 232994, etc.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system/copy_operation.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system/copy_operation.cc:167: ResourceEntry*
resource_entry,
ResourceEntry* entry

should be ok.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system/operation_observer.h (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system/operation_observer.h:34: virtual void
OnDirectoryChangedByOperation(
The "Directory" in the name might better be changed.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system/remove_operation.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system/remove_operation.cc:113: FileError
error2 = metadata_->GetResourceEntryById(*local_id, &entry);
this is violating thread restriction (metadata must be accessed on
blocking_task_runner_.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/sync/entry_revert_performer.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/sync/entry_revert_performer.cc:58:
FileChange::FILE_TYPE_FILE,
File type may be FILE or DIRECTORY here.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/sync/entry_revert_performer.cc:77:
FileChange::FILE_TYPE_FILE,
ditto

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/sync/entry_update_performer.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/sync/entry_update_performer.cc:128:
file_resource->file_id(), &existing_local_id);
The |error| value for this call must be checked in some way.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/sync/entry_update_performer.cc:136: if
(existing_local_id != local_id && !existing_local_id.empty()) {
and if the above |error| is properly checked, existing_local_id can be assumed
non-empty.

[yoshiki, 2014-06-23 06:20:43]

> One quick question: how the change affects performance?
> The only case I worry is what happens when a large change from
> ChangeListProcessor (typically at the end of the initial loading) came.

Good catch. I assume passing data to JS may be slow. I'm considering not to pass
the detailed data and trigger full refresh if the changelist is large. It should
solve the problem of one large changelist.

But I'm wondering about the case of the frequent multiple changelists. Do you
think we need to care about it? Is there possibility to get many changelists at
short interval?

[kinaba, 2014-06-23 23:49:56]

On 2014/06/23 06:20:43, yoshiki wrote:
> > One quick question: how the change affects performance?
> > The only case I worry is what happens when a large change from
> > ChangeListProcessor (typically at the end of the initial loading) came.
> 
> Good catch. I assume passing data to JS may be slow. I'm considering not to
pass
> the detailed data and trigger full refresh if the changelist is large. It
should
> solve the problem of one large changelist.
> 
> But I'm wondering about the case of the frequent multiple changelists. Do you
> think we need to care about it? Is there possibility to get many changelists
at
> short interval?

One possible case is when the user is shared a large folder one by one in a
short
interval, but I don't think we need to care too much. (And I believe your
approach
to squash too large change list should just work in this case as well.)

[yoshiki, 2014-06-24 02:02:23]

@kinaba, PTAL again. Thanks.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_change.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_change.cc:125:
entry.file_info().is_directory() ? FILE_TYPE_DIRECTORY : FILE_TYPE_FILE;
On 2014/06/23 05:37:43, kinaba wrote:
> As far as I remember, in entry.deleted() case, the type is undetermined from
> |entry|. Probably you need to handle the case and assign FILE_TYPE_UNKNOWN.

Done.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system.cc:849: 
On 2014/06/23 05:37:43, kinaba wrote:
> What about moving this dispatch-by-directory code to EventRouter and keep
> drive::FileSystem to simply pass FileChange as is?
> 
> Since EventRouter is the place currently caring which path is watched, I guess
> it will be more natural to have path-dependent screening in that layer, and
> besides it'll be probably better for Bug 243634, 232994, etc.

Done.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system/copy_operation.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system/copy_operation.cc:167: ResourceEntry*
resource_entry,
On 2014/06/23 05:37:43, kinaba wrote:
> ResourceEntry* entry
> 
> should be ok.

Done.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system/operation_observer.h (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system/operation_observer.h:34: virtual void
OnDirectoryChangedByOperation(
On 2014/06/23 05:37:43, kinaba wrote:
> The "Directory" in the name might better be changed.

Done.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system/remove_operation.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system/remove_operation.cc:113: FileError
error2 = metadata_->GetResourceEntryById(*local_id, &entry);
On 2014/06/23 05:37:43, kinaba wrote:
> this is violating thread restriction (metadata must be accessed on
> blocking_task_runner_.

Done.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/sync/entry_revert_performer.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/sync/entry_revert_performer.cc:58:
FileChange::FILE_TYPE_FILE,
On 2014/06/23 05:37:43, kinaba wrote:
> File type may be FILE or DIRECTORY here.

Done.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/sync/entry_revert_performer.cc:77:
FileChange::FILE_TYPE_FILE,
On 2014/06/23 05:37:43, kinaba wrote:
> ditto

Done.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/sync/entry_update_performer.cc (right):

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/sync/entry_update_performer.cc:128:
file_resource->file_id(), &existing_local_id);
On 2014/06/23 05:37:43, kinaba wrote:
> The |error| value for this call must be checked in some way.

Done.

https://codereview.chromium.org/343073003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/sync/entry_update_performer.cc:136: if
(existing_local_id != local_id && !existing_local_id.empty()) {
On 2014/06/23 05:37:43, kinaba wrote:
> and if the above |error| is properly checked, existing_local_id can be assumed
> non-empty.

Done.

[kinaba, 2014-06-24 07:24:47]

lgtm

https://codereview.chromium.org/343073003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/file_change.cc (right):

https://codereview.chromium.org/343073003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_change.cc:106: }
nit: maybe you can drop the above 4 lines. map_[file_path] is automatically set
to an empty ChangeList if it was not set.

https://codereview.chromium.org/343073003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/343073003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:97: const int
kDirectoryChangeEventMaxDetailInfoSize = 1000;
maybe: size_t

[yoshiki, 2014-06-24 14:09:29]

Thanks!

https://codereview.chromium.org/343073003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/file_change.cc (right):

https://codereview.chromium.org/343073003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_change.cc:106: }
On 2014/06/24 07:24:47, kinaba wrote:
> nit: maybe you can drop the above 4 lines. map_[file_path] is automatically
set
> to an empty ChangeList if it was not set.

Done.

https://codereview.chromium.org/343073003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/343073003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:97: const int
kDirectoryChangeEventMaxDetailInfoSize = 1000;
On 2014/06/24 07:24:47, kinaba wrote:
> maybe: size_t 

Done.

[yoshiki, 2014-06-24 14:09:33]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-06-24 14:10:42]

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

[commit-bot: I haz the power, 2014-06-24 18:12:37]

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

[commit-bot: I haz the power, 2014-06-24 18:16:15]

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

[commit-bot: I haz the power, 2014-06-24 18:16:16]

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

[yoshiki, 2014-06-25 02:41:35]

@sargent, could you take a look at the changes in file_browser_private.idl?
Thanks.

[asargent_no_longer_on_chrome, 2014-06-25 16:47:29]

file_browser_private.idl lgtm

[yoshiki, 2014-07-01 08:09:46]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-07-01 08:10:42]

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

[commit-bot: I haz the power, 2014-07-01 08:14:18]

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

[commit-bot: I haz the power, 2014-07-01 08:17:27]

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

[commit-bot: I haz the power, 2014-07-01 08:17:28]

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

[yoshiki, 2014-07-01 14:20:30]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-07-01 14:21:12]

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

[commit-bot: I haz the power, 2014-07-01 18:40:33]

Change committed as 280854