Add support for persistent file access in apps.

apps/saved_files_service.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "apps/saved_files_service.h"
+
+#include <algorithm>
+
+#include "apps/saved_files_service_factory.h"
+#include "base/basictypes.h"
+#include "base/hash_tables.h"
+#include "base/value_conversions.h"
+#include "chrome/browser/extensions/extension_host.h"
+#include "chrome/browser/extensions/extension_prefs.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "chrome/common/extensions/permissions/api_permission.h"
+#include "chrome/common/extensions/permissions/permission_set.h"
+
+namespace apps {
+
+using extensions::APIPermission;
+using extensions::Extension;
+using extensions::ExtensionHost;
+using extensions::ExtensionPrefs;
+
+namespace {
+
+// Preference keys
+
+// The file entries that an extension has permission to access.
+const char kFileEntries[] = "file_entries";
+
+// The path to a file entry that an extension had permission to access.
+const char kFileEntryPath[] = "path";
+
+// Whether or not an extension had write access to a file entry.
+const char kFileEntryWritable[] = "writable";
+
+// The sequence number in the LRU of the file entry.
+const char kFileEntrySequenceNumber[] = "sequence_number";
+
+const size_t kMaxSavedFileEntries = 500;
+const int kMaxSequenceNumber = kint32max;
+
+// These might be different to the constant values in tests.
+size_t g_max_saved_file_entries = kMaxSavedFileEntries;
+int g_max_sequence_number = kMaxSequenceNumber;
+
+void AddSavedFileEntry(ExtensionPrefs* prefs,

[Matt Giuca, 2013/05/22 08:26:14]

Nit: One-line comment on each of these.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+                       const std::string& extension_id,
+                       const SavedFileEntry& file_entry) {
+  ExtensionPrefs::ScopedDictionaryUpdate update(
+      prefs, extension_id, kFileEntries);
+  DictionaryValue* file_entries = update.Get();
+  if (!file_entries)
+    file_entries = update.Create();
+  DCHECK(!file_entries->GetDictionaryWithoutPathExpansion(file_entry.id, NULL));
+
+  DictionaryValue* file_entry_dict = new DictionaryValue();
+  file_entry_dict->Set(kFileEntryPath, CreateFilePathValue(file_entry.path));
+  file_entry_dict->SetBoolean(kFileEntryWritable, file_entry.writable);
+  file_entry_dict->SetInteger(kFileEntrySequenceNumber,
+                              file_entry.sequence_number);
+  file_entries->SetWithoutPathExpansion(file_entry.id, file_entry_dict);
+}
+
+void UpdateSavedFileEntry(ExtensionPrefs* prefs,
+                          const std::string& extension_id,
+                          const SavedFileEntry& file_entry) {
+  ExtensionPrefs::ScopedDictionaryUpdate update(
+      prefs, extension_id, kFileEntries);
+  DictionaryValue* file_entries = update.Get();
+  DCHECK(file_entries);
+  DictionaryValue* file_entry_dict = NULL;
+  file_entries->GetDictionaryWithoutPathExpansion(file_entry.id,
+                                                  &file_entry_dict);
+  DCHECK(file_entry_dict);
+  file_entry_dict->SetInteger(kFileEntrySequenceNumber,
+                              file_entry.sequence_number);
+}
+
+void RemoveSavedFileEntry(ExtensionPrefs* prefs,
+                          const std::string& extension_id,
+                          const std::string& file_entry_id) {
+  ExtensionPrefs::ScopedDictionaryUpdate update(
+      prefs, extension_id, kFileEntries);
+  DictionaryValue* file_entries = update.Get();
+  if (!file_entries)
+    file_entries = update.Create();
+  file_entries->RemoveWithoutPathExpansion(file_entry_id, NULL);
+}
+
+void ClearSavedFileEntries(ExtensionPrefs* prefs,
+                           const std::string& extension_id) {
+  prefs->UpdateExtensionPref(extension_id, kFileEntries, NULL);
+}
+
+void GetSavedFileEntries(ExtensionPrefs* prefs,
+                         const std::string& extension_id,
+                         std::vector<SavedFileEntry>* out) {
+  const DictionaryValue* file_entries = NULL;
+  if (!prefs->ReadPrefAsDictionary(extension_id, kFileEntries, &file_entries))
+    return;
+
+  for (DictionaryValue::Iterator it(*file_entries); !it.IsAtEnd();
+       it.Advance()) {
+    const DictionaryValue* file_entry = NULL;
+    if (!it.value().GetAsDictionary(&file_entry))
+      continue;
+    const base::Value* path_value;
+    if (!file_entry->Get(kFileEntryPath, &path_value))
+      continue;
+    base::FilePath file_path;
+    if (!GetValueAsFilePath(*path_value, &file_path))
+      continue;
+    bool writable = false;
+    if (!file_entry->GetBoolean(kFileEntryWritable, &writable))
+      continue;
+    int sequence_number = 0;
+    if (!file_entry->GetInteger(kFileEntrySequenceNumber, &sequence_number))
+      continue;
+    if (!sequence_number)
+      continue;
+    out->push_back(
+        SavedFileEntry(it.key(), file_path, writable, sequence_number));
+  }
+}
+
+}  // namespace
+
+class SavedFilesService::SavedFiles {
+ public:
+  SavedFiles(Profile* profile, const std::string& extension_id);
+  ~SavedFiles();
+
+  void RetainFileUntilAppSuspend(const std::string& id,
+                                 const base::FilePath& file_path,
+                                 bool writable);
+  void MoveEntryToBackOfQueue(const std::string& id);
+  bool IsRetained(const std::string& id) const;
+  bool GetFileEntry(const std::string& id, SavedFileEntry* out) const;
+  std::vector<SavedFileEntry> GetAllFileEntries() const;
+
+ private:
+  void MaybeCompactSequenceNumbers();

[Matt Giuca, 2013/05/22 08:26:14]

Document this. In particular, what is the "Maybe" condition?

(The key thing to capture is that it is safe, from a performance perspective, to
call this every time you do anything, because it will only do any real work if
the universe is about to end.)

[Sam McNally, 2013/05/23 03:47:28]

Done.

+
+  Profile* profile_;
+  const std::string extension_id_;
+
+  // Owns values.
+  base::hash_map<std::string, SavedFileEntry*> file_id_to_file_entry_map_;

[Matt Giuca, 2013/05/22 08:26:14]

What does it mean to be in this map? It seems to mean that you will be retained
until app quit but not persistently.

+  STLValueDeleter<base::hash_map<std::string, SavedFileEntry*> >
+      file_id_to_file_entry_map_deleter_;

[Matt Giuca, 2013/05/22 08:26:14]

Hmm, so I see you manually deleting these later on. Does the STLValueDeleter
only delete the values upon its own destruction? That doesn't give anywhere near
the abstraction of boost::ptr_map
(http://www.boost.org/doc/libs/1_53_0/libs/ptr_container/doc/ptr_map.html),
which would delete values whenever they are removed from the map.

I *suppose* this is the best we can do with the current infrastructure, but it
might be worth looking around a bit more to see if there is any equivalent of
boost::ptr_map. It's pretty bad that there is no infrastructure to do this
automatically.

+
+  // Values are a subset of values in file_id_to_file_entry_map_.

[Matt Giuca, 2013/05/22 08:26:14]

What are the keys? What does it mean to be in this map? Say that the keys are
sequence numbers (the int mapping onto a SavedFileEntry is its own
sequence_number), such that the file entries are ordered with the least-recently
used entry at the (back|front).

It's also worth noting that this list should be kept in sync with the extension
prefs. (If that's the case, I'm not sure how in-sync they are; you should
document it here.)

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  std::map<int, SavedFileEntry*> saved_file_lru_;
+
+  DISALLOW_COPY_AND_ASSIGN(SavedFiles);
+};
+
+// static
+SavedFilesService* SavedFilesService::Get(Profile* profile) {
+  return SavedFilesServiceFactory::GetForProfile(profile);
+}
+
+SavedFilesService::SavedFilesService(Profile* profile)
+    : extension_id_to_saved_files_deleter_(&extension_id_to_saved_files_),
+      profile_(profile) {
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+                 content::NotificationService::AllSources());
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_APP_TERMINATING,
+                 content::NotificationService::AllSources());
+}
+
+SavedFilesService::~SavedFilesService() {}
+
+void SavedFilesService::Observe(int type,
+                                const content::NotificationSource& source,
+                                const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED: {
+      ExtensionHost* host = content::Details<ExtensionHost>(details).ptr();
+      const Extension* extension = host->extension();
+      if (extension) {
+        MaybeClearQueue(extension);
+        Clear(extension->id());
+      }
+      break;
+    }
+
+    case chrome::NOTIFICATION_APP_TERMINATING: {
+      // Stop listening to NOTIFICATION_EXTENSION_HOST_DESTROYED in particular
+      // as all extension hosts will be destroyed as a result of shutdown.
+      registrar_.RemoveAll();
+      break;
+    }
+  }
+}
+
+void SavedFilesService::RetainFileUntilAppSuspend(
+    const std::string& extension_id,
+    const std::string& id,
+    const base::FilePath& file_path,
+    bool writable) {
+  GetOrInsert(extension_id)->RetainFileUntilAppSuspend(id, file_path, writable);
+}
+
+void SavedFilesService::MoveEntryToBackOfQueue(const std::string& extension_id,
+                                               const std::string& id) {
+  GetOrInsert(extension_id)->MoveEntryToBackOfQueue(id);
+}
+
+std::vector<SavedFileEntry> SavedFilesService::GetAllFileEntries(
+    const std::string& extension_id) {
+  return GetOrInsert(extension_id)->GetAllFileEntries();
+}
+
+bool SavedFilesService::IsRetained(const std::string& extension_id,
+                                   const std::string& id) {
+  return GetOrInsert(extension_id)->IsRetained(id);
+}
+
+bool SavedFilesService::GetFileEntry(const std::string& extension_id,
+                                     const std::string& id,
+                                     SavedFileEntry* out) {
+  return GetOrInsert(extension_id)->GetFileEntry(id, out);
+}
+
+void SavedFilesService::MaybeClearQueue(const Extension* extension) {
+  if (!extension->GetActivePermissions()->HasAPIPermission(
+      APIPermission::kFileSystemRetainFiles)) {
+    ClearSavedFileEntries(ExtensionPrefs::Get(profile_), extension->id());
+    Clear(extension->id());
+  }
+}
+
+SavedFilesService::SavedFiles* SavedFilesService::GetOrInsert(
+    const std::string& extension_id) {
+  std::map<std::string, SavedFiles*>::iterator it =
+      extension_id_to_saved_files_.find(extension_id);
+  if (it != extension_id_to_saved_files_.end())
+    return it->second;
+
+  SavedFiles* saved_files = new SavedFiles(profile_, extension_id);
+  extension_id_to_saved_files_.insert(
+      std::make_pair(extension_id, saved_files));
+  return saved_files;
+}
+
+void SavedFilesService::Clear(const std::string& extension_id) {
+  std::map<std::string, SavedFiles*>::iterator it =
+      extension_id_to_saved_files_.find(extension_id);
+  if (it != extension_id_to_saved_files_.end()) {
+    delete it->second;
+    extension_id_to_saved_files_.erase(it);
+  }
+}
+
+SavedFilesService::SavedFiles::SavedFiles(Profile* profile,
+                                          const std::string& extension_id)
+    : profile_(profile),
+      extension_id_(extension_id),
+      file_id_to_file_entry_map_deleter_(&file_id_to_file_entry_map_) {
+  std::vector<SavedFileEntry> saved_entries;
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(profile_);
+  GetSavedFileEntries(prefs, extension_id_, &saved_entries);
+  for (std::vector<SavedFileEntry>::iterator it = saved_entries.begin();
+       it != saved_entries.end(); ++it) {
+    SavedFileEntry* file_entry = new SavedFileEntry(*it);
+    file_id_to_file_entry_map_.insert(
+        std::make_pair(file_entry->id, file_entry));
+    saved_file_lru_.insert(
+        std::make_pair(file_entry->sequence_number, file_entry));
+  }
+}
+
+SavedFilesService::SavedFiles::~SavedFiles() {}
+
+void SavedFilesService::SavedFiles::RetainFileUntilAppSuspend(
+    const std::string& id,
+    const base::FilePath& file_path,
+    bool writable) {
+  if (ContainsKey(file_id_to_file_entry_map_, id))
+    return;
+
+  file_id_to_file_entry_map_.insert(
+      std::make_pair(id, new SavedFileEntry(id, file_path, writable, 0)));
+}
+
+void SavedFilesService::SavedFiles::MoveEntryToBackOfQueue(
+    const std::string& id) {
+  base::hash_map<std::string, SavedFileEntry*>::iterator it =
+      file_id_to_file_entry_map_.find(id);
+  if (it == file_id_to_file_entry_map_.end())
+    return;

[Matt Giuca, 2013/05/22 08:26:14]

Doesn't this mean that you will need to call RetainFileUntilAppSuspend before
calling this, or it won't work? That isn't documented in the header file.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+
+  SavedFileEntry* file_entry = it->second;
+  int old_sequence_number = file_entry->sequence_number;
+  if (!saved_file_lru_.empty()) {

[Matt Giuca, 2013/05/22 08:26:14]

// Get the sequence number after the last file entry in the LRU.

+    std::map<int, SavedFileEntry*>::reverse_iterator it =
+        saved_file_lru_.rbegin();
+    if (it->second == file_entry)
+      return;
+
+    file_entry->sequence_number = it->first + 1;
+  } else {
+    file_entry->sequence_number = 1;

[Matt Giuca, 2013/05/22 08:26:14]

Why not 0?

Oh, I think I get it ... reading into the details, the sequence number 0 is
reserved for entries that are not in the LRU. This is an important detail which
should be documented! (See comment in header file.)

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  }
+  saved_file_lru_.insert(
+      std::make_pair(file_entry->sequence_number, file_entry));
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(profile_);
+  if (old_sequence_number) {
+    saved_file_lru_.erase(old_sequence_number);
+    UpdateSavedFileEntry(prefs, extension_id_, *file_entry);
+  } else {
+    AddSavedFileEntry(prefs, extension_id_, *file_entry);
+    if (saved_file_lru_.size() > g_max_saved_file_entries) {
+      std::map<int, SavedFileEntry*>::iterator it = saved_file_lru_.begin();
+      it->second->sequence_number = 0;
+      RemoveSavedFileEntry(prefs, extension_id_, it->second->id);
+      saved_file_lru_.erase(it);
+    }
+  }
+  MaybeCompactSequenceNumbers();
+}
+
+bool SavedFilesService::SavedFiles::IsRetained(const std::string& id) const {
+  return ContainsKey(file_id_to_file_entry_map_, id);
+}
+
+bool SavedFilesService::SavedFiles::GetFileEntry(const std::string& id,

[Matt Giuca, 2013/05/22 08:26:14]

Wait, this doesn't do what its documentation describes.

"Stores the file entry with the specified |id| in |out| and returns true if the
file entry is retained or returns false."

This implies that it will always write to |out|, and separately return
true/false based on whether the file is in the LRU. But no, if the file is in
the entry map at all, it returns true (even if not in the LRU), and if it isn't,
then it doesn't set |out| (note that the documentation does not provide for
|out| not being set).

Given that you have the return value bool simply saying whether you got a value
or not, can't you just return a pointer which might be NULL? This would change
the semantics somewhat (returning a borrowed pointer, rather than copying the
SavedFileEntry), but I was about to suggest changing those semantics anyway.

So:
// Gets a borrowed pointer to the file entry with the specified |id|. Returns
NULL if the file entry has not been retained.
// Note that this will return file entries retained until app stop, not just
those in the LRU.
const SavedFileEntry* SavedFilesService::SavedFiles::GetFileEntry(const
std::string& id) const;

[Sam McNally, 2013/05/23 03:47:28]

Done.

+                                                 SavedFileEntry* out) const {
+  base::hash_map<std::string, SavedFileEntry*>::const_iterator it =
+      file_id_to_file_entry_map_.find(id);
+  if (it == file_id_to_file_entry_map_.end())
+    return false;
+
+  *out = *it->second;
+  return true;
+}
+
+std::vector<SavedFileEntry>
+SavedFilesService::SavedFiles::GetAllFileEntries() const {
+  std::vector<SavedFileEntry> result;
+  for (base::hash_map<std::string, SavedFileEntry*>::const_iterator it =
+       file_id_to_file_entry_map_.begin();
+       it != file_id_to_file_entry_map_.end(); ++it) {
+    result.push_back(*it->second);
+  }
+  return result;
+}
+
+void SavedFilesService::SavedFiles::MaybeCompactSequenceNumbers() {

[Matt Giuca, 2013/05/22 08:26:14]

DCHECK(g_max_sequence_number >= g_max_saved_file_entries);

I'm not sure what would happen if this condition is violated, but in any case,
it should always be true.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+  std::map<int, SavedFileEntry*>::reverse_iterator it =
+      saved_file_lru_.rbegin();
+  if (it == saved_file_lru_.rend())
+    return;
+
+  if (it->first < g_max_sequence_number)

[Matt Giuca, 2013/05/22 08:26:14]

// Only compact sequence numbers if the last entry's sequence number is the
maximum value.
// This should almost never be the case.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+    return;
+
+  int sequence_number = 0;
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(profile_);
+  for (std::map<int, SavedFileEntry*>::iterator it = saved_file_lru_.begin();
+       it != saved_file_lru_.end(); ++it) {
+    sequence_number++;

[Matt Giuca, 2013/05/22 08:26:14]

Nit: ++sequence_number (style)

+    if (it->second->sequence_number == sequence_number)
+      continue;
+
+    SavedFileEntry* file_entry = it->second;
+    file_entry->sequence_number = sequence_number;
+    UpdateSavedFileEntry(prefs, extension_id_, *file_entry);
+    if (it == saved_file_lru_.begin()) {

[Matt Giuca, 2013/05/22 08:26:14]

You don't (really) need this special case. The overload of insert() that takes
an iterator is just treating the iterator as a hint, so you can use that form
regardless.

See my later comment for a proposed new version.

[Sam McNally, 2013/05/23 03:47:28]

Done.

+      saved_file_lru_.erase(it);
+      it = saved_file_lru_.insert(std::make_pair(file_entry->sequence_number,
+                                                 file_entry)).first;
+    } else {
+      saved_file_lru_.erase(it--);

[Matt Giuca, 2013/05/22 08:26:14]

This relies on undefined behaviour.
"Iterators ... referring to elements removed by the function are invalidated."
You can't assume that the iterator passed to operator-- is valid.

std::map<int, SavedFileEntry*>::iterator insert_hint = it;
// Hint to insert from the element before it, if possible.
if (insert_hint != saved_file_lru_.begin())
  --insert_hint;
saved_file_lru_.erase(it);
it = saved_file_lru_.insert(insert_hint, ...)

This would remove the need for the first branch of this if statement.

[Sam McNally, 2013/05/23 03:47:28]

Switched to hint following.

+      it = saved_file_lru_.insert(
+          it, std::make_pair(file_entry->sequence_number, file_entry));
+    }
+  }
+}
+
+// static
+void SavedFilesService::SetMaxSequenceNumberForTest(int max_value) {
+  g_max_sequence_number = max_value;
+}
+
+// static
+void SavedFilesService::ClearMaxSequenceNumberForTest() {
+  g_max_sequence_number = kMaxSequenceNumber;
+}
+
+// static
+void SavedFilesService::SetLruSizeForTest(int size) {
+  g_max_saved_file_entries = size;
+}
+
+// static
+void SavedFilesService::ClearLruSizeForTest() {
+  g_max_saved_file_entries = kMaxSavedFileEntries;
+}
+
+}  // namespace apps