[fsp] Add support for observing entries and notifying about changes.

[fsp] Add support for observing entries and notifying about changes.

This patch adds:
1. Flag supports_notify_tag to chrome.fsp.mount().
2. Method chrome.fsp.Notify() to notify about changes of observed entries.
3. Events chrome.fsp.OnObserveDirectory and OnUnobserveEntry.

Missing things, to be added in a separate patch:
1. Glueing chrome.fsp.Notify to the C++ layer properly, not with fake data.
2. Saving and restoring observed entries together with tags from preferences.

TEST=unit_test: *FileSystemProviderProvidedFileSystem*,
     *FileSystemProvider*Observe*, *FileSystemProvider*Unobserve*
BUG=248427
Committed: https://crrev.com/9b1e24c73938eb4d7e1c631758d213e0f05525b7
Cr-Commit-Position: refs/heads/master@{#299058}

[mtomasz, 2014-10-03 04:22:04]

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

[mtomasz, 2014-10-03 04:22:48]

@hirono: PTAL.

This is a huge patch, so I added comments explaining changes. Please read them
in order of files in the patchset.

Let me know if there is anything unclear. Thanks.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:67:
params->options.supports_notify_tag)) {
If a file system supports_notify_tag is |true|, then when calling
chrome.fileSystemProvider.Notify |tag| must be provided, too.

The tag can be eg. a revision number for Drive notifications. The purpose of it
is to never miss any notifications, eg.:

1. Drive providing extension notifies about a /hoge-fuka.txt file being changed,
with a tag "123".
2. Chromebook is shutdown.
3. A lot of files are changed. No notification emited, because computer is off.
4. Chromebook is resumed.
5. Drive providing extension emits all notifications which happened since tag
"123".

Without tags, we can't provide continuous notifications when the computer is
off, or internet is off. Drive provides a changeId field, which can be used as
tag. Dropbox provides a tag too.

Note, that extensions may not provide a tag. If not, then notifications may not
arrive when the computer is off, or internet is off. Caching needs reliable
notifications about changes, in order to invalidate changed files. Hence, we
will enable caching only if supports_notify_tag is true.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:148:
if (!file_system->Notify(
This is not used yet.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.h
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.h:43:
class FileSystemProviderNotifyFunction : public ChromeSyncExtensionFunction {
Extensions call this method to notify Chrome about changes to files/directories.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:127:
virtual AbortCallback ObserveDirectory(
Currently we support observing directories only, never single files, but it will
be added as ObserveFile in the future.

I want to separate these two methods, since |recursive| doesn't make sense for
files, but only for directories.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:131:
virtual AbortCallback UnobserveEntry(
Method for unobserving files would have the same signature, hence there will be
only one method UnobserveEntry which will unobserve both directories and files.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:137:
virtual void AddObserver(ProvidedFileSystemObserver* observer) OVERRIDE;
Cache implementation will add itself as an observer. So it will be able to
invalidate entries which are changed remotely.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:139:
virtual bool Notify(
This is called from file_system_provider_api.cc, so by providing extensions.
This method simply updates the tag in the internal collection of observed
entries, and notifies observers.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/fileapi/provider_async_file_util_unittest.cc
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/provider_async_file_util_unittest.cc:143:
false /* supports_notify_tag */);
Most of changed unit test files basically just add this flag.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/operations/unobserve_entry.cc
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/unobserve_entry.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
This file is mostly a copy paste from observe_directory.cc.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:38: //
Automatically calls the |update_callback| after all of the callbacks created
This class is very important. Basically as in the comment here we want to update
the last tag in our internal map of observed entries |observed_entries| after
the upcoming Cache implementation invalidates the entries.

Eg.

0. /hoge-fuga.txt is observed, and tag of the last notification is 'tag-1'.
1. /hoge-fuga.txt is changed remotely.
2. Extension is invoking ProvidedFileSystem::Notify('/hoge-fuga.txt', 'tag-2').
3. Cache is notified about change to '/hoge-fuga.txt'
4. Cache asynchronously removes from cache the entry. It takes several
PostTasks.
5. Also other observers do other stuff.
6. AutoUpdate calls |update_callback|, which updates the tag from 'tag-1' to
'tag-2'.

Why is that? If crash happened during invalidating the cache, then after a
restart we will have the old tag in the |observed_entries_|, and the extension
will generate the notification about '/hoge-fuga.txt' again.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:171: //
Called when all observers finished handling the change notification. It
This is called when all observers in |observers_| finished handling
OnObservedEntryChanged().

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_observer.h
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_observer.h:51:
// Called when an observed entry is changed, including removals. |callback|
As described before, the passed |callback| is created using
AutoUpdate::CreateCallback. When all of such callbacks are executed, then it
means that all observers finished handling this event, and the tag will be
updated.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
The original file on the left was removed by me yesterday. Basically the entire
file is now new.

[hirono, 2014-10-03 07:19:13]

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/operations/unobserve_entry.h
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/unobserve_entry.h:47:
base::FilePath entry_path_;
Maybe const?

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:68: if
(!created_callbacks_)
After update_callback_ is called in OnPendingCallback, the destructor still
calls update_callback_ because pending_callback_ is 0. Is it OK?

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:395:
ProvidedFileSystem::AbortCallback ProvidedFileSystem::UnobserveEntry(
Should the operation be abortable?

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:37:
const char kDirectoryPath[] = "/hello/world";
char -> base::FilePath::CharType ?

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:92:
ProvidedFileSystemInterface* file_system_;  // Not owned.
const?

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:115:
ProvidedFileSystemObserver::ChangeType change_type_;
const?

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:116:
ProvidedFileSystemObserver::ChildChanges child_changes_;
const?

[hirono, 2014-10-03 07:25:33]

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:367: if
(!recursive || it->second.recursive) {
recursive = false -> recursive = true succeeds.
recursive = true -> recursive = false fails.
Is it needed to depends on order?

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:367: if
(!recursive || it->second.recursive) {
The following steps seem succeed. Should it failed?

1. ObserveDirectory recursive = true
2. ObserveDirectory recursive = false
3. OnObserveDirectoryCompleted for 1.
4. OnObserveDirectoryCompleted for 2.

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:372: 
Should we check the number of observed entries here?

[mtomasz, 2014-10-03 08:35:34]

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/operations/unobserve_entry.h
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/unobserve_entry.h:47:
base::FilePath entry_path_;
On 2014/10/03 07:19:13, hirono wrote:
> Maybe const?

Done.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:68: if
(!created_callbacks_)
On 2014/10/03 07:19:13, hirono wrote:
> After update_callback_ is called in OnPendingCallback, the destructor still
> calls update_callback_ because pending_callback_ is 0. Is it OK?

In destructor we check created_callbacks_, not pending_callbacks_. So in
destructor we will call update_callback_ only if there were no created
callbacks.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:395:
ProvidedFileSystem::AbortCallback ProvidedFileSystem::UnobserveEntry(
On 2014/10/03 07:19:13, hirono wrote:
> Should the operation be abortable?

Good point. It shouldn't. Done.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc
(right):

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:37:
const char kDirectoryPath[] = "/hello/world";
On 2014/10/03 07:19:13, hirono wrote:
> char -> base::FilePath::CharType ?

Done.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:37:
const char kDirectoryPath[] = "/hello/world";
On 2014/10/03 07:19:13, hirono wrote:
> char -> base::FilePath::CharType ?

Done.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:92:
ProvidedFileSystemInterface* file_system_;  // Not owned.
On 2014/10/03 07:19:13, hirono wrote:
> const?

Done.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:115:
ProvidedFileSystemObserver::ChangeType change_type_;
On 2014/10/03 07:19:13, hirono wrote:
> const?

Done.

https://codereview.chromium.org/625463002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:116:
ProvidedFileSystemObserver::ChildChanges child_changes_;
On 2014/10/03 07:19:13, hirono wrote:
> const?

Done.

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:367: if
(!recursive || it->second.recursive) {
On 2014/10/03 07:25:33, hirono wrote:
> The following steps seem succeed. Should it failed?
> 
> 1. ObserveDirectory recursive = true
> 2. ObserveDirectory recursive = false
> 3. OnObserveDirectoryCompleted for 1.
> 4. OnObserveDirectoryCompleted for 2.

Good point. It is not easy to solve it though. I was thinking about wrappint it
with some kind of AsyncUtil.Queue, but we don't have such thing in C++.

See comment:
https://codereview.chromium.org/625463002/diff/1/chrome/browser/chromeos/file...

I put back the TODO. WDYT?

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:367: if
(!recursive || it->second.recursive) {
On 2014/10/03 07:25:33, hirono wrote:
> recursive = false -> recursive = true succeeds.
> recursive = true -> recursive = false fails.
> Is it needed to depends on order?

Recursive = false -> recursive = true makes sense, since we increase scope of
the watcher.

The opposite is no-op. Recursive = true already includes recursive = false
notifications.

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:372: 
On 2014/10/03 07:25:33, hirono wrote:
> Should we check the number of observed entries here?

Why? Do you mean for security reasons?

[hirono, 2014-10-06 02:41:38]

lgtm!

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:367: if
(!recursive || it->second.recursive) {
On 2014/10/03 08:35:34, mtomasz wrote:
> On 2014/10/03 07:25:33, hirono wrote:
> > recursive = false -> recursive = true succeeds.
> > recursive = true -> recursive = false fails.
> > Is it needed to depends on order?
> 
> Recursive = false -> recursive = true makes sense, since we increase scope of
> the watcher.
> 
> The opposite is no-op. Recursive = true already includes recursive = false
> notifications.

SGTM

https://codereview.chromium.org/625463002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:372: 
On 2014/10/03 08:35:34, mtomasz wrote:
> On 2014/10/03 07:25:33, hirono wrote:
> > Should we check the number of observed entries here?
> 
> Why? Do you mean for security reasons?

I misunderstood. We don't have the upper limit of the watcher number.

[mtomasz, 2014-10-06 03:37:18]

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

[mtomasz, 2014-10-06 03:37:32]

@benwells: PTAL at IDL. Thanks.

[mtomasz, 2014-10-06 13:18:50]

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

[mtomasz, 2014-10-06 13:19:04]

@isherman: PTAL at histograms. Thanks.

[Ilya Sherman, 2014-10-06 21:47:21]

histograms LGTM

[benwells, 2014-10-09 04:36:44]

Just some comment nits.

https://codereview.chromium.org/625463002/diff/120001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/625463002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:315: // An ID of the
request tothis change.
nit: "tothis". not sure if this change is intentional, it seemed to make sense
before.

https://codereview.chromium.org/625463002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:450: // internal cache.
This stuff about tagging isn't very obvious (i.e. it won't be obvious to someone
trying to use this documentation to make a file system provider).

I think you should explain more or add a TODO to explain it better.

[benwells, 2014-10-09 04:37:11]

(lgtm)

[mtomasz, 2014-10-09 07:37:52]

https://codereview.chromium.org/625463002/diff/120001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/625463002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:315: // An ID of the
request tothis change.
On 2014/10/09 04:36:44, benwells wrote:
> nit: "tothis". not sure if this change is intentional, it seemed to make sense
> before.

Accidental change. Done.

https://codereview.chromium.org/625463002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:450: // internal cache.
On 2014/10/09 04:36:44, benwells wrote:
> This stuff about tagging isn't very obvious (i.e. it won't be obvious to
someone
> trying to use this documentation to make a file system provider).
> 
> I think you should explain more or add a TODO to explain it better.

Done.

[mtomasz, 2014-10-10 00:36:17]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-10-10 00:37:29]

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

[commit-bot: I haz the power, 2014-10-10 00:44:14]

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

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

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/67005)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mtomasz, 2014-10-10 03:23:49]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-10-10 03:25:05]

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

[commit-bot: I haz the power, 2014-10-10 04:39:15]

Committed patchset #9 (id:300001)

[commit-bot: I haz the power, 2014-10-10 04:39:55]

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