Add support for persistent file access in apps.

apps/saved_files_service.h

+// 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.
+
+#ifndef APPS_SAVED_FILES_SERVICE_H_
+#define APPS_SAVED_FILES_SERVICE_H_
+
+#include <map>
+#include <set>
+#include <string>
+#include <vector>
+
+#include "base/files/file_path.h"
+#include "base/gtest_prod_util.h"
+#include "base/stl_util.h"
+#include "components/browser_context_keyed_service/browser_context_keyed_service.h"

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

Line too long. Not sure if the style guide permits this or whether you're
supposed to wrap it.

+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+
+class Profile;
+class SavedFilesServiceUnitTest;
+FORWARD_DECLARE_TEST(SavedFilesServiceUnitTest, RetainTwoFilesTest);
+FORWARD_DECLARE_TEST(SavedFilesServiceUnitTest, EvictionTest);
+FORWARD_DECLARE_TEST(SavedFilesServiceUnitTest, SequenceNumberCompactionTest);
+FORWARD_DECLARE_TEST(SavedFilesServiceUnitTest,
+                     SequenceNumberCompactionFirstAboveOneTest);
+
+namespace extensions {
+class Extension;
+}  // namespace extensions
+
+namespace apps {
+
+// Represents a file entry that a user has given an app permission to
+// access. Intended to be persisted to disk (in the Preferences file), so should

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

s/intended to/will/

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

Done.

+// remain serializable.
+struct SavedFileEntry {
+  SavedFileEntry()

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

I don't think these constructors should be in the header file. Not sure though.

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

Done.

+      : writable(false),
+        sequence_number(0) {}
+
+  SavedFileEntry(const std::string& id,
+                 const base::FilePath& path,
+                 bool writable,
+                 int sequence_number)
+      : id(id),
+        path(path),
+        writable(writable),
+        sequence_number(sequence_number) {}
+
+  // The opaque id of this file entry.
+  std::string id;
+
+  // The path to a file entry that an extension had permission to access.

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

s/an extension/the app

(Should be "the" to clarify that this is a per-app setting, not remembering
whether there is any app that has access.)

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

Done.

+  base::FilePath path;
+
+  // Whether or not an extension had write access to a file entry.

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

s/an extension/the app

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

Done.

+  bool writable;
+
+  // The sequence number in the LRU of the file entry.

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

// The value 0 indicates that the entry is not in the LRU.

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

Done.

+  int sequence_number;
+};
+
+class SavedFilesService : public ProfileKeyedService,

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

Here I think is a good place to discuss what this class represents and the
difference between "retaining" a file (until app quit) and "adding it to the
queue of entries to be retained between app runs."

Honestly, I am still confused about the difference, and the comment on
RetainFileUntilAppSuspend doesn't really help since to a casual reader, it
basically says "the file will be retained but it will not be retained." The
details are very hard to understand.

+                          public content::NotificationObserver {
+ public:
+  explicit SavedFilesService(Profile* profile);
+  virtual ~SavedFilesService();
+
+  static SavedFilesService* Get(Profile* profile);
+
+  // Adds a file entry to be retained, but does not add it to the queue of

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

"...to be retained if the app is automatically restarted, but..." (I want to
emphasise that the point of this is to survive an auto-restart, but not an
explicit user close and re-open.)

+  // entries to be retained between app runs.
+  void RetainFileUntilAppSuspend(const std::string& extension_id,

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

Can this be called RetainFileUntilAppQuit? Suspend implies that it will only be
retained until the computer suspends and resumes; rather, the whole point of
this minor retainment is that it survives a suspend and resume.

+                                 const std::string& id,
+                                 const base::FilePath& file_path,
+                                 bool writable);
+
+  // Adds the file with |id| to the back of the queue to be retained
+  // permanently, evicting the least recently used entry at the front of the
+  // queue if it is full, or moves it to the back if already present.

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

Reword:

"If the file with |id| is not in the queue of files to be retained permanently,
adds the file to the back of the queue, evicting the least recently used entry
at the front of the queue if it is full. If it is already present, moves it to
the back of the queue."

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

Done.

+  void MoveEntryToBackOfQueue(const std::string& extension_id,

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

Hmm, actually, can we not use the word "Move"; it implies that the entry must
already be in the queue. How about PutEntryAtBackOfQueue or
AddEntryToBackOfQueue.

+                              const std::string& id);
+
+  // Returns whether the file entry with the given |id| is retained, either
+  // permanently or just until app suspend.
+  bool IsRetained(const std::string& extension_id, const std::string& id);
+
+  // Stores the file entry with the specified |id| in |out| and returns true if
+  // the file entry is retained or returns false.
+  bool GetFileEntry(const std::string& extension_id,
+                    const std::string& id,
+                    SavedFileEntry* out);
+
+  // Returns all retained file entries.

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

Retained until app quit, or persistently retained?

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

all!

+  std::vector<SavedFileEntry> GetAllFileEntries(
+      const std::string& extension_id);
+
+  // Clears all retained files if the app does not have the
+  // fileSystem.retainFiles permission.
+  void MaybeClearQueue(const extensions::Extension* extension);
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(::SavedFilesServiceUnitTest, RetainTwoFilesTest);
+  FRIEND_TEST_ALL_PREFIXES(::SavedFilesServiceUnitTest, EvictionTest);
+  FRIEND_TEST_ALL_PREFIXES(::SavedFilesServiceUnitTest,
+                           SequenceNumberCompactionTest);
+  FRIEND_TEST_ALL_PREFIXES(::SavedFilesServiceUnitTest,
+                           SequenceNumberCompactionFirstAboveOneTest);
+  friend class ::SavedFilesServiceUnitTest;
+
+  class SavedFiles;

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

Can you explain what SavedFiles represents? (I gather from reading the code that
it is the collection of saved files for a given app.)

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

Done.

+
+  // content::NotificationObserver.
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  SavedFiles* GetOrInsert(const std::string& extension_id);

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

Brief comment.

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

Done.

+
+  void Clear(const std::string& extension_id);

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

Brief comment.

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

Done.

+
+  static void SetMaxSequenceNumberForTest(int max_value);
+  static void ClearMaxSequenceNumberForTest();
+  static void SetLruSizeForTest(int size);
+  static void ClearLruSizeForTest();
+
+  std::map<std::string, SavedFiles*> extension_id_to_saved_files_;
+  STLValueDeleter<std::map<std::string, SavedFiles*> >
+      extension_id_to_saved_files_deleter_;
+  content::NotificationRegistrar registrar_;
+  Profile* profile_;
+
+  DISALLOW_COPY_AND_ASSIGN(SavedFilesService);
+};
+
+}  // namespace apps
+
+#endif  // APPS_SAVED_FILES_SERVICE_H_