Commit 70fb5591 authored by mtomasz's avatar mtomasz Committed by Commit bot

[fsp] Allow to create multiple observers for a directory, up to one per origin.

This patch allows to create multiple subscribers for observed entry change
notifications, up to one per origin for the same entry. Eg. multiple extensions
may want to observe the same directory.

Also, the persistent argument is introduced. Persistent subscriber mean, that the
origin is watching the entry persistently, despite reboots. It will be used by
cache only.

Extensions should never use persistent origins. Such subscribers will be removed
on reboot.

TEST=unit_tests: *FileSystemProvider*Registry*,
     *FileSystemProvider*ProvidedFileSystem*
BUG=248427

Review URL: https://codereview.chromium.org/642023003

Cr-Commit-Position: refs/heads/master@{#301586}
parent f183f3ad
......@@ -323,14 +323,17 @@ ProvidedFileSystemInterface::AbortCallback FakeProvidedFileSystem::WriteFile(
ProvidedFileSystemInterface::AbortCallback
FakeProvidedFileSystem::ObserveDirectory(
const GURL& origin,
const base::FilePath& directory_path,
bool recursive,
bool persistent,
const storage::AsyncFileUtil::StatusCallback& callback) {
// TODO(mtomasz): Implement it once needed.
return PostAbortableTask(base::Bind(callback, base::File::FILE_OK));
}
void FakeProvidedFileSystem::UnobserveEntry(
const GURL& origin,
const base::FilePath& entry_path,
bool recursive,
const storage::AsyncFileUtil::StatusCallback& callback) {
......
......@@ -18,6 +18,7 @@
#include "chrome/browser/chromeos/file_system_provider/provided_file_system_info.h"
#include "chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
#include "chrome/browser/chromeos/file_system_provider/provided_file_system_observer.h"
#include "url/gurl.h"
class Profile;
......@@ -126,10 +127,13 @@ class FakeProvidedFileSystem : public ProvidedFileSystemInterface {
int length,
const storage::AsyncFileUtil::StatusCallback& callback) override;
virtual AbortCallback ObserveDirectory(
const GURL& origin,
const base::FilePath& directory_path,
bool recursive,
bool persistent,
const storage::AsyncFileUtil::StatusCallback& callback) override;
virtual void UnobserveEntry(
const GURL& origin,
const base::FilePath& entry_path,
bool recursive,
const storage::AsyncFileUtil::StatusCallback& callback) override;
......
......@@ -22,6 +22,12 @@ bool ObservedEntryKey::Comparator::operator()(const ObservedEntryKey& a,
return a.recursive < b.recursive;
}
Subscriber::Subscriber() : persistent(false) {
}
Subscriber::~Subscriber() {
}
ObservedEntry::ObservedEntry() : recursive(false) {
}
......
......@@ -6,14 +6,17 @@
#define CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_OBSERVED_ENTRY_H_
#include <map>
#include <set>
#include <string>
#include "base/files/file_path.h"
#include "url/gurl.h"
namespace chromeos {
namespace file_system_provider {
struct ObservedEntry;
struct Subscriber;
// Key for storing an observed entry in the map. There may be two observers
// per path, as long as one is recursive, and the other one not.
......@@ -33,13 +36,38 @@ struct ObservedEntryKey {
typedef std::map<ObservedEntryKey, ObservedEntry, ObservedEntryKey::Comparator>
ObservedEntries;
// Map of subscribers for notifications about an observed entry.
typedef std::map<GURL, Subscriber> Subscribers;
// Represents a subscriber for notification about an observed entry. There may
// be up to one subscriber per origin for the same observed entry.
struct Subscriber {
Subscriber();
~Subscriber();
// Origin of the subscriber.
GURL origin;
// Whether the subscriber should be restored after shutdown or not.
bool persistent;
};
// Represents an observed entry on a file system.
struct ObservedEntry {
ObservedEntry();
~ObservedEntry();
// Map of subscribers for notifications of the observed entry.
Subscribers subscribers;
// Path of the observed entry.
base::FilePath entry_path;
// Whether observing is recursive or not.
bool recursive;
// Tag of the last notification for this observed entry. May be empty if not
// supported.
std::string last_tag;
};
......
......@@ -42,6 +42,13 @@ namespace {
void EmptyStatusCallback(base::File::Error /* result */) {
}
// Discards the error code and always calls the callback with a success.
void AlwaysSuccessCallback(
const storage::AsyncFileUtil::StatusCallback& callback,
base::File::Error /* result */) {
callback.Run(base::File::FILE_OK);
}
} // namespace
AutoUpdater::AutoUpdater(const base::Closure& update_callback)
......@@ -358,16 +365,35 @@ ProvidedFileSystem::AbortCallback ProvidedFileSystem::Truncate(
}
ProvidedFileSystem::AbortCallback ProvidedFileSystem::ObserveDirectory(
const GURL& origin,
const base::FilePath& directory_path,
bool recursive,
bool persistent,
const storage::AsyncFileUtil::StatusCallback& callback) {
// TODO(mtomasz): Wrap the entire method body with an asynchronous queue to
// avoid races.
if (persistent && !file_system_info_.supports_notify_tag()) {
OnObserveDirectoryCompleted(origin,
directory_path,
recursive,
persistent,
callback,
base::File::FILE_ERROR_INVALID_OPERATION);
return AbortCallback();
}
const ObservedEntryKey key(directory_path, recursive);
const ObservedEntries::const_iterator it = observed_entries_.find(key);
if (it != observed_entries_.end()) {
const bool exists =
it->second.subscribers.find(origin) != it->second.subscribers.end();
OnObserveDirectoryCompleted(
directory_path, recursive, callback, base::File::FILE_ERROR_EXISTS);
origin,
directory_path,
recursive,
persistent,
callback,
exists ? base::File::FILE_ERROR_EXISTS : base::File::FILE_OK);
return AbortCallback();
}
......@@ -381,11 +407,19 @@ ProvidedFileSystem::AbortCallback ProvidedFileSystem::ObserveDirectory(
recursive,
base::Bind(&ProvidedFileSystem::OnObserveDirectoryCompleted,
weak_ptr_factory_.GetWeakPtr(),
origin,
directory_path,
recursive,
persistent,
callback))));
if (!request_id) {
callback.Run(base::File::FILE_ERROR_SECURITY);
OnObserveDirectoryCompleted(origin,
directory_path,
recursive,
persistent,
callback,
base::File::FILE_ERROR_SECURITY);
return AbortCallback();
}
......@@ -394,38 +428,51 @@ ProvidedFileSystem::AbortCallback ProvidedFileSystem::ObserveDirectory(
}
void ProvidedFileSystem::UnobserveEntry(
const GURL& origin,
const base::FilePath& entry_path,
bool recursive,
const storage::AsyncFileUtil::StatusCallback& callback) {
const ObservedEntryKey key(entry_path, recursive);
const ObservedEntries::const_iterator it = observed_entries_.find(key);
if (it == observed_entries_.end()) {
const ObservedEntries::iterator it = observed_entries_.find(key);
if (it == observed_entries_.end() ||
it->second.subscribers.find(origin) == it->second.subscribers.end()) {
callback.Run(base::File::FILE_ERROR_NOT_FOUND);
return;
}
// Delete the watcher in advance since the list of observed entries is owned
// by the C++ layer, not by the extension.
observed_entries_.erase(it);
// Delete the subscriber in advance, since the list of observed entries is
// owned by the C++ layer, not by the extension.
it->second.subscribers.erase(origin);
FOR_EACH_OBSERVER(
ProvidedFileSystemObserver,
observers_,
OnObservedEntryListChanged(file_system_info_, observed_entries_));
// TODO(mtomasz): Consider returning always an OK error code, since for the
// callers it's important that the entry is not watched anymore. The watcher
// is removed even if the extension returns an error.
// If there are other subscribers, then do not remove the obsererver, but
// simply return a success.
if (it->second.subscribers.size()) {
callback.Run(base::File::FILE_OK);
return;
}
// Delete the watcher in advance.
observed_entries_.erase(it);
// Even if the extension returns an error, the callback is called with base::
// File::FILE_OK. The reason for that is that the observed is not watched
// anymore anyway, as it's removed in advance.
const int request_id = request_manager_->CreateRequest(
UNOBSERVE_ENTRY,
scoped_ptr<RequestManager::HandlerInterface>(
new operations::UnobserveEntry(event_router_,
file_system_info_,
entry_path,
recursive,
callback)));
new operations::UnobserveEntry(
event_router_,
file_system_info_,
entry_path,
recursive,
base::Bind(&AlwaysSuccessCallback, callback))));
if (!request_id)
callback.Run(base::File::FILE_ERROR_SECURITY);
callback.Run(base::File::FILE_OK);
}
const ProvidedFileSystemInfo& ProvidedFileSystem::GetFileSystemInfo() const {
......@@ -512,8 +559,10 @@ void ProvidedFileSystem::Abort(
}
void ProvidedFileSystem::OnObserveDirectoryCompleted(
const GURL& origin,
const base::FilePath& directory_path,
bool recursive,
bool persistent,
const storage::AsyncFileUtil::StatusCallback& callback,
base::File::Error result) {
if (result != base::File::FILE_OK) {
......@@ -528,8 +577,11 @@ void ProvidedFileSystem::OnObserveDirectoryCompleted(
return;
}
observed_entries_[key].entry_path = directory_path;
observed_entries_[key].recursive = recursive;
ObservedEntry* const observed_entry = &observed_entries_[key];
observed_entry->entry_path = directory_path;
observed_entry->recursive = recursive;
observed_entry->subscribers[origin].origin = origin;
observed_entry->subscribers[origin].persistent |= persistent;
FOR_EACH_OBSERVER(
ProvidedFileSystemObserver,
......@@ -569,8 +621,17 @@ void ProvidedFileSystem::OnNotifyCompleted(
OnObservedEntryTagUpdated(file_system_info_, it->second));
// If the observed entry is deleted, then unobserve it.
if (change_type == ProvidedFileSystemObserver::DELETED)
UnobserveEntry(observed_path, recursive, base::Bind(&EmptyStatusCallback));
if (change_type == ProvidedFileSystemObserver::DELETED) {
// Make a copy, since the |it| iterator will get invalidated on the last
// subscriber.
Subscribers subscribers = it->second.subscribers;
for (const auto& subscriber_it : subscribers) {
UnobserveEntry(subscriber_it.second.origin,
observed_path,
recursive,
base::Bind(&EmptyStatusCallback));
}
}
}
} // namespace file_system_provider
......
......@@ -16,6 +16,7 @@
#include "chrome/browser/chromeos/file_system_provider/provided_file_system_observer.h"
#include "chrome/browser/chromeos/file_system_provider/request_manager.h"
#include "storage/browser/fileapi/async_file_util.h"
#include "url/gurl.h"
class Profile;
......@@ -137,10 +138,13 @@ class ProvidedFileSystem : public ProvidedFileSystemInterface {
int length,
const storage::AsyncFileUtil::StatusCallback& callback) override;
virtual AbortCallback ObserveDirectory(
const GURL& origin,
const base::FilePath& directory_path,
bool recursive,
bool persistent,
const storage::AsyncFileUtil::StatusCallback& callback) override;
virtual void UnobserveEntry(
const GURL& origin,
const base::FilePath& entry_path,
bool recursive,
const storage::AsyncFileUtil::StatusCallback& callback) override;
......@@ -163,10 +167,13 @@ class ProvidedFileSystem : public ProvidedFileSystemInterface {
void Abort(int operation_request_id,
const storage::AsyncFileUtil::StatusCallback& callback);
// Called when a directory becomes watched successfully.
// Called when observing a directory process is completed with either success
// or en error.
void OnObserveDirectoryCompleted(
const GURL& origin,
const base::FilePath& directory_path,
bool recursive,
bool persistent,
const storage::AsyncFileUtil::StatusCallback& callback,
base::File::Error result);
......
......@@ -17,6 +17,7 @@
#include "chrome/browser/chromeos/file_system_provider/observed_entry.h"
#include "chrome/browser/chromeos/file_system_provider/provided_file_system_observer.h"
#include "storage/browser/fileapi/async_file_util.h"
#include "url/gurl.h"
class EventRouter;
......@@ -177,13 +178,16 @@ class ProvidedFileSystemInterface {
// Requests observing a directory.
virtual AbortCallback ObserveDirectory(
const GURL& origin,
const base::FilePath& directory_path,
bool recursive,
bool persistent,
const storage::AsyncFileUtil::StatusCallback& callback) = 0;
// Requests unobserving an entry, which is immediately removed from the
// internal list, hence the operation is not abortable.
virtual void UnobserveEntry(
const GURL& origin,
const base::FilePath& entry_path,
bool recursive,
const storage::AsyncFileUtil::StatusCallback& callback) = 0;
......
......@@ -31,6 +31,7 @@ const char kPrefKeyObservedEntries[] = "observed-entries";
const char kPrefKeyObservedEntryEntryPath[] = "entry-path";
const char kPrefKeyObservedEntryRecursive[] = "recursive";
const char kPrefKeyObservedEntryLastTag[] = "last-tag";
const char kPrefKeyObservedEntryPersistentOrigins[] = "persistent-origins";
void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
registry->RegisterDictionaryPref(
......@@ -72,6 +73,15 @@ void Registry::RememberFileSystem(
kPrefKeyObservedEntryRecursive, it.second.recursive);
observed_entry->SetStringWithoutPathExpansion(kPrefKeyObservedEntryLastTag,
it.second.last_tag);
base::ListValue* const persistent_origins_value = new base::ListValue();
observed_entry->SetWithoutPathExpansion(
kPrefKeyObservedEntryPersistentOrigins, persistent_origins_value);
for (const auto& subscriber_it : it.second.subscribers) {
// Only persistent subscribers should be stored in persistent storage.
// Other ones should not be restired after a restart.
if (subscriber_it.second.persistent)
persistent_origins_value->AppendString(subscriber_it.first.spec());
}
}
PrefService* const pref_service = profile_->GetPrefs();
......@@ -182,6 +192,7 @@ scoped_ptr<Registry::RestoredFileSystems> Registry::RestoreFileSystems(
std::string entry_path;
bool recursive = false;
std::string last_tag;
const base::ListValue* persistent_origins = NULL;
if (!observed_entry_value->GetAsDictionary(&observed_entry) ||
!observed_entry->GetStringWithoutPathExpansion(
......@@ -190,8 +201,11 @@ scoped_ptr<Registry::RestoredFileSystems> Registry::RestoreFileSystems(
kPrefKeyObservedEntryRecursive, &recursive) ||
!observed_entry->GetStringWithoutPathExpansion(
kPrefKeyObservedEntryLastTag, &last_tag) ||
!observed_entry->GetListWithoutPathExpansion(
kPrefKeyObservedEntryPersistentOrigins, &persistent_origins) ||
it.key() != entry_path || entry_path.empty() ||
(!options.supports_notify_tag && !last_tag.empty())) {
(!options.supports_notify_tag &&
(!last_tag.empty() || persistent_origins->GetSize()))) {
LOG(ERROR) << "Malformed observed entry information in preferences.";
continue;
}
......@@ -201,6 +215,17 @@ scoped_ptr<Registry::RestoredFileSystems> Registry::RestoreFileSystems(
base::FilePath::FromUTF8Unsafe(entry_path);
restored_observed_entry.recursive = recursive;
restored_observed_entry.last_tag = last_tag;
for (const auto& persistent_origin : *persistent_origins) {
std::string origin;
if (persistent_origin->GetAsString(&origin)) {
LOG(ERROR) << "Malformed subscriber information in preferences.";
continue;
}
const GURL origin_as_gurl(origin);
restored_observed_entry.subscribers[origin_as_gurl].origin =
origin_as_gurl;
restored_observed_entry.subscribers[origin_as_gurl].persistent = true;
}
restored_file_system.observed_entries[ObservedEntryKey(
base::FilePath::FromUTF8Unsafe(entry_path), recursive)] =
restored_observed_entry;
......
......@@ -28,6 +28,7 @@ extern const char kPrefKeySupportsNotifyTag[];
extern const char kPrefKeyObservedEntries[];
extern const char kPrefKeyObservedEntryEntryPath[];
extern const char kPrefKeyObservedEntryRecursive[];
extern const char kPrefKeyObservedEntryPersistentOrigins[];
extern const char kPrefKeyObservedEntryLastTag[];
class ProvidedFileSystemInfo;
......
......@@ -10,7 +10,6 @@
#include "base/files/file_path.h"
#include "base/memory/scoped_ptr.h"
#include "chrome/browser/chromeos/file_system_provider/provided_file_system_info.h"
#include "chrome/browser/chromeos/file_system_provider/registry.h"
#include "chrome/common/pref_names.h"
#include "chrome/test/base/testing_browser_process.h"
#include "chrome/test/base/testing_pref_service_syncable.h"
......@@ -24,6 +23,10 @@ namespace chromeos {
namespace file_system_provider {
namespace {
const char kTemporaryOrigin[] =
"chrome-extension://abcabcabcabcabcabcabcabcabcabcabcabca/";
const char kPersistentOrigin[] =
"chrome-extension://efgefgefgefgefgefgefgefgefgefgefgefge/";
const char kExtensionId[] = "mbflcebpggnecokmikipoihdbecnjfoj";
const char kDisplayName[] = "Camera Pictures";
......@@ -41,6 +44,8 @@ void RememberFakeFileSystem(TestingProfile* profile,
bool writable,
bool supports_notify_tag,
const ObservedEntry& observed_entry) {
// Warning. Updating this code means that backward compatibility may be
// broken, what is unexpected and should be avoided.
TestingPrefServiceSyncable* const pref_service =
profile->GetTestingPrefService();
ASSERT_TRUE(pref_service);
......@@ -71,6 +76,13 @@ void RememberFakeFileSystem(TestingProfile* profile,
kPrefKeyObservedEntryRecursive, observed_entry.recursive);
observed_entry_value->SetStringWithoutPathExpansion(
kPrefKeyObservedEntryLastTag, observed_entry.last_tag);
base::ListValue* const persistent_origins_value = new base::ListValue();
observed_entry_value->SetWithoutPathExpansion(
kPrefKeyObservedEntryPersistentOrigins, persistent_origins_value);
for (const auto& subscriber_it : observed_entry.subscribers) {
if (subscriber_it.second.persistent)
persistent_origins_value->AppendString(subscriber_it.first.spec());
}
pref_service->Set(prefs::kFileSystemProviderMounted, extensions);
}
......@@ -92,6 +104,12 @@ class FileSystemProviderRegistryTest : public testing::Test {
fake_observed_entry_.entry_path =
base::FilePath(FILE_PATH_LITERAL("/a/b/c"));
fake_observed_entry_.recursive = true;
fake_observed_entry_.subscribers[GURL(kTemporaryOrigin)].origin =
GURL(kTemporaryOrigin);
fake_observed_entry_.subscribers[GURL(kTemporaryOrigin)].persistent = false;
fake_observed_entry_.subscribers[GURL(kPersistentOrigin)].origin =
GURL(kPersistentOrigin);
fake_observed_entry_.subscribers[GURL(kPersistentOrigin)].persistent = true;
fake_observed_entry_.last_tag = "hello-world";
}
......@@ -215,6 +233,19 @@ TEST_F(FileSystemProviderRegistryTest, RememberFileSystem) {
EXPECT_TRUE(observed_entry->GetStringWithoutPathExpansion(
kPrefKeyObservedEntryLastTag, &last_tag));
EXPECT_EQ(fake_observed_entry_.last_tag, last_tag);
const base::ListValue* persistent_origins = NULL;
ASSERT_TRUE(observed_entry->GetListWithoutPathExpansion(
kPrefKeyObservedEntryPersistentOrigins, &persistent_origins));
ASSERT_GT(fake_observed_entry_.subscribers.size(),
persistent_origins->GetSize());
ASSERT_EQ(1u, persistent_origins->GetSize());
std::string persistent_origin;
EXPECT_TRUE(persistent_origins->GetString(0, &persistent_origin));
const auto& fake_subscriber_it =
fake_observed_entry_.subscribers.find(GURL(persistent_origin));
ASSERT_NE(fake_observed_entry_.subscribers.end(), fake_subscriber_it);
EXPECT_TRUE(fake_subscriber_it->second.persistent);
}
TEST_F(FileSystemProviderRegistryTest, ForgetFileSystem) {
......@@ -279,26 +310,6 @@ TEST_F(FileSystemProviderRegistryTest, UpdateObservedEntryTag) {
file_systems->GetWithoutPathExpansion(kFileSystemId, &file_system_value));
ASSERT_TRUE(file_system_value->GetAsDictionary(&file_system));
std::string file_system_id;
EXPECT_TRUE(file_system->GetStringWithoutPathExpansion(kPrefKeyFileSystemId,
&file_system_id));
EXPECT_EQ(kFileSystemId, file_system_id);
std::string display_name;
EXPECT_TRUE(file_system->GetStringWithoutPathExpansion(kPrefKeyDisplayName,
&display_name));
EXPECT_EQ(kDisplayName, display_name);
bool writable = false;
EXPECT_TRUE(
file_system->GetBooleanWithoutPathExpansion(kPrefKeyWritable, &writable));
EXPECT_TRUE(writable);
bool supports_notify_tag = false;
EXPECT_TRUE(file_system->GetBooleanWithoutPathExpansion(
kPrefKeySupportsNotifyTag, &supports_notify_tag));
EXPECT_TRUE(supports_notify_tag);
const base::DictionaryValue* observed_entries_value = NULL;
ASSERT_TRUE(file_system->GetDictionaryWithoutPathExpansion(
kPrefKeyObservedEntries, &observed_entries_value));
......@@ -307,16 +318,6 @@ TEST_F(FileSystemProviderRegistryTest, UpdateObservedEntryTag) {
ASSERT_TRUE(observed_entries_value->GetDictionaryWithoutPathExpansion(
fake_observed_entry_.entry_path.value(), &observed_entry));
std::string entry_path;
EXPECT_TRUE(observed_entry->GetStringWithoutPathExpansion(
kPrefKeyObservedEntryEntryPath, &entry_path));
EXPECT_EQ(fake_observed_entry_.entry_path.value(), entry_path);
bool recursive = false;
EXPECT_TRUE(observed_entry->GetBooleanWithoutPathExpansion(
kPrefKeyObservedEntryRecursive, &recursive));
EXPECT_EQ(fake_observed_entry_.recursive, recursive);
std::string last_tag;
EXPECT_TRUE(observed_entry->GetStringWithoutPathExpansion(
kPrefKeyObservedEntryLastTag, &last_tag));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment