Add support for persistent file access in apps.

Add support for persistent file access in apps.

This replaces chrome.fileSystem.getEntry(By)Id with retainEntry,
restoreEntry and isRestorable, which enable restoring file access after
an app is closed for apps with the fileSystem.retainFiles permission.
Retained files are evicted by LRU, with a maximum of 500 retained files.

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

[Matt Giuca, 2013-05-16 06:40:59]

Here are some starter comments for the main files. I haven't done a full review
yet.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:711:
EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &entry_id));
Comment: // Add the file to the retain list if it is not already on there.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:716: }
Comment: // Move the file to the front of the queue.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:725:
EXTENSION_FUNCTION_VALIDATE(args_->GetString(2, &filesystem_path));
Question: Does GetString guarantee a valid UTF-8 string? You use this later in
FromUTF8Unsafe, which has a precondition of valid UTF-8.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:742:
base::FilePath::FromUTF8Unsafe(filesystem_path);
If the above is true, can you add a comment here that filesystem_path is valid
UTF-8 from GetString.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:769:
EXTENSION_FUNCTION_VALIDATE(args_->GetBoolean(1, &needs_new_file_entry));
What is needs_new_file_entry? Where is it used? It isn't documented in the .idl.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:776:
SavedFilesService::Get(profile())->RecordFileAccess(extension_->id(),
// Move the file to the front of the queue.

(Maybe RecordFileAccess needs a better name so we don't need this comment, like
MoveEntryToFrontOfQueue.)

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.h:46: const
std::string& id);
Since you have had to make existing calls pass "" here, would it be nicer to add
an overload that does not take an id?

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.h:128: bool
AddFileEntry(const std::string& entry_id);
Document this. Be sure to mention that |entry_id| needs to be an Isolated FS.

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
chrome/common/extensions/api/file_system.idl:88: // given file entry. Retained
entries are evicted by LRU when more than 500
Should be a bit more formal since this is user-facing docs. "Only the 500 most
recently used entries are retained, where calls to retainEntry and restoreEntry
count as use."

https://codereview.chromium.org/14607023/diff/48002/chrome/renderer/resources...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/14607023/diff/48002/chrome/renderer/resources...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:93: return
'';
Why not null?

[benwells, 2013-05-16 06:50:17]

I also haven't done a full review.

I have a question / comment. In saved_files_service.cc, there is lots of pointer
usage which makes ownership a little confusing. Feels like this could be greatly
simplified by storing / passing structures, vectors and maps by value. Was the
pointer stuff deliberately done for performance reasons?

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
Super nit: no (c)

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:12: #include
"base/files/file_path.h"
Nit: blank line between <> includes and others

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:20: namespace
extensions {
I'm thinking this should be in the apps folder / namespace as it is kind of
shared by a variety of apis.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:53: void
AddFileEntry(const std::string& extension_id,
Nit: I assume this maps to retainFileEntry, can we make its name something like
RetainFileEntry?

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:60: bool
IsSaved(const std::string& extension_id, const std::string& id);
Nit: IsRetained?

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:66: void
GetFileEntries(const std::string& extension_id,
Why not return the vector?

Nit: GetAllFileEntries

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:93: void
SetMaxSequenceNumberForTest(int max_value);
Nit: Please make these static methods on the service.

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:212: "channel": "stable",
Let's restrict this to trunk until we get the UI done as well.

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:215: "channel": "stable",
Also let's not give this to QuickOffice unless they need it (i.e. let's not
perpetuate this hack ...)

[Matt Giuca, 2013-05-16 07:20:06]

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2013/05/16 06:50:17, benwells wrote:
> Super nit: no (c)

Meta-nit: This is a tiny nit, not a super nit.

[Sam McNally, 2013-05-17 03:38:51]

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:711:
EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &entry_id));
On 2013/05/16 06:40:59, Matt Giuca wrote:
> Comment: // Add the file to the retain list if it is not already on there.

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:716: }
On 2013/05/16 06:40:59, Matt Giuca wrote:
> Comment: // Move the file to the front of the queue.

Renamed instead.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:725:
EXTENSION_FUNCTION_VALIDATE(args_->GetString(2, &filesystem_path));
On 2013/05/16 06:40:59, Matt Giuca wrote:
> Question: Does GetString guarantee a valid UTF-8 string? You use this later in
> FromUTF8Unsafe, which has a precondition of valid UTF-8.

Yes.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:742:
base::FilePath::FromUTF8Unsafe(filesystem_path);
On 2013/05/16 06:40:59, Matt Giuca wrote:
> If the above is true, can you add a comment here that filesystem_path is valid
> UTF-8 from GetString.

Refactored to remove this duplicate code.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:769:
EXTENSION_FUNCTION_VALIDATE(args_->GetBoolean(1, &needs_new_file_entry));
On 2013/05/16 06:40:59, Matt Giuca wrote:
> What is needs_new_file_entry? Where is it used? It isn't documented in the
.idl.

It's an implementation detail. Added a comment.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:776:
SavedFilesService::Get(profile())->RecordFileAccess(extension_->id(),
On 2013/05/16 06:40:59, Matt Giuca wrote:
> // Move the file to the front of the queue.
> 
> (Maybe RecordFileAccess needs a better name so we don't need this comment,
like
> MoveEntryToFrontOfQueue.)

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.h:46: const
std::string& id);
On 2013/05/16 06:40:59, Matt Giuca wrote:
> Since you have had to make existing calls pass "" here, would it be nicer to
add
> an overload that does not take an id?

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.h:128: bool
AddFileEntry(const std::string& entry_id);
On 2013/05/16 06:40:59, Matt Giuca wrote:
> Document this. Be sure to mention that |entry_id| needs to be an Isolated FS.

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2013/05/16 06:50:17, benwells wrote:
> Super nit: no (c)

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:12: #include
"base/files/file_path.h"
On 2013/05/16 06:50:17, benwells wrote:
> Nit: blank line between <> includes and others

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:20: namespace
extensions {
On 2013/05/16 06:50:17, benwells wrote:
> I'm thinking this should be in the apps folder / namespace as it is kind of
> shared by a variety of apis.

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:53: void
AddFileEntry(const std::string& extension_id,
On 2013/05/16 06:50:17, benwells wrote:
> Nit: I assume this maps to retainFileEntry, can we make its name something
like
> RetainFileEntry?

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:60: bool
IsSaved(const std::string& extension_id, const std::string& id);
On 2013/05/16 06:50:17, benwells wrote:
> Nit: IsRetained?

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:66: void
GetFileEntries(const std::string& extension_id,
On 2013/05/16 06:50:17, benwells wrote:
> Why not return the vector?
> 
> Nit: GetAllFileEntries

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/saved_files_service.h:93: void
SetMaxSequenceNumberForTest(int max_value);
On 2013/05/16 06:50:17, benwells wrote:
> Nit: Please make these static methods on the service.

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:212: "channel": "stable",
On 2013/05/16 06:50:17, benwells wrote:
> Let's restrict this to trunk until we get the UI done as well.

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:215: "channel": "stable",
On 2013/05/16 06:50:17, benwells wrote:
> Also let's not give this to QuickOffice unless they need it (i.e. let's not
> perpetuate this hack ...)

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/14607023/diff/48002/chrome/common/extensions/...
chrome/common/extensions/api/file_system.idl:88: // given file entry. Retained
entries are evicted by LRU when more than 500
On 2013/05/16 06:40:59, Matt Giuca wrote:
> Should be a bit more formal since this is user-facing docs. "Only the 500 most
> recently used entries are retained, where calls to retainEntry and
restoreEntry
> count as use."

Done.

https://codereview.chromium.org/14607023/diff/48002/chrome/renderer/resources...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/14607023/diff/48002/chrome/renderer/resources...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:93: return
'';
On 2013/05/16 06:40:59, Matt Giuca wrote:
> Why not null?

The idl schema parser and/or json schema compiler do not support nullable return
types.

[Matt Giuca, 2013-05-17 08:28:45]

OK I just read the header file. I think mostly I want documentation ;)

I will take a proper read of saved_files_service.cc on Monday.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.cc:45: const size_t kDefaultMaxSavedFileEntries = 500;
I think that the only way to get a non-default size here is in a test, right?
(using SetLruSizeForTest). Since that's test-only, I don't think "Default" is an
appropriate name because it implies that it can be changed by the user.

Rename this to kMaxSavedFileEntries.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.cc:46: const int kDefaultMaxSequenceNumber = kint32max;
And this to kMaxSequenceNumber.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.cc:48: size_t g_max_saved_file_entries =
kDefaultMaxSavedFileEntries;
... and add a comment here that this might be different to kMaxSavedFileEntries
in testing.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:23: // Represents a file entry that a user has given
an extension permission to
s/extension/app

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:26: struct SavedFileEntry {
If it's serializable, how do you serialize it? Shouldn't there be a method for
doing so?

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:40: std::string id;
Document (briefly) each of these fields.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:54: void RetainFileEntry(const std::string&
extension_id,
Document.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:59: void MoveEntryToFrontOfQueue(const std::string&
extension_id,
Document.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:62: bool IsRetained(const std::string& extension_id,
const std::string& id);
Document.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:64: bool GetFileEntry(const std::string&
extension_id,
Document.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:68: std::vector<SavedFileEntry> GetAllFileEntries(
Document.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:71: void ClearExtensionForTest(const std::string&
extension_id);
Document.

https://codereview.chromium.org/14607023/diff/78001/apps/saved_files_service....
apps/saved_files_service.h:73: static void SetMaxSequenceNumberForTest(int
max_value);
Document these four, and explain (in a paragraph above) that these should only
be used for testing purposes, and not in production code. I know the name
implies this, but it's always good to document as well.

Or, perhaps better yet, make them private and make the test class a friend.

[Sam McNally, 2013-05-20 01:17:13]

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
File apps/saved_files_service.cc (right):

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.cc:45: const size_t kDefaultMaxSavedFileEntries = 500;
On 2013/05/17 08:28:45, Matt Giuca wrote:
> I think that the only way to get a non-default size here is in a test, right?
> (using SetLruSizeForTest). Since that's test-only, I don't think "Default" is
an
> appropriate name because it implies that it can be changed by the user.
> 
> Rename this to kMaxSavedFileEntries.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.cc:46: const int kDefaultMaxSequenceNumber = kint32max;
On 2013/05/17 08:28:45, Matt Giuca wrote:
> And this to kMaxSequenceNumber.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.cc:48: size_t g_max_saved_file_entries =
kDefaultMaxSavedFileEntries;
On 2013/05/17 08:28:45, Matt Giuca wrote:
> ... and add a comment here that this might be different to
kMaxSavedFileEntries
> in testing.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
File apps/saved_files_service.h (right):

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:23: // Represents a file entry that a user has given
an extension permission to
On 2013/05/17 08:28:45, Matt Giuca wrote:
> s/extension/app

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:26: struct SavedFileEntry {
On 2013/05/17 08:28:45, Matt Giuca wrote:
> If it's serializable, how do you serialize it? Shouldn't there be a method for
> doing so?

See AddSavedFileEntry and friends in saved_files_service.cc.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:40: std::string id;
On 2013/05/17 08:28:45, Matt Giuca wrote:
> Document (briefly) each of these fields.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:54: void RetainFileEntry(const std::string&
extension_id,
On 2013/05/17 08:28:45, Matt Giuca wrote:
> Document.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:59: void MoveEntryToFrontOfQueue(const std::string&
extension_id,
On 2013/05/17 08:28:45, Matt Giuca wrote:
> Document.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:62: bool IsRetained(const std::string& extension_id,
const std::string& id);
On 2013/05/17 08:28:45, Matt Giuca wrote:
> Document.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:64: bool GetFileEntry(const std::string&
extension_id,
On 2013/05/17 08:28:45, Matt Giuca wrote:
> Document.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:68: std::vector<SavedFileEntry> GetAllFileEntries(
On 2013/05/17 08:28:45, Matt Giuca wrote:
> Document.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:71: void ClearExtensionForTest(const std::string&
extension_id);
On 2013/05/17 08:28:45, Matt Giuca wrote:
> Document.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/78001/apps/saved_files_s...
apps/saved_files_service.h:73: static void SetMaxSequenceNumberForTest(int
max_value);
On 2013/05/17 08:28:45, Matt Giuca wrote:
> Document these four, and explain (in a paragraph above) that these should only
> be used for testing purposes, and not in production code. I know the name
> implies this, but it's always good to document as well.
> 
> Or, perhaps better yet, make them private and make the test class a friend.

Done.

[Matt Giuca, 2013-05-20 01:32:57]

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service...
apps/app_restore_service.cc:74: if (should_restore_apps)
Not your fault, but can you please add a comment here explaining what
circumstances each branch will be taken in. I don't understand just from looking
at the code.

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service...
apps/saved_files_service.h:98: void ClearRetainedFiles(const
extensions::Extension* extension);
This name is not appropriate. Because it basically clears everything OTHER than
the retained files queue. Think of a better name.

https://codereview.chromium.org/14607023/diff/103001/chrome/renderer/extensio...
File chrome/renderer/extensions/file_system_natives.cc (right):

https://codereview.chromium.org/14607023/diff/103001/chrome/renderer/extensio...
chrome/renderer/extensions/file_system_natives.cc:110: if
(!fileapi::CrackIsolatedFileSystemName(filesystem_name, &filesystem_id)) {
Nit: Remove braces.

[Matt Giuca, 2013-05-20 01:34:12]

It would probably be a good thing if you split out the changes that add a the
retainFiles permission into a separate CL.

It's a bit weird having a permission that does nothing, but I think it would
help to reduce the scope of this CL somewhat. Ben, what do you think? (I would
guess that those changes are about 5% of this CL, so not going to reduce it by
much, but at least it means this CL is adding 1 thing instead of 2.)

[benwells, 2013-05-20 02:18:59]

On 2013/05/20 01:34:12, Matt Giuca wrote:
> It would probably be a good thing if you split out the changes that add a the
> retainFiles permission into a separate CL.
> 
> It's a bit weird having a permission that does nothing, but I think it would
> help to reduce the scope of this CL somewhat. Ben, what do you think? (I would
> guess that those changes are about 5% of this CL, so not going to reduce it by
> much, but at least it means this CL is adding 1 thing instead of 2.)

If it is only 5% of the CL I don't think it is worth it.

[Sam McNally, 2013-05-20 04:25:19]

https://chromiumcodereview.appspot.com/14607023/diff/103001/apps/app_restore_...
File apps/app_restore_service.cc (right):

https://chromiumcodereview.appspot.com/14607023/diff/103001/apps/app_restore_...
apps/app_restore_service.cc:74: if (should_restore_apps)
On 2013/05/20 01:32:57, Matt Giuca wrote:
> Not your fault, but can you please add a comment here explaining what
> circumstances each branch will be taken in. I don't understand just from
looking
> at the code.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/103001/apps/saved_files_...
File apps/saved_files_service.h (right):

https://chromiumcodereview.appspot.com/14607023/diff/103001/apps/saved_files_...
apps/saved_files_service.h:98: void ClearRetainedFiles(const
extensions::Extension* extension);
On 2013/05/20 01:32:57, Matt Giuca wrote:
> This name is not appropriate. Because it basically clears everything OTHER
than
> the retained files queue. Think of a better name.

Done.

https://chromiumcodereview.appspot.com/14607023/diff/103001/chrome/renderer/e...
File chrome/renderer/extensions/file_system_natives.cc (right):

https://chromiumcodereview.appspot.com/14607023/diff/103001/chrome/renderer/e...
chrome/renderer/extensions/file_system_natives.cc:110: if
(!fileapi::CrackIsolatedFileSystemName(filesystem_name, &filesystem_id)) {
On 2013/05/20 01:32:57, Matt Giuca wrote:
> Nit: Remove braces.

Done.

[Matt Giuca, 2013-05-20 05:02:03]

https://codereview.chromium.org/14607023/diff/121003/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/121003/apps/saved_files_service...
apps/saved_files_service.h:125: Profile* profile_;
Can this be a const reference?

[Matt Giuca, 2013-05-20 05:06:33]

Ben: Sam walked me through all the places where pointers are used. I think
they're all appropriate uses of pointers (or, in some cases, inappropriate, but
they are necessary to talk to existing services that take pointers).

https://codereview.chromium.org/14607023/diff/121003/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/121003/apps/saved_files_service...
apps/saved_files_service.cc:153: 
Nit: Remove this blank line.

[koz (OOO until 15th September), 2013-05-20 05:33:51]

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service...
File apps/app_restore_service_browsertest.cc (right):

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service...
apps/app_restore_service_browsertest.cc:83: // will have them all cleared.
This comment is probably doing more harm than good now, could you remove it?

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service...
apps/app_restore_service_browsertest.cc:90: file_entries.clear();
No need to clear file_entries here.

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service...
apps/app_restore_service_browsertest.cc:125: // will have them all cleared.
Same here.

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service...
apps/saved_files_service.h:51: std::string id;
nit: newlines between these fields.

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service...
apps/saved_files_service.h:70: void RetainFileEntry(const std::string&
extension_id,
Sounds like SavedFilesService has two sets of files - those that will be
retained between runs and those that are only retained until the extension is
suspended. I think exposing this in the method names of this class would make
its usage much clearer. What do you think of the following renames:

RetainFileEntry() -> RetainFileUntilAppSuspend()
MoveEntryToFrontOfQueue() -> RetainFilePermanently()
FlushRetainedEntries() -> ReleaseNonPermanentFiles()

I think it's enough to note in comments that "permanent" is a relative term
here.

[Matt Giuca, 2013-05-20 05:41:05]

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service...
apps/saved_files_service.h:70: void RetainFileEntry(const std::string&
extension_id,
On 2013/05/20 05:33:51, koz wrote:
> Sounds like SavedFilesService has two sets of files - those that will be
> retained between runs and those that are only retained until the extension is
> suspended. I think exposing this in the method names of this class would make
> its usage much clearer. What do you think of the following renames:
> 
> RetainFileEntry() -> RetainFileUntilAppSuspend()
> MoveEntryToFrontOfQueue() -> RetainFilePermanently()
> FlushRetainedEntries() -> ReleaseNonPermanentFiles()
> 
> I think it's enough to note in comments that "permanent" is a relative term
> here.

I don't like RetainFilePermanently because it doesn't imply that it will bring
the file to the front of the queue. Otherwise agree.

[koz (OOO until 15th September), 2013-05-20 06:10:54]

On 2013/05/20 05:41:05, Matt Giuca wrote:
>
https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service.h
> File apps/saved_files_service.h (right):
> 
>
https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service...
> apps/saved_files_service.h:70: void RetainFileEntry(const std::string&
> extension_id,
> On 2013/05/20 05:33:51, koz wrote:
> > Sounds like SavedFilesService has two sets of files - those that will be
> > retained between runs and those that are only retained until the extension
is
> > suspended. I think exposing this in the method names of this class would
make
> > its usage much clearer. What do you think of the following renames:
> > 
> > RetainFileEntry() -> RetainFileUntilAppSuspend()
> > MoveEntryToFrontOfQueue() -> RetainFilePermanently()
> > FlushRetainedEntries() -> ReleaseNonPermanentFiles()
> > 
> > I think it's enough to note in comments that "permanent" is a relative term
> > here.
> 
> I don't like RetainFilePermanently because it doesn't imply that it will bring
> the file to the front of the queue. Otherwise agree.

I think it's more important that this function connects somehow with
FlushRetainedEntries(). Another option could be to rename FlushRetainedEntries()
to ReleaseEntriesNotInQueue(). That way the connection to
MoveEntryToFrontOfQueue() is clearer.

[Sam McNally, 2013-05-20 07:13:33]

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service...
File apps/app_restore_service_browsertest.cc (right):

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service...
apps/app_restore_service_browsertest.cc:83: // will have them all cleared.
On 2013/05/20 05:33:51, koz wrote:
> This comment is probably doing more harm than good now, could you remove it?

Done.

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service...
apps/app_restore_service_browsertest.cc:90: file_entries.clear();
On 2013/05/20 05:33:51, koz wrote:
> No need to clear file_entries here.

Done.

https://codereview.chromium.org/14607023/diff/103001/apps/app_restore_service...
apps/app_restore_service_browsertest.cc:125: // will have them all cleared.
On 2013/05/20 05:33:51, koz wrote:
> Same here.

Done.

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service...
apps/saved_files_service.h:51: std::string id;
On 2013/05/20 05:33:51, koz wrote:
> nit: newlines between these fields.

Done.

https://codereview.chromium.org/14607023/diff/103001/apps/saved_files_service...
apps/saved_files_service.h:70: void RetainFileEntry(const std::string&
extension_id,
On 2013/05/20 05:41:05, Matt Giuca wrote:
> On 2013/05/20 05:33:51, koz wrote:
> > Sounds like SavedFilesService has two sets of files - those that will be
> > retained between runs and those that are only retained until the extension
is
> > suspended. I think exposing this in the method names of this class would
make
> > its usage much clearer. What do you think of the following renames:
> > 
> > RetainFileEntry() -> RetainFileUntilAppSuspend()
> > MoveEntryToFrontOfQueue() -> RetainFilePermanently()
> > FlushRetainedEntries() -> ReleaseNonPermanentFiles()
> > 
> > I think it's enough to note in comments that "permanent" is a relative term
> > here.
> 
> I don't like RetainFilePermanently because it doesn't imply that it will bring
> the file to the front of the queue. Otherwise agree.

Done.

[Matt Giuca, 2013-05-22 08:26:13]

OK, I've basically completed my review. There are a bunch of things I didn't
look at in a lot of detail (like all the mechanical cleanup stuff), but I looked
over the new service in a lot of detail.

Again, my main comment is: more documentation please. This review was quite hard
to do because I basically had to sit through and understand all the code which
had no comments at all. This may indicate that the code itself needs to be
refactored, but I think it is actually pretty good, but it is just an
intrinsically confusing concept (especially with the two separate retain lists).
I think I would have been able to go through the review much more quickly if
there were detailed comments like I am asking for here.

Remember, the comments should not only be for when things are impossible to
understand: any minor difficulties when reading the code should have comments,
so that a reader can quickly skim through and find the part they are looking
for. Also, all functions and class variables (including private ones), should be
at least minimally documented, unless they are completely trivial. The style
guide says:

"Every function declaration should have comments immediately preceding it that
describe what the function does and how to use it."
and
"Each class data member ... should have a comment describing what it is used
for."

I found that pretty much all private functions are completely undocumented,
which means that I had to go and read their source code to find out the details
of how they behave.

Of course, feel free to push back on some of these requests. I have asked for a
lot of documentation. But I generally think the level of documentation should be
higher before sending out a review since it would speed up the review process
greatly.

https://codereview.chromium.org/14607023/diff/162003/apps/app_restore_service...
File apps/app_restore_service_browsertest.cc (left):

https://codereview.chromium.org/14607023/diff/162003/apps/app_restore_service...
apps/app_restore_service_browsertest.cc:85: // will have them all cleared.
Why is this comment removed?

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:50: void AddSavedFileEntry(ExtensionPrefs* prefs,
Nit: One-line comment on each of these.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:146: void MaybeCompactSequenceNumbers();
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.)

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:152: base::hash_map<std::string, SavedFileEntry*>
file_id_to_file_entry_map_;
What does it mean to be in this map? It seems to mean that you will be retained
until app quit but not persistently.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:154: file_id_to_file_entry_map_deleter_;
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.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:156: // Values are a subset of values in
file_id_to_file_entry_map_.
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.)

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:298: return;
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.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:302: if (!saved_file_lru_.empty()) {
// Get the sequence number after the last file entry in the LRU.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:310: file_entry->sequence_number = 1;
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.)

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:334: bool
SavedFilesService::SavedFiles::GetFileEntry(const std::string& id,
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;

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:356: void
SavedFilesService::SavedFiles::MaybeCompactSequenceNumbers() {
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.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:362: if (it->first < g_max_sequence_number)
// Only compact sequence numbers if the last entry's sequence number is the
maximum value.
// This should almost never be the case.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:369: sequence_number++;
Nit: ++sequence_number (style)

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:376: if (it == saved_file_lru_.begin()) {
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.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:381: saved_file_lru_.erase(it--);
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.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:16: #include
"components/browser_context_keyed_service/browser_context_keyed_service.h"
Line too long. Not sure if the style guide permits this or whether you're
supposed to wrap it.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:35: // access. Intended to be persisted to disk (in
the Preferences file), so should
s/intended to/will/

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:38: SavedFileEntry()
I don't think these constructors should be in the header file. Not sure though.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:54: // The path to a file entry that an extension had
permission to access.
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.)

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:57: // Whether or not an extension had write access
to a file entry.
s/an extension/the app

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:60: // The sequence number in the LRU of the file
entry.
// The value 0 indicates that the entry is not in the LRU.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:64: class SavedFilesService : public
ProfileKeyedService,
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.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:72: // Adds a file entry to be retained, but does not
add it to the queue of
"...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.)

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:74: void RetainFileUntilAppSuspend(const std::string&
extension_id,
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.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:81: // queue if it is full, or moves it to the back
if already present.
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."

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:82: void MoveEntryToBackOfQueue(const std::string&
extension_id,
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.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:95: // Returns all retained file entries.
Retained until app quit, or persistently retained?

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:112: class SavedFiles;
Can you explain what SavedFiles represents? (I gather from reading the code that
it is the collection of saved files for a given app.)

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:119: SavedFiles* GetOrInsert(const std::string&
extension_id);
Brief comment.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:121: void Clear(const std::string& extension_id);
Brief comment.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
File apps/saved_files_service_factory.h (right):

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_factory.h:9: #include
"components/browser_context_keyed_service/browser_context_keyed_service_factory.h"
same.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
File apps/saved_files_service_unittest.cc (right):

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:39: class SavedFilesServiceUnitTest :
public testing::Test {
Sam: These are really great tests. Very comprehensive and no redundancy (except
the confusing last one).

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:43: extension_ =
env_.MakeExtension(*base::test::ParseJson(
Can you expand this into a proper multi-line string.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:56: void CheckSavedFileEntry(const
std::string& id, int sequence_number) {
Document.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:56: void CheckSavedFileEntry(const
std::string& id, int sequence_number) {
It seems like you call this with GenerateId(...) every single time.

Could you just take the int, and call GenerateId inside this function? It would
make the tests easier to read.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:67: void CheckSavedFileEntries(int start,
int end) {
Document.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:69: for (int i = start; i < end; i++) {
++i

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:79: TEST_F(SavedFilesServiceUnitTest,
RetainTwoFilesTest) {
Nit: Maybe add some blank lines to space this out a bit.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:85: extension_->id(), GenerateId(3), path_,
true);
Please add a line explaining what each of these sections is testing. For example
// Test that all entries have the empty sequence number.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:90:
TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 1));
// Test that entry #1 has a valid sequence number.
etc

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:105:
EXPECT_FALSE(service_->IsRetained(extension_->id(), "another id"));
Why don't you test IsRetained on one of the actually retained files?
You should also test that IsRetained returns false after calling
RetainFileUntilAppSuspend but before MoveEntryToBackOfQueue.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:108: service_->MaybeClearQueue(extension_);
// MaybeClearQueue should have no effect, since the app has the "retainFiles"
permission.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:112: service_->Clear(extension_->id());
I don't follow why Clear has no effect here?

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:130: service_->MaybeClearQueue(extension_);
// MaybeClearQueue should clear the queue, since the app does not have the
"retainFiles" permission.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:138: for (int i = 0; i < 10; i++) {
++i (find and replace throughout this file)

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:143: service_->RetainFileUntilAppSuspend(
// Expect that entries 0 to 9 are in the LRU.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:149:
TRACE_CALL(CheckSavedFileEntry(GenerateId(0), 0));
// Expect that entries 1 to 10 are in the LRU, and 0 has been evicted.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:156:
service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(1));
Might it be worth testing this on id 2 instead of 1, to prove that the least
recently used id (1) was not evicted?

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:157:
TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 12));
// Expect that entry 1 is now at the back of the queue, and no further entries
have been evicted.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:159: service_->Clear(extension_->id());
Again, I don't know why Clear is not expected to have any effect?

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:165:
SavedFilesService::SetMaxSequenceNumberForTest(8);
SavedFilesService::SetLruSizeForTest(8);

To satisfy DCHECK(g_max_sequence_number >= g_max_saved_file_entries); which I
suggested in saved_files_service.cc.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:174:
TRACE_CALL(CheckSavedFileEntry(GenerateId(0), 1));
// The sequence numbers should be sparse, as they have not gone over the limit.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:184:
service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(3));
// This should push the sequence number to the limit of 8, and trigger a
sequence number compaction.
// Expect that the sequence numbers are contiguous from 1 to 4.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:190: TEST_F(SavedFilesServiceUnitTest,
SequenceNumberCompactionFirstAboveOneTest) {
I don't get what this is testing that the previous one did not test. Please
explain in comments or delete this test.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:191:
SavedFilesService::SetMaxSequenceNumberForTest(10);
SavedFilesService::SetLruSizeForTest(10);

https://codereview.chromium.org/14607023/diff/162003/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/14607023/diff/162003/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:794: // |entry_id|.
Can you explain why you used file_entry.id instead of letting it use the id of
the writable file entry?

https://codereview.chromium.org/14607023/diff/162003/chrome/common/extensions...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/14607023/diff/162003/chrome/common/extensions...
chrome/common/extensions/api/file_system.idl:80: // Returns the file entry with
the given id if it can be restored.
Otherwise, returns ...?

https://codereview.chromium.org/14607023/diff/162003/chrome/renderer/resource...
File chrome/renderer/resources/extensions/entry_id_manager.js (right):

https://codereview.chromium.org/14607023/diff/162003/chrome/renderer/resource...
chrome/renderer/resources/extensions/entry_id_manager.js:35: var id =
nameToIds[name];
Maybe explain what this is doing.

https://codereview.chromium.org/14607023/diff/162003/chrome/renderer/resource...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/14607023/diff/162003/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:89: 
PLEASE NOTE: I have not looked at this in detail and I don't have time now.
Remind me to look at it later if I don't get around to it :)

https://codereview.chromium.org/14607023/diff/162003/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system/restore_entry/test.js
(right):

https://codereview.chromium.org/14607023/diff/162003/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system/restore_entry/test.js:18: true
/*shouldBeWritable */);
Wrapping. Indent to column 17.

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

https://codereview.chromium.org/14607023/diff/162003/apps/app_restore_service...
File apps/app_restore_service_browsertest.cc (left):

https://codereview.chromium.org/14607023/diff/162003/apps/app_restore_service...
apps/app_restore_service_browsertest.cc:85: // will have them all cleared.
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Why is this comment removed?

koz said to.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:50: void AddSavedFileEntry(ExtensionPrefs* prefs,
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Nit: One-line comment on each of these.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:146: void MaybeCompactSequenceNumbers();
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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.)

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:156: // Values are a subset of values in
file_id_to_file_entry_map_.
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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.)

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:298: return;
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:310: file_entry->sequence_number = 1;
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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.)

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:334: bool
SavedFilesService::SavedFiles::GetFileEntry(const std::string& id,
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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;

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:356: void
SavedFilesService::SavedFiles::MaybeCompactSequenceNumbers() {
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:362: if (it->first < g_max_sequence_number)
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // Only compact sequence numbers if the last entry's sequence number is the
> maximum value.
> // This should almost never be the case.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:376: if (it == saved_file_lru_.begin()) {
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.cc:381: saved_file_lru_.erase(it--);
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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.

Switched to hint following.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:35: // access. Intended to be persisted to disk (in
the Preferences file), so should
On 2013/05/22 08:26:14, Matt Giuca wrote:
> s/intended to/will/

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:38: SavedFileEntry()
On 2013/05/22 08:26:14, Matt Giuca wrote:
> I don't think these constructors should be in the header file. Not sure
though.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:54: // The path to a file entry that an extension had
permission to access.
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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.)

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:57: // Whether or not an extension had write access
to a file entry.
On 2013/05/22 08:26:14, Matt Giuca wrote:
> s/an extension/the app

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:60: // The sequence number in the LRU of the file
entry.
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // The value 0 indicates that the entry is not in the LRU.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:81: // queue if it is full, or moves it to the back
if already present.
On 2013/05/22 08:26:14, Matt Giuca wrote:
> 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."

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:95: // Returns all retained file entries.
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Retained until app quit, or persistently retained?

all!

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:112: class SavedFiles;
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Can you explain what SavedFiles represents? (I gather from reading the code
that
> it is the collection of saved files for a given app.)

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:119: SavedFiles* GetOrInsert(const std::string&
extension_id);
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Brief comment.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service.h:121: void Clear(const std::string& extension_id);
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Brief comment.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
File apps/saved_files_service_unittest.cc (right):

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:43: extension_ =
env_.MakeExtension(*base::test::ParseJson(
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Can you expand this into a proper multi-line string.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:56: void CheckSavedFileEntry(const
std::string& id, int sequence_number) {
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Document.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:56: void CheckSavedFileEntry(const
std::string& id, int sequence_number) {
On 2013/05/22 08:26:14, Matt Giuca wrote:
> It seems like you call this with GenerateId(...) every single time.
> 
> Could you just take the int, and call GenerateId inside this function? It
would
> make the tests easier to read.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:67: void CheckSavedFileEntries(int start,
int end) {
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Document.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:79: TEST_F(SavedFilesServiceUnitTest,
RetainTwoFilesTest) {
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Nit: Maybe add some blank lines to space this out a bit.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:85: extension_->id(), GenerateId(3), path_,
true);
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Please add a line explaining what each of these sections is testing. For
example
> // Test that all entries have the empty sequence number.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:90:
TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 1));
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // Test that entry #1 has a valid sequence number.
> etc

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:105:
EXPECT_FALSE(service_->IsRetained(extension_->id(), "another id"));
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Why don't you test IsRetained on one of the actually retained files?
> You should also test that IsRetained returns false after calling
> RetainFileUntilAppSuspend but before MoveEntryToBackOfQueue.

CheckSavedFileEntry tests that. It's intended to return true for that case.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:108: service_->MaybeClearQueue(extension_);
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // MaybeClearQueue should have no effect, since the app has the "retainFiles"
> permission.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:112: service_->Clear(extension_->id());
On 2013/05/22 08:26:14, Matt Giuca wrote:
> I don't follow why Clear has no effect here?

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:130: service_->MaybeClearQueue(extension_);
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // MaybeClearQueue should clear the queue, since the app does not have the
> "retainFiles" permission.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:143: service_->RetainFileUntilAppSuspend(
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // Expect that entries 0 to 9 are in the LRU.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:149:
TRACE_CALL(CheckSavedFileEntry(GenerateId(0), 0));
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // Expect that entries 1 to 10 are in the LRU, and 0 has been evicted.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:156:
service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(1));
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Might it be worth testing this on id 2 instead of 1, to prove that the least
> recently used id (1) was not evicted?

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:157:
TRACE_CALL(CheckSavedFileEntry(GenerateId(1), 12));
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // Expect that entry 1 is now at the back of the queue, and no further entries
> have been evicted.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:159: service_->Clear(extension_->id());
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Again, I don't know why Clear is not expected to have any effect?

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:165:
SavedFilesService::SetMaxSequenceNumberForTest(8);
On 2013/05/22 08:26:14, Matt Giuca wrote:
> SavedFilesService::SetLruSizeForTest(8);
> 
> To satisfy DCHECK(g_max_sequence_number >= g_max_saved_file_entries); which I
> suggested in saved_files_service.cc.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:174:
TRACE_CALL(CheckSavedFileEntry(GenerateId(0), 1));
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // The sequence numbers should be sparse, as they have not gone over the
limit.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:184:
service_->MoveEntryToBackOfQueue(extension_->id(), GenerateId(3));
On 2013/05/22 08:26:14, Matt Giuca wrote:
> // This should push the sequence number to the limit of 8, and trigger a
> sequence number compaction.
> // Expect that the sequence numbers are contiguous from 1 to 4.

Done.

https://codereview.chromium.org/14607023/diff/162003/apps/saved_files_service...
apps/saved_files_service_unittest.cc:190: TEST_F(SavedFilesServiceUnitTest,
SequenceNumberCompactionFirstAboveOneTest) {
On 2013/05/22 08:26:14, Matt Giuca wrote:
> I don't get what this is testing that the previous one did not test. Please
> explain in comments or delete this test.

Done.

https://codereview.chromium.org/14607023/diff/162003/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/14607023/diff/162003/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:794: // |entry_id|.
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Can you explain why you used file_entry.id instead of letting it use the id of
> the writable file entry?

Done.

https://codereview.chromium.org/14607023/diff/162003/chrome/common/extensions...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/14607023/diff/162003/chrome/common/extensions...
chrome/common/extensions/api/file_system.idl:80: // Returns the file entry with
the given id if it can be restored.
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Otherwise, returns ...?

Otherwise, fails.

https://codereview.chromium.org/14607023/diff/162003/chrome/renderer/resource...
File chrome/renderer/resources/extensions/entry_id_manager.js (right):

https://codereview.chromium.org/14607023/diff/162003/chrome/renderer/resource...
chrome/renderer/resources/extensions/entry_id_manager.js:35: var id =
nameToIds[name];
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Maybe explain what this is doing.

Done.

https://codereview.chromium.org/14607023/diff/162003/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system/restore_entry/test.js
(right):

https://codereview.chromium.org/14607023/diff/162003/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system/restore_entry/test.js:18: true
/*shouldBeWritable */);
On 2013/05/22 08:26:14, Matt Giuca wrote:
> Wrapping. Indent to column 17.

Done.

[Matt Giuca, 2013-05-23 04:53:42]

https://codereview.chromium.org/14607023/diff/162003/chrome/common/extensions...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/14607023/diff/162003/chrome/common/extensions...
chrome/common/extensions/api/file_system.idl:80: // Returns the file entry with
the given id if it can be restored.
What does "fail" mean? Silently? This is user-facing documentation so it needs
to be very clear and unambiguous.

https://codereview.chromium.org/14607023/diff/162003/chrome/renderer/resource...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/14607023/diff/162003/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:89: 
On 2013/05/22 08:26:14, Matt Giuca wrote:
> PLEASE NOTE: I have not looked at this in detail and I don't have time now.
> Remind me to look at it later if I don't get around to it :)

Done.

https://codereview.chromium.org/14607023/diff/162003/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:95: var
relativePath = fileEntry.fullPath.slice(1);
Would it make more sense to do this slice in the C++ code rather than in the JS
code? (So that the filePath parameter to the C++ code has the same semantics as
it does in the FileEntry object, instead of the confusingly different semantics
of being a relative path?)

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service...
apps/saved_files_service.cc:165: // g_max_sequence_number. Outside testing, it
is set to kint32max, so this
Nit: Outside of testing.

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service...
apps/saved_files_service.cc:333: file_entry->sequence_number = 1;
Nit: Thanks for adding the comment in the header file. But probably also worth
adding a comment here
// The first sequence number is 1, as 0 means the entry is not in the LRU.

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service...
apps/saved_files_service.h:67: // Registers a file entry. File entries that are
in the retained files queue
Registers a file entry ...? (for what?)

Perhaps "Registers a file entry with the saved files service, making it eligible
to be put into the queue."

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service...
apps/saved_files_service.h:78: // |id| has not been registered.
Shouldn't it be an error or CHECK fail if |id| has not been registered?

[Sam McNally, 2013-05-23 05:21:10]

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service...
apps/saved_files_service.cc:165: // g_max_sequence_number. Outside testing, it
is set to kint32max, so this
On 2013/05/23 04:53:43, Matt Giuca wrote:
> Nit: Outside of testing.

Done.

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service...
apps/saved_files_service.cc:333: file_entry->sequence_number = 1;
On 2013/05/23 04:53:43, Matt Giuca wrote:
> Nit: Thanks for adding the comment in the header file. But probably also worth
> adding a comment here
> // The first sequence number is 1, as 0 means the entry is not in the LRU.

Done.

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service...
apps/saved_files_service.h:67: // Registers a file entry. File entries that are
in the retained files queue
On 2013/05/23 04:53:43, Matt Giuca wrote:
> Registers a file entry ...? (for what?)
> 
> Perhaps "Registers a file entry with the saved files service, making it
eligible
> to be put into the queue."

Done.

https://codereview.chromium.org/14607023/diff/184002/apps/saved_files_service...
apps/saved_files_service.h:78: // |id| has not been registered.
On 2013/05/23 04:53:43, Matt Giuca wrote:
> Shouldn't it be an error or CHECK fail if |id| has not been registered?

Done.

[Matt Giuca, 2013-05-23 05:38:34]

LGTM!

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.cc:319: DCHECK(it != registered_file_entries_.end());
Please make this a CHECK, since it's memory corruption ville if this condition
fails. (Actually I have no idea what end() is, maybe it's NULL, but still I
think it's worth checking in Release build.)

[benwells, 2013-05-23 06:26:36]

Looking pretty good. Only nits so far, but I am only about 1/4 through ...

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
File apps/saved_files_service_factory.h (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_factory.h:14: namespace apps {
Nit: blank linke after the namespace.

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:784:
->GetFileEntry(extension_->id(), entry_id);
Is it the right style to put the "->" on the next line?

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:788: }
Nit: blank line here please. I like them after early returns.

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:796: // Reuse the
ID of the retained file entry so it retainEntry returns the
Nit: 'so it retainEntry returns ...' doesn't make sense.

[koz (OOO until 15th September), 2013-05-23 07:32:21]

Very close, I didn't quite get around to going over the .js tests but I'll do
that tomorrow morning.

https://codereview.chromium.org/14607023/diff/202001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/app_restore_service...
apps/app_restore_service.cc:74: // On a Chrome restart, restore the running
apps. Otherwise, on startup,
I know Matt asked for this comment but I think it's a bad idea to put this here
- ultimately whether or not apps are restored is something callers can do for
whatever reason, and that reason shouldn't be restated here.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.cc:289:
registered_file_entries_deleter_(&registered_file_entries_) {
I think this would be clearer as a method, so the constructor body would just be
 ... {
  LoadSavedFileEntriesFromPreferences();
}

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.cc:409: it = saved_file_lru_.insert(
I learned a new thing! Though I feel compelled to point out that as this will
almost never happen it's probably better to just not calculate the hint for the
extra code clarity. I leave it with you, though :-)

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.h:30: }  // namespace extensions
nit: don't need this comment

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.h:59: class SavedFilesService : public
ProfileKeyedService,
I think a class comment here would be awesome. It doesn't have to be very long.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
File apps/saved_files_service_factory.h (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_factory.h:20: // ProfileKeyedServiceFactory
implementation:
Remove this comment.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
File apps/saved_files_service_unittest.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_unittest.cc:65: void CheckSavedFileEntry(int id, int
sequence_number) {
I think this would be better named CheckEntrySequenceNumber(). It does a lot of
other checks but most are just opportunistic consistency checks.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_unittest.cc:79: void CheckSavedFileEntries(int start,
int end) {
CheckRangeEnqueuedInOrder()?

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_unittest.cc:84: }
nit: newline

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_unittest.cc:207: TEST_F(SavedFilesServiceUnitTest,
SequenceNumberCompactionTest) {
Just wanted to say, these tests are awesome!

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.h:50: // the generated
id.
Could you expand a bit on why it would be useful to override the generated id?

https://codereview.chromium.org/14607023/diff/202001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/14607023/diff/202001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:117: return
[id, false, null];
A clarifying comment here would help de-mystify these parameter lists.

[koz (OOO until 15th September), 2013-05-24 00:13:16]

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.cc:212: MaybeClearQueue(extension);
ClearQueueIfNoRetainPermission()?

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:52:
service->RegisterFileEntry(extension->id(), "magic id", path_to_save, true);
Add inline comment to true to indicate what it is for.

https://codereview.chromium.org/14607023/diff/202001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/entry_id_manager.js (right):

https://codereview.chromium.org/14607023/diff/202001/chrome/renderer/resource...
chrome/renderer/resources/extensions/entry_id_manager.js:19: return fileSystemId
+ ':' + entry.fullPath.slice(1);
Could you add a comment explaining why the slice(1) here?

https://codereview.chromium.org/14607023/diff/202001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system/retain_entry/test.js
(right):

https://codereview.chromium.org/14607023/diff/202001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system/retain_entry/test.js:9:
backgroundPage.callback = chrome.test.callbackPass(
This callback should go down to where chrome.app.window.create() is and add a
comment to explain that we are stashing it on the background page so it can be
called from the other window. Alternatively, have you considered using the
callback from chrome.app.window.create() to stash this callback on the other
window itself?

[Matt Giuca, 2013-05-24 00:24:02]

https://codereview.chromium.org/14607023/diff/202001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/app_restore_service...
apps/app_restore_service.cc:74: // On a Chrome restart, restore the running
apps. Otherwise, on startup,
OK I agree we shouldn't assume why the caller set this flag. But I still find
these four lines pretty eye-glazey. How about:
// If we are not restoring apps (e.g., because it is a clean restart), and the
app does not have retain permission, explicitly clear the retained entries
queue.

[koz (OOO until 15th September), 2013-05-24 00:28:43]

https://codereview.chromium.org/14607023/diff/202001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/app_restore_service...
apps/app_restore_service.cc:74: // On a Chrome restart, restore the running
apps. Otherwise, on startup,
On 2013/05/24 00:24:02, Matt Giuca wrote:
> OK I agree we shouldn't assume why the caller set this flag. But I still find
> these four lines pretty eye-glazey. How about:
> // If we are not restoring apps (e.g., because it is a clean restart), and the
> app does not have retain permission, explicitly clear the retained entries
> queue.

I love it!

[Sam McNally, 2013-05-24 00:46:02]

https://codereview.chromium.org/14607023/diff/202001/apps/app_restore_service.cc
File apps/app_restore_service.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/app_restore_service...
apps/app_restore_service.cc:74: // On a Chrome restart, restore the running
apps. Otherwise, on startup,
On 2013/05/24 00:24:02, Matt Giuca wrote:
> OK I agree we shouldn't assume why the caller set this flag. But I still find
> these four lines pretty eye-glazey. How about:
> // If we are not restoring apps (e.g., because it is a clean restart), and the
> app does not have retain permission, explicitly clear the retained entries
> queue.

Done.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service.cc
File apps/saved_files_service.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.cc:212: MaybeClearQueue(extension);
On 2013/05/24 00:13:16, koz wrote:
> ClearQueueIfNoRetainPermission()?

Done.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.cc:289:
registered_file_entries_deleter_(&registered_file_entries_) {
On 2013/05/23 07:32:22, koz wrote:
> I think this would be clearer as a method, so the constructor body would just
be
>  ... {
>   LoadSavedFileEntriesFromPreferences();
> }

Done.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service.h
File apps/saved_files_service.h (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.h:30: }  // namespace extensions
On 2013/05/23 07:32:22, koz wrote:
> nit: don't need this comment

Done.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service.h:59: class SavedFilesService : public
ProfileKeyedService,
On 2013/05/23 07:32:22, koz wrote:
> I think a class comment here would be awesome. It doesn't have to be very
long.

Done.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
File apps/saved_files_service_factory.h (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_factory.h:14: namespace apps {
On 2013/05/23 06:26:36, benwells wrote:
> Nit: blank linke after the namespace.

Done.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_factory.h:20: // ProfileKeyedServiceFactory
implementation:
On 2013/05/23 07:32:22, koz wrote:
> Remove this comment.

Done.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
File apps/saved_files_service_unittest.cc (right):

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_unittest.cc:65: void CheckSavedFileEntry(int id, int
sequence_number) {
On 2013/05/23 07:32:22, koz wrote:
> I think this would be better named CheckEntrySequenceNumber(). It does a lot
of
> other checks but most are just opportunistic consistency checks.

Done.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_unittest.cc:79: void CheckSavedFileEntries(int start,
int end) {
On 2013/05/23 07:32:22, koz wrote:
> CheckRangeEnqueuedInOrder()?

Done.

https://codereview.chromium.org/14607023/diff/202001/apps/saved_files_service...
apps/saved_files_service_unittest.cc:84: }
On 2013/05/23 07:32:22, koz wrote:
> nit: newline

Done.

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:784:
->GetFileEntry(extension_->id(), entry_id);
On 2013/05/23 06:26:36, benwells wrote:
> Is it the right style to put the "->" on the next line?

Clang format likes it, but I'm not a fan.

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:788: }
On 2013/05/23 06:26:36, benwells wrote:
> Nit: blank line here please. I like them after early returns.

Done.

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:796: // Reuse the
ID of the retained file entry so it retainEntry returns the
On 2013/05/23 06:26:36, benwells wrote:
> Nit: 'so it retainEntry returns ...' doesn't make sense.

Done.

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.h:50: // the generated
id.
On 2013/05/23 07:32:22, koz wrote:
> Could you expand a bit on why it would be useful to override the generated id?

Done.

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:52:
service->RegisterFileEntry(extension->id(), "magic id", path_to_save, true);
On 2013/05/24 00:13:16, koz wrote:
> Add inline comment to true to indicate what it is for.

Done.

https://codereview.chromium.org/14607023/diff/202001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/entry_id_manager.js (right):

https://codereview.chromium.org/14607023/diff/202001/chrome/renderer/resource...
chrome/renderer/resources/extensions/entry_id_manager.js:19: return fileSystemId
+ ':' + entry.fullPath.slice(1);
On 2013/05/24 00:13:16, koz wrote:
> Could you add a comment explaining why the slice(1) here?

Done.

https://codereview.chromium.org/14607023/diff/202001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/14607023/diff/202001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:117: return
[id, false, null];
On 2013/05/23 07:32:22, koz wrote:
> A clarifying comment here would help de-mystify these parameter lists.

Done.

https://codereview.chromium.org/14607023/diff/202001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system/retain_entry/test.js
(right):

https://codereview.chromium.org/14607023/diff/202001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system/retain_entry/test.js:9:
backgroundPage.callback = chrome.test.callbackPass(
On 2013/05/24 00:13:16, koz wrote:
> This callback should go down to where chrome.app.window.create() is and add a
> comment to explain that we are stashing it on the background page so it can be
> called from the other window. Alternatively, have you considered using the
> callback from chrome.app.window.create() to stash this callback on the other
> window itself?

Done.

[koz (OOO until 15th September), 2013-05-24 00:53:44]

LGTM

Nicely done!

[Matt Giuca, 2013-05-24 00:56:59]

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/14607023/diff/202001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:784:
->GetFileEntry(extension_->id(), entry_id);
I've seen humans recommend it too.

Perhaps the rationale is that, since it's two characters instead of one, if it
were to be at the end of the previous line, you would run the risk that it
didn't fit, and have to overflow potentially a lot more characters onto the next
line.

[benwells, 2013-05-24 03:26:32]

Removing myself as a reviewer as koz and mgiuca have covered it pretty
thoroughly.

Looking forward to seeing this one land :)

[commit-bot: I haz the power, 2013-05-24 04:38:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/14607023/234001

[commit-bot: I haz the power, 2013-05-24 04:38:19]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 531.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
b1c7f055e1962623ae1af6baffee84d14e2db344..59f048c9695a97d8c8cc74996e4bfd671e4132da
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -531,6 +531,9 @@ enum HistogramValue {
   METRICSPRIVATE_GETISCRASHRECORDINGENABLED,
   IDENTITYPRIVATE_GETRESOURCES,
   NOTIFICATIONS_GET_ALL,
+  FILESYSTEM_RETAINENTRY,
+  FILESYSTEM_ISRESTORABLE,
+  FILESYSTEM_RESTOREENTRY,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-05-24 04:42:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/14607023/238001

[commit-bot: I haz the power, 2013-05-24 13:01:13]

Retried try job too often on win_rel for step(s) base_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests,
components_unittests, content_unittests, crypto_unittests, gpu_unittests,
interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests,
net_unittests, ppapi_unittests, printing_unittests, remoting_unittests,
sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-24 23:33:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/14607023/238001

[commit-bot: I haz the power, 2013-05-24 23:34:10]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 530.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
44b709f0766fa633a1ec5c4c1a8675a6d7b9d982..3066c1547f4ed6d35ef1c3142fcf48b002c728ed
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -530,6 +530,9 @@ enum HistogramValue {
   WEBVIEW_INSERTCSS,
   METRICSPRIVATE_GETISCRASHRECORDINGENABLED,
   IDENTITYPRIVATE_GETRESOURCES,
+  FILESYSTEM_RETAINENTRY,
+  FILESYSTEM_ISRESTORABLE,
+  FILESYSTEM_RESTOREENTRY,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-05-24 23:44:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/14607023/257001

[commit-bot: I haz the power, 2013-05-25 14:09:26]

Change committed as 202276