Offline By Default

Offline By Default

Update the shell to try to run applications from the cache
before trying to download those from the web.

If an application is thus run from the cache, the shell
will asynchronously connect the server to update the
application for the next run, if needed.

BUG=https://github.com/domokit/mojo/issues/363
R=ppi@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/d15e3f4b8a536b3b5eb3ae506653dc14d1f48d4a

[qsr, 2015-09-07 15:46:14]

qsr@chromium.org changed reviewers:
+ ppi@chromium.org

[ppi, 2015-09-08 11:56:25]

Please find the first batch of comments regarding url_response_disk_cache_db
below. Will come back with more comments regarding the other pieces.

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.cc:69: bool
IsMetaDataKey(const leveldb::Slice& s) {
Please describe what meta data key is, maybe give an example.

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db.h (right):

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.h:54:
scoped_ptr<Iterator> Iterate() override;
As discussed offline, I'd not implement this interface if no-one is using it.
I'd think it's fine to only allow iteration (or pruning, see below) on the
snapshot, if this is all we need.

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.h:69: void
Delete(LevelDBKeyPtr key);
As discussed offline, can we push the PruneCache logic down here (injecting the
file deletion logic), so that we no longer need to expose Delete and Iterate
methods?

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc
(right):

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:21: db_
= nullptr;
maybe .reset() instead?

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_entry.mojom
(right):

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:10: struct
LevelDBKey {
Please indicate in a comment that these mojom structs are declared only to get
generated serialization code (ie. are not used for ipc).

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:10: struct
LevelDBKey {
This key type can make sense regardless of the particular db backend. How about
"InternalKey" or "InternalEntryKey"?

[qsr, 2015-09-08 13:35:23]

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.cc:69: bool
IsMetaDataKey(const leveldb::Slice& s) {
On 2015/09/08 11:56:24, ppi wrote:
> Please describe what meta data key is, maybe give an example.

Comment added.

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db.h (right):

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.h:54:
scoped_ptr<Iterator> Iterate() override;
On 2015/09/08 11:56:24, ppi wrote:
> As discussed offline, I'd not implement this interface if no-one is using it.
> I'd think it's fine to only allow iteration (or pruning, see below) on the
> snapshot, if this is all we need.

Ack. Won't change after offline discussion. To sum it up, this will be a pure
database class without logic for expiration (that will need to be made more
complex when we will want to keep the cache size controlled). Instead Put/Get
will have more documentation about acting on newest element.

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.h:69: void
Delete(LevelDBKeyPtr key);
On 2015/09/08 11:56:24, ppi wrote:
> As discussed offline, can we push the PruneCache logic down here (injecting
the
> file deletion logic), so that we no longer need to expose Delete and Iterate
> methods?

Acknowledged.

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc
(right):

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:21: db_
= nullptr;
On 2015/09/08 11:56:24, ppi wrote:
> maybe .reset() instead?

We usually assign to scoped_refptr.

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_entry.mojom
(right):

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:10: struct
LevelDBKey {
On 2015/09/08 11:56:24, ppi wrote:
> This key type can make sense regardless of the particular db backend. How
about
> "InternalKey" or "InternalEntryKey"?

Done.

https://codereview.chromium.org/1276073004/diff/50036/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:10: struct
LevelDBKey {
On 2015/09/08 11:56:24, ppi wrote:
> Please indicate in a comment that these mojom structs are declared only to get
> generated serialization code (ie. are not used for ipc).

Done.

[ppi, 2015-09-08 13:45:46]

https://codereview.chromium.org/1276073004/diff/130001/mojo/services/url_resp...
File
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom
(right):

https://codereview.chromium.org/1276073004/diff/130001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:18:
interface URLResponseDiskCache {
As discussed offline, it would be great to rename the methods to better align
with their semantics, e.g. UpdateCache does a subset of what GetFile does, but
you wouldn't guess that from the names.

https://codereview.chromium.org/1276073004/diff/130001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_entry.mojom
(right):

https://codereview.chromium.org/1276073004/diff/130001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:19: string
cache_directory;
Should this be called "client directory"?

https://codereview.chromium.org/1276073004/diff/130001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_impl.cc (right):

https://codereview.chromium.org/1276073004/diff/130001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:54: void
MovePathInDir(const base::FilePath& source,
MovePathIntoDir?

[qsr, 2015-09-08 13:53:40]

https://codereview.chromium.org/1276073004/diff/130001/mojo/services/url_resp...
File
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom
(right):

https://codereview.chromium.org/1276073004/diff/130001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:18:
interface URLResponseDiskCache {
On 2015/09/08 13:45:45, ppi wrote:
> As discussed offline, it would be great to rename the methods to better align
> with their semantics, e.g. UpdateCache does a subset of what GetFile does, but
> you wouldn't guess that from the names.

Done.

https://codereview.chromium.org/1276073004/diff/130001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_entry.mojom
(right):

https://codereview.chromium.org/1276073004/diff/130001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:19: string
cache_directory;
On 2015/09/08 13:45:45, ppi wrote:
> Should this be called "client directory"?

Not sure I agree. This directory is not in the control of the client (a
subdirectory of it is). This is the base directory where everything related to
this entry is stored.

https://codereview.chromium.org/1276073004/diff/130001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_impl.cc (right):

https://codereview.chromium.org/1276073004/diff/130001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:54: void
MovePathInDir(const base::FilePath& source,
On 2015/09/08 13:45:45, ppi wrote:
> MovePathIntoDir?

Done.

[ppi, 2015-09-08 15:46:27]

https://codereview.chromium.org/1276073004/diff/210001/mojo/services/url_resp...
File
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom
(right):

https://codereview.chromium.org/1276073004/diff/210001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:24:
// to store content.
Please clarify when dir at cache_dir_path gets cleared, as in the methods below.

https://codereview.chromium.org/1276073004/diff/210001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:32:
// Given a URLResponse, returns a pair of paths. |file_path| is a file
Given a URLResponse, updates the cache and returns ...

https://codereview.chromium.org/1276073004/diff/210001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:41:
// Given a URLResponse that is expected to have a zipped body, returns a
Given a URLResponse that is expected to have a zipped body, updates the cache
and returns ...

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_apptest.cc
(right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_apptest.cc:54:
TEST_F(URLResponseDiskCacheAppTest, UpdateAndGet) {
Should we have some tests for the new methods too?

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.cc:110: class
DBIterator : public DBReader::Iterator {
Is there any reason not to put the implementations of methods next to the
classes for DBIterator, DBReaderImpl and DBSnapshot?

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.cc:241: // virtual
I know we do this for // static, but grepping for this comment in the codebase
yields no matches. Maybe skip for consistency?

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db.h (right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.h:54:
scoped_ptr<Iterator> Iterate() override;
This, as opposed to Iterate() on a snapshot, returns an iterator for the live
database. Can you document the contract on how this works when the database
changes underneath?

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc
(right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:57:
CacheEntryPtr entry = db_->Get(origin, url);
can you put two entries and verify that a newer was returned?

btw. if the method was called GetNewest() it would have been more prominent that
we should test that property too.

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:64:
TEST_F(URLResponseDiskCacheDBTest, Iterator) {
can you verify the ordering contract?

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:104:
std::string url2 = "url";
did you mean this to be a different url from |url|?

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_entry.mojom
(right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:11: struct
InternalEntryKey {
Since we expose this in the iterator interface, might as well call this
CacheKey.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/application_manager.cc:190:
&authenticating_network_service_);
Why this change is needed?

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
File shell/application_manager/network_fetcher.cc (right):

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.cc:53: auto header =
mojo::HttpHeader::New();
this should probably be called architecture_header

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.cc:62: 
Please document the lifetime.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.cc:67: mojo::NetworkService*
network_service);
Why not take the delay as an argument? I can imagine wanting to set it to 0 for
testing.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.cc:294: if (!PathExists(map_path))
As you are using the if-else style with braces in new code in this file, can you
update this?

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
File shell/application_manager/network_fetcher.h (right):

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.h:51: void LoadFromCache(bool
initial);
|initial| doesn't say a lot about how this flag will affect execution. Should we
say "sheduleUpdate"?

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.h:53: void
OnCachedResponseReceived(bool initial,
Same here.

[qsr, 2015-09-11 15:47:58]

https://codereview.chromium.org/1276073004/diff/210001/mojo/services/url_resp...
File
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom
(right):

https://codereview.chromium.org/1276073004/diff/210001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:24:
// to store content.
On 2015/09/08 15:46:25, ppi wrote:
> Please clarify when dir at cache_dir_path gets cleared, as in the methods
below.

Done.

https://codereview.chromium.org/1276073004/diff/210001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:32:
// Given a URLResponse, returns a pair of paths. |file_path| is a file
On 2015/09/08 15:46:25, ppi wrote:
> Given a URLResponse, updates the cache and returns ...

Done.

https://codereview.chromium.org/1276073004/diff/210001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:41:
// Given a URLResponse that is expected to have a zipped body, returns a
On 2015/09/08 15:46:26, ppi wrote:
> Given a URLResponse that is expected to have a zipped body, updates the cache
> and returns ...

Done.

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_apptest.cc
(right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_apptest.cc:54:
TEST_F(URLResponseDiskCacheAppTest, UpdateAndGet) {
On 2015/09/08 15:46:26, ppi wrote:
> Should we have some tests for the new methods too?

It is extremely hard to do. Update is completely asynchronous, and best effort.
In practice, unless I wait for a long time, I have no way to know when a Get()
will return a value.

I updated the test with a 1s wait, but I really do not like this.

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.cc:110: class
DBIterator : public DBReader::Iterator {
On 2015/09/08 15:46:26, ppi wrote:
> Is there any reason not to put the implementations of methods next to the
> classes for DBIterator, DBReaderImpl and DBSnapshot?

I usually put all my declaration first and the implementation after. But changed
as suggested.

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.cc:241: // virtual
On 2015/09/08 15:46:26, ppi wrote:
> I know we do this for // static, but grepping for this comment in the codebase
> yields no matches. Maybe skip for consistency?

Done.

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db.h (right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.h:54:
scoped_ptr<Iterator> Iterate() override;
On 2015/09/08 15:46:26, ppi wrote:
> This, as opposed to Iterate() on a snapshot, returns an iterator for the live
> database. Can you document the contract on how this works when the database
> changes underneath?

Done.

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc
(right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:57:
CacheEntryPtr entry = db_->Get(origin, url);
On 2015/09/08 15:46:26, ppi wrote:
> can you put two entries and verify that a newer was returned?
> 
> btw. if the method was called GetNewest() it would have been more prominent
that
> we should test that property too.

Renamed and Done.

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:64:
TEST_F(URLResponseDiskCacheDBTest, Iterator) {
On 2015/09/08 15:46:26, ppi wrote:
> can you verify the ordering contract?

Done.

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:104:
std::string url2 = "url";
On 2015/09/08 15:46:26, ppi wrote:
> did you mean this to be a different url from |url|?

Yes. Done.

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_entry.mojom
(right):

https://codereview.chromium.org/1276073004/diff/210001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:11: struct
InternalEntryKey {
On 2015/09/08 15:46:26, ppi wrote:
> Since we expose this in the iterator interface, might as well call this
> CacheKey.

Done.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/application_manager.cc:190:
&authenticating_network_service_);
On 2015/09/08 15:46:26, ppi wrote:
> Why this change is needed?

Because otherwise the network service is changed in the middle of the
initialization sequence, and it blocks because it expects an interceptor that
cannot be initialized correctly.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
File shell/application_manager/network_fetcher.cc (right):

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.cc:53: auto header =
mojo::HttpHeader::New();
On 2015/09/08 15:46:26, ppi wrote:
> this should probably be called architecture_header

Done.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.cc:62: 
On 2015/09/08 15:46:26, ppi wrote:
> Please document the lifetime.

Done.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.cc:67: mojo::NetworkService*
network_service);
On 2015/09/08 15:46:26, ppi wrote:
> Why not take the delay as an argument? I can imagine wanting to set it to 0
for
> testing.

Done.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.cc:294: if (!PathExists(map_path))
On 2015/09/08 15:46:26, ppi wrote:
> As you are using the if-else style with braces in new code in this file, can
you
> update this?

Done.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
File shell/application_manager/network_fetcher.h (right):

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.h:51: void LoadFromCache(bool
initial);
On 2015/09/08 15:46:26, ppi wrote:
> |initial| doesn't say a lot about how this flag will affect execution. Should
we
> say "sheduleUpdate"?

Done.

https://codereview.chromium.org/1276073004/diff/210001/shell/application_mana...
shell/application_manager/network_fetcher.h:53: void
OnCachedResponseReceived(bool initial,
On 2015/09/08 15:46:26, ppi wrote:
> Same here.

Done.

[ppi, 2015-09-15 15:21:23]

https://codereview.chromium.org/1276073004/diff/270001/mojo/devtools/common/m...
File mojo/devtools/common/mojo_test (right):

https://codereview.chromium.org/1276073004/diff/270001/mojo/devtools/common/m...
mojo/devtools/common/mojo_test:70:
common_shell_args.append("--args-for=mojo:url_response_disk_cache --clear")
good idea! Would you mind extracting the url into _CACHE_SERVICE_URL global as
we do in mojo_benchmark?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.cc:127: void
URLResponseDiskCacheDB::Iterator::GetNext(CacheKeyPtr* key,
Do you want to either document that HasNext() needs to be called before
GetNext() to skip metadata, or rewind past the metadata here, too?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.cc:156:
memcpy(&version, value.data(), sizeof(version));
should we include <string.h> for memcpy?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db.h (right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.h:50: void
PutNewest(const std::string& request_origin, const std::string& url,
nit: I'd name these methods:

 - PutNew
 - GetNewest (as it is right now)

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.h:65:
scoped_ptr<Iterator> Iterate();
nit: maybe call this conservatively GetIterator()?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc
(right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:79:
TEST_F(URLResponseDiskCacheDBTest, Iterator) {
here we are putting the entries in the same order they appear in the iterator.
Could you test that entries put out of order appear in order in the iterator?
e.g. with urls "b" "a" "c".

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_entry.mojom
(right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:20: string
cache_directory;
"cache_directory" is very general. If this dir is entry-specific, should we call
this "entry_directory"?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:21: string
content_path;
wdyt about "response_body_path"?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_impl.cc (right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:46: std::string
GetNewIdentifier() {
What is the identifier for? If this is to identify pending update requests, can
we say that in the comment? Should we call the function
GetNewUpdateIdentifier()? Or GetNewUpdateId()?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:122: // to
prevent leaking files if there is an interuption while downloading a
s/interuption/interruption/

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:135: // Returns
the path of the sentinel used to keep track of a zipped response has
s/used to keep track of/used to mark that/

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:137:
base::FilePath GetExtractedSentinel(const base::FilePath& cache_directory) {
nit: wdyt about GetExtractionSentinel?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:141: // Returns
the path of the directory when a zipped content is extracted.
s/when/where/

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:142:
base::FilePath GetExtractedDirectory(const base::FilePath& cache_directory) {
nit: GetExtractionDirectiory?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:142:
base::FilePath GetExtractedDirectory(const base::FilePath& cache_directory) {
if "cache_directory" here is per-entry, I'd suggest calling it
"entry_directory". "cache_directory" sounds global.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:165:
base::FilePath staging_content = GetStagingDirectory().Append(identifier);
nit: how about "staged_content_path"?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:166:
base::FilePath cache_directory = GetCacheDirectory().Append(identifier);
nit: how about "entry_directory"?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:167:
base::FilePath content_path = cache_directory.Append(identifier);
nit: how about "target_content_path"?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:243: void
PruneCache(scoped_refptr<URLResponseDiskCacheDB> db,
"Prune" usually means reduce to meet some limit. Should we call this "EvictAll"
?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:294: // Move
the database to trash.
should these two be in 

 if (force_clean) {

?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:367: if
(PathExists(GetExtractedSentinel(cache_directory))) {
Should we invert the flow to match the other if above?

if (there is an issue) {
  UpdateAndGetInternal(...)
}

callback.Run(...)

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:407:
base::FilePath staging_content = GetStagingDirectory().Append(identifier);
s/staging_content/stage_at_path/ maybe?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:413: //
database only if  the copy of the body succeded.
nit: remove one of the two spaces after if:)

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:432:
base::FilePath cache_directory = consumer_cache_directory.DirName();
I'd also call this entry_directory here.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:433:
base::FilePath extracted_directory = GetExtractedDirectory(cache_directory);
I'd s/extracted/extraction/ here and below.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_impl.h (left):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.h:18: using
FilePathPairCallback =
nit: wdyt about calling this by meaning, ie. ResponseFileAndCacheDirCallback?

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_impl.h (right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.h:23: // Create
the disk cache database. If |force_clean| is true, or the database
nit: Creates

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.h:47: // As
|UpdateAndGet|, but uses FilePath instead of mojo arrays.
nit: this comment forces the reader to go back all the way to the mojom file to
know the meaning of the FilePaths.

I'd suggest saying:
"Internal implementation of |UpdateAndGet| using a pair of base::FilePath to
represent the paths the the response body and to the per-response client cache
directory."

https://codereview.chromium.org/1276073004/diff/270001/shell/application_mana...
File shell/application_manager/network_fetcher.h (right):

https://codereview.chromium.org/1276073004/diff/270001/shell/application_mana...
shell/application_manager/network_fetcher.h:51: void LoadFromCache(bool
sheduleUpdate);
We probably want "shedule_update" (I see that I got it wrong in the comment,
sorry about that!)

[qsr, 2015-09-16 11:46:39]

https://codereview.chromium.org/1276073004/diff/270001/mojo/devtools/common/m...
File mojo/devtools/common/mojo_test (right):

https://codereview.chromium.org/1276073004/diff/270001/mojo/devtools/common/m...
mojo/devtools/common/mojo_test:70:
common_shell_args.append("--args-for=mojo:url_response_disk_cache --clear")
On 2015/09/15 15:21:22, ppi wrote:
> good idea! Would you mind extracting the url into _CACHE_SERVICE_URL global as
> we do in mojo_benchmark?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.cc:127: void
URLResponseDiskCacheDB::Iterator::GetNext(CacheKeyPtr* key,
On 2015/09/15 15:21:22, ppi wrote:
> Do you want to either document that HasNext() needs to be called before
> GetNext() to skip metadata, or rewind past the metadata here, too?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.cc:156:
memcpy(&version, value.data(), sizeof(version));
On 2015/09/15 15:21:22, ppi wrote:
> should we include <string.h> for memcpy?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db.h (right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.h:50: void
PutNewest(const std::string& request_origin, const std::string& url,
On 2015/09/15 15:21:22, ppi wrote:
> nit: I'd name these methods:
> 
>  - PutNew
>  - GetNewest (as it is right now)

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db.h:65:
scoped_ptr<Iterator> Iterate();
On 2015/09/15 15:21:22, ppi wrote:
> nit: maybe call this conservatively GetIterator()?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc
(right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:79:
TEST_F(URLResponseDiskCacheDBTest, Iterator) {
On 2015/09/15 15:21:22, ppi wrote:
> here we are putting the entries in the same order they appear in the iterator.
> Could you test that entries put out of order appear in order in the iterator?
> e.g. with urls "b" "a" "c".

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_entry.mojom
(right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:20: string
cache_directory;
On 2015/09/15 15:21:22, ppi wrote:
> "cache_directory" is very general. If this dir is entry-specific, should we
call
> this "entry_directory"?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_entry.mojom:21: string
content_path;
On 2015/09/15 15:21:22, ppi wrote:
> wdyt about "response_body_path"?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_impl.cc (right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:46: std::string
GetNewIdentifier() {
On 2015/09/15 15:21:23, ppi wrote:
> What is the identifier for? If this is to identify pending update requests,
can
> we say that in the comment? Should we call the function
> GetNewUpdateIdentifier()? Or GetNewUpdateId()?

Added comment. This is not to identify a request -> it is an unique id that
serves to create the entry directory. I cannot use the safer method of creating
a directory because I need to create it in the staging directory and then move
it to the final one.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:122: // to
prevent leaking files if there is an interuption while downloading a
On 2015/09/15 15:21:23, ppi wrote:
> s/interuption/interruption/

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:135: // Returns
the path of the sentinel used to keep track of a zipped response has
On 2015/09/15 15:21:22, ppi wrote:
> s/used to keep track of/used to mark that/

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:137:
base::FilePath GetExtractedSentinel(const base::FilePath& cache_directory) {
On 2015/09/15 15:21:23, ppi wrote:
> nit: wdyt about GetExtractionSentinel?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:141: // Returns
the path of the directory when a zipped content is extracted.
On 2015/09/15 15:21:23, ppi wrote:
> s/when/where/

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:142:
base::FilePath GetExtractedDirectory(const base::FilePath& cache_directory) {
On 2015/09/15 15:21:23, ppi wrote:
> if "cache_directory" here is per-entry, I'd suggest calling it
> "entry_directory". "cache_directory" sounds global.

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:142:
base::FilePath GetExtractedDirectory(const base::FilePath& cache_directory) {
On 2015/09/15 15:21:23, ppi wrote:
> nit: GetExtractionDirectiory?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:165:
base::FilePath staging_content = GetStagingDirectory().Append(identifier);
On 2015/09/15 15:21:22, ppi wrote:
> nit: how about "staged_content_path"?

Used staged_response_body_path

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:166:
base::FilePath cache_directory = GetCacheDirectory().Append(identifier);
On 2015/09/15 15:21:23, ppi wrote:
> nit: how about "entry_directory"?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:167:
base::FilePath content_path = cache_directory.Append(identifier);
On 2015/09/15 15:21:23, ppi wrote:
> nit: how about "target_content_path"?

Used response_body_path

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:243: void
PruneCache(scoped_refptr<URLResponseDiskCacheDB> db,
On 2015/09/15 15:21:22, ppi wrote:
> "Prune" usually means reduce to meet some limit. Should we call this
"EvictAll"
> ?

Which is what we do. We keep an entry per key.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:294: // Move
the database to trash.
On 2015/09/15 15:21:23, ppi wrote:
> should these two be in 
> 
>  if (force_clean) {
> 
> ?

No. If we do not want to clean the db, we would have returned on line 289. At
this point we need to clean the database, whether because it was forced, or
because it was of an other version.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:367: if
(PathExists(GetExtractedSentinel(cache_directory))) {
On 2015/09/15 15:21:23, ppi wrote:
> Should we invert the flow to match the other if above?
> 
> if (there is an issue) {
>   UpdateAndGetInternal(...)
> }
> 
> callback.Run(...)

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:407:
base::FilePath staging_content = GetStagingDirectory().Append(identifier);
On 2015/09/15 15:21:23, ppi wrote:
> s/staging_content/stage_at_path/ maybe?

Used staged_response_body_path

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:413: //
database only if  the copy of the body succeded.
On 2015/09/15 15:21:23, ppi wrote:
> nit: remove one of the two spaces after if:)

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:432:
base::FilePath cache_directory = consumer_cache_directory.DirName();
On 2015/09/15 15:21:23, ppi wrote:
> I'd also call this entry_directory here.

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:433:
base::FilePath extracted_directory = GetExtractedDirectory(cache_directory);
On 2015/09/15 15:21:23, ppi wrote:
> I'd s/extracted/extraction/ here and below.

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_impl.h (left):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.h:18: using
FilePathPairCallback =
On 2015/09/15 15:21:23, ppi wrote:
> nit: wdyt about calling this by meaning, ie. ResponseFileAndCacheDirCallback?

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_impl.h (right):

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.h:23: // Create
the disk cache database. If |force_clean| is true, or the database
On 2015/09/15 15:21:23, ppi wrote:
> nit: Creates

Done.

https://codereview.chromium.org/1276073004/diff/270001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.h:47: // As
|UpdateAndGet|, but uses FilePath instead of mojo arrays.
On 2015/09/15 15:21:23, ppi wrote:
> nit: this comment forces the reader to go back all the way to the mojom file
to
> know the meaning of the FilePaths.
> 
> I'd suggest saying:
> "Internal implementation of |UpdateAndGet| using a pair of base::FilePath to
> represent the paths the the response body and to the per-response client cache
> directory." 

Done.

https://codereview.chromium.org/1276073004/diff/270001/shell/application_mana...
File shell/application_manager/network_fetcher.h (right):

https://codereview.chromium.org/1276073004/diff/270001/shell/application_mana...
shell/application_manager/network_fetcher.h:51: void LoadFromCache(bool
sheduleUpdate);
On 2015/09/15 15:21:23, ppi wrote:
> We probably want "shedule_update" (I see that I got it wrong in the comment,
> sorry about that!)

Done.

[ppi, 2015-09-16 12:32:28]

lgtm % comments

https://codereview.chromium.org/1276073004/diff/290001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc
(right):

https://codereview.chromium.org/1276073004/diff/290001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:83:
std::string url3 = "b";
did you mean either url2 or url3 to be "c"?

https://codereview.chromium.org/1276073004/diff/290001/shell/application_mana...
File shell/application_manager/network_fetcher.h (right):

https://codereview.chromium.org/1276073004/diff/290001/shell/application_mana...
shell/application_manager/network_fetcher.h:51: void LoadFromCache(bool
scheduleUpdate);
Good catch with the spelling, but I meant casing -> schedule_update, and same in
the .cc file please.

[qsr, 2015-09-16 13:50:29]

https://codereview.chromium.org/1276073004/diff/290001/services/url_response_...
File services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc
(right):

https://codereview.chromium.org/1276073004/diff/290001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_db_unittests.cc:83:
std::string url3 = "b";
On 2015/09/16 12:32:27, ppi wrote:
> did you mean either url2 or url3 to be "c"?

Done.

[qsr, 2015-09-16 13:52:12]

https://codereview.chromium.org/1276073004/diff/290001/shell/application_mana...
File shell/application_manager/network_fetcher.h (right):

https://codereview.chromium.org/1276073004/diff/290001/shell/application_mana...
shell/application_manager/network_fetcher.h:51: void LoadFromCache(bool
scheduleUpdate);
On 2015/09/16 12:32:27, ppi wrote:
> Good catch with the spelling, but I meant casing -> schedule_update, and same
in
> the .cc file please.

Done.

[qsr, 2015-09-17 09:26:22]

Committed patchset #18 (id:330001) manually as
d15e3f4b8a536b3b5eb3ae506653dc14d1f48d4a (presubmit successful).