Adding URLResponse Disk Cache to mojo.

Adding URLResponse Disk Cache to mojo.

This CL introduces a url response disk cache that allows the shell and content
handlers to access cached response data as file without the need to
stream the full content nor create a new file. It also allows
content handlers to access uncompressed cached archive.

The cache service is loaded internally by the shell because it is used
during application loads.

The android handler is using this service to cache the uncompressed archive of
android applications and the compiled dex files

R=blundell@chromium.org, davemoore@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/5d429aa56783d74fc94ec9dcb26e95c0d346ec33

[qsr, 2015-04-29 15:22:28]

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

[qsr, 2015-04-29 15:22:29]

Dave -> Can you do a first pass review of this?

This is missing comment, and an app tests, but the overall principles are there.

Thanks

[DaveMoore, 2015-05-05 21:20:59]

https://codereview.chromium.org/1088533003/diff/160001/mojo/services/service_...
File mojo/services/service_cache/public/interfaces/service_cache.mojom (right):

https://codereview.chromium.org/1088533003/diff/160001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:20:
(array<uint8>? file_path, array<uint8>? cache_dir_path);
Why not String?

https://codereview.chromium.org/1088533003/diff/160001/services/service_cache...
File services/service_cache/BUILD.gn (right):

https://codereview.chromium.org/1088533003/diff/160001/services/service_cache...
services/service_cache/BUILD.gn:10: source_set("service_cache") {
Why not a native application?

https://codereview.chromium.org/1088533003/diff/160001/services/service_cache...
File services/service_cache/service_cache_app.h (right):

https://codereview.chromium.org/1088533003/diff/160001/services/service_cache...
services/service_cache/service_cache_app.h:16: namespace service_cache {
Although we're not consistent everywhere I think it's better to have this just
in the mojo namespace

https://codereview.chromium.org/1088533003/diff/160001/shell/android/android_...
File shell/android/android_handler.cc (right):

https://codereview.chromium.org/1088533003/diff/160001/shell/android/android_...
shell/android/android_handler.cc:110: base::RunLoop().Run();
What makes this runloop exit?

[qsr, 2015-05-06 08:13:49]

https://codereview.chromium.org/1088533003/diff/160001/mojo/services/service_...
File mojo/services/service_cache/public/interfaces/service_cache.mojom (right):

https://codereview.chromium.org/1088533003/diff/160001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:20:
(array<uint8>? file_path, array<uint8>? cache_dir_path);
On 2015/05/05 21:20:59, DaveMoore wrote:
> Why not String?

Because String is utf8 encoded, and there is absolutely no reason to think that
the encoding of the fs will be utf8.

https://codereview.chromium.org/1088533003/diff/160001/services/service_cache...
File services/service_cache/BUILD.gn (right):

https://codereview.chromium.org/1088533003/diff/160001/services/service_cache...
services/service_cache/BUILD.gn:10: source_set("service_cache") {
On 2015/05/05 21:20:59, DaveMoore wrote:
> Why not a native application?

Because this is used as part of application loading, so needs to be bundled in
the shell.

https://codereview.chromium.org/1088533003/diff/160001/services/service_cache...
File services/service_cache/service_cache_app.h (right):

https://codereview.chromium.org/1088533003/diff/160001/services/service_cache...
services/service_cache/service_cache_app.h:16: namespace service_cache {
On 2015/05/05 21:20:59, DaveMoore wrote:
> Although we're not consistent everywhere I think it's better to have this just
> in the mojo namespace

 I thought we wanted to stop putting everything in the mojo namespace. Isn't it
the case in the end?

https://codereview.chromium.org/1088533003/diff/160001/shell/android/android_...
File shell/android/android_handler.cc (right):

https://codereview.chromium.org/1088533003/diff/160001/shell/android/android_...
shell/android/android_handler.cc:110: base::RunLoop().Run();
On 2015/05/05 21:20:59, DaveMoore wrote:
> What makes this runloop exit?

 In the previous line, the callback to ExtractApplication is:
 base::Bind(&RunClosure, loop.task_runner(),
base::MessageLoop::QuitWhenIdleClosure())));

Changed to not need the intermediary RunClosure function.

[DaveMoore, 2015-05-06 22:31:21]

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
File services/service_cache/service_cache_impl.cc (right):

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
services/service_cache/service_cache_impl.cc:89: void RunCallbackWithSucess(
nit: Should be RunCallbackWithSuccess

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
services/service_cache/service_cache_impl.cc:147: if
(!ReadFileToString(entry_path, &serialized_entry))
Is it worth reading the whole file up front? Doesn't that make cache misses more
expensive? Maybe that's better than two separate reads on hits, but shouldn't
there be a way to keep the file open and only read up to the end of the headers
first and then the rest after checking. Or does that break the serialization?

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
services/service_cache/service_cache_impl.cc:220: base::FilePath dir =
GetDirName(base_directory_, response->url);
I'm not sure about the speed of directory traversal on Android. It might be
better to encode the url into something smaller. Just a TODO for now.

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
services/service_cache/service_cache_impl.cc:221: if (dir.empty()) {
Why would the GetDirName() fail?

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

https://codereview.chromium.org/1088533003/diff/180001/shell/application_mana...
shell/application_manager/network_fetcher.cc:219: mojo::URLRequestPtr
request(mojo::URLRequest::New());
I thought the point of etags was that you sent them and the last date fetched in
the request and then could get a response that indicated that there was no
change. But I don't see any place where we first look for the tag / date and add
that to the request.

[qsr, 2015-05-07 08:40:55]

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
File services/service_cache/service_cache_impl.cc (right):

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
services/service_cache/service_cache_impl.cc:89: void RunCallbackWithSucess(
On 2015/05/06 22:31:21, DaveMoore wrote:
> nit: Should be RunCallbackWithSuccess

Done.

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
services/service_cache/service_cache_impl.cc:147: if
(!ReadFileToString(entry_path, &serialized_entry))
On 2015/05/06 22:31:21, DaveMoore wrote:
> Is it worth reading the whole file up front? Doesn't that make cache misses
more
> expensive? Maybe that's better than two separate reads on hits, but shouldn't
> there be a way to keep the file open and only read up to the end of the
headers
> first and then the rest after checking. Or does that break the serialization?

I don't expect this file to be very long. In practice, right now, is it less
than 1k.

Also, I do not even know how to do what you are suggesting. This file is a
serialized mojo struct. We do not support partial deserialization at this point.

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
services/service_cache/service_cache_impl.cc:220: base::FilePath dir =
GetDirName(base_directory_, response->url);
On 2015/05/06 22:31:21, DaveMoore wrote:
> I'm not sure about the speed of directory traversal on Android. It might be
> better to encode the url into something smaller. Just a TODO for now.

Done.

https://codereview.chromium.org/1088533003/diff/180001/services/service_cache...
services/service_cache/service_cache_impl.cc:221: if (dir.empty()) {
On 2015/05/06 22:31:21, DaveMoore wrote:
> Why would the GetDirName() fail?

 First version was creating the directory if it didn't exist. Removed.

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

https://codereview.chromium.org/1088533003/diff/180001/shell/application_mana...
shell/application_manager/network_fetcher.cc:219: mojo::URLRequestPtr
request(mojo::URLRequest::New());
On 2015/05/06 22:31:21, DaveMoore wrote:
> I thought the point of etags was that you sent them and the last date fetched
in
> the request and then could get a response that indicated that there was no
> change. But I don't see any place where we first look for the tag / date and
add
> that to the request.

The network service is doing all of this for us. Also ETags do not use date,
just the hermetic identifier.

[blundell, 2015-05-07 11:29:19]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2015-05-07 11:29:22]

Drive-by :)

As a general note, the CL would benefit from more comments, especially in the
Android code and the service cache impl.

The CL description could also be expanded:
- Mention that the ServiceCache is loaded internally by the shell.
- Mention somewhere the policy on separating cached url response ptrs by origin
of the application that connected to the ServiceCache.
- Mention that the AndroidHandler is changed to use the new mechanisms, in
particular its use of the cache dir.

I didn't look at the apptests in this pass.

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
File mojo/services/service_cache/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/BUILD.gn:25: mojo_sdk_deps = [
"mojo/public/interfaces/application" ]
This looks like it's unneeded.

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
File mojo/services/service_cache/public/interfaces/service_cache.mojom (right):

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:9: // This
service allows the mojo shell and content handler to efficiently cache
I would expand on this comment to reflect our offline discussion that this is
really about caching URLResponses on disk.

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:12: // also
handles all the other http cache mechanisms.
I would expand on this to be more explicit about what the current policy is on
when the cached entry will be invalidated. IIUC, it's this:
- If the response and the cached entry have etags, the etag contents will be
used to determine whether to invalidate the entry.
- In all other cases, the cached entry will always be regarded as invalid.

Is that correct? In other words, the lack of handling of other caching
mechanisms is a performance issue, not a correctness issue.

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:15: // Given a
URLResponse, returns a pair of path. |file_path| is a file
s/path/paths

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:15: // Given a
URLResponse, returns a pair of path. |file_path| is a file
nit: s/path/paths

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:17: // the
applicaton can use to store content. This service guarantee that
Since it's a little opaque why |cache_dir_path| is involved, I would give an
example usage of this ("e.g., a content handler that is backed by a VM that
compiles files could have the VM use this directory to cache the compiled
files").

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
File services/service_cache/service_cache_impl.cc (right):

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:33: Serialize_(input.Pass(), &buf,
&data_type);
The usage of these internal SDK functions is a little suspicious. Is there a
particular reason that the cache entry has to be a mojom struct? For example,
what about using //base/pickle?

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:102: std::string serialize_entry;
nit: s/serialize_entry/serialized_entry

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:125: while (begin < end && *begin
== ' ')
Can you use //base/string_util's TrimString() function here to make this
cleaner?

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:167: // Looking for the first etag
header.
Will there ever be more than 1 etag header? What's the semantics in that case?

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:202: if (dir.empty()) {
Why won't this be the case the first time that this method is called for
|response|?

https://codereview.chromium.org/1088533003/diff/200001/shell/android/android_...
File shell/android/android_handler.cc (right):

https://codereview.chromium.org/1088533003/diff/200001/shell/android/android_...
shell/android/android_handler.cc:97: base::MessageLoop loop;
Any reason not to put the stuff from line 110 on in a separate method and use
PostTaskAndReply()?

https://codereview.chromium.org/1088533003/diff/200001/shell/android/apk/src/...
File shell/android/apk/src/org/chromium/mojo/shell/AndroidHandler.java (right):

https://codereview.chromium.org/1088533003/diff/200001/shell/android/apk/src/...
shell/android/apk/src/org/chromium/mojo/shell/AndroidHandler.java:83: // Rename
the bootstrap library to prevent dlopen to think it is alread opened.
Why is this necessary now?

https://codereview.chromium.org/1088533003/diff/200001/shell/application_mana...
File shell/application_manager/local_fetcher.cc (right):

https://codereview.chromium.org/1088533003/diff/200001/shell/application_mana...
shell/application_manager/local_fetcher.cc:67: "ETag: \"%" PRId64 "-%" PRId64
"\"", stat_result.st_ino,
Should this include st_dev as well for completeness?

http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html

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

https://codereview.chromium.org/1088533003/diff/200001/shell/application_mana...
shell/application_manager/network_fetcher.cc:184: // TODO(qsr) Test is not
enough anymore -> we do not have anything while
This seems like a bug waiting to happen, unless it's somehow an expected part of
the contract that this function will only be called once (which it doesn't seem
to be). Maybe add a DCHECK that this function isn't called more than once and
place this TODO on that DCHECK?

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

https://codereview.chromium.org/1088533003/diff/200001/shell/application_mana...
shell/application_manager/network_fetcher.h:55: void CacheFileRetrieved(
nit: I would call this |OnFileRetrievedFromCache| to avoid any ambiguity as to
what this function is doing.

[blundell, 2015-05-07 11:33:20]

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
File services/service_cache/service_cache_impl.cc (right):

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:202: if (dir.empty()) {
On 2015/05/07 11:29:21, blundell wrote:
> Why won't this be the case the first time that this method is called for
> |response|?

OK, in the case of a content-handled application it won't since the shell
already fetched the initial URLResponse (i.e., with the shbang prepended) via
GetFile(). That doesn't seem like it generalizes though?

[qsr, 2015-05-07 12:49:02]

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
File mojo/services/service_cache/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/BUILD.gn:25: mojo_sdk_deps = [
"mojo/public/interfaces/application" ]
On 2015/05/07 11:29:21, blundell wrote:
> This looks like it's unneeded.

Done.

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
File mojo/services/service_cache/public/interfaces/service_cache.mojom (right):

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:9: // This
service allows the mojo shell and content handler to efficiently cache
On 2015/05/07 11:29:21, blundell wrote:
> I would expand on this comment to reflect our offline discussion that this is
> really about caching URLResponses on disk.

Done.

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:12: // also
handles all the other http cache mechanisms.
On 2015/05/07 11:29:21, blundell wrote:
> I would expand on this to be more explicit about what the current policy is on
> when the cached entry will be invalidated. IIUC, it's this:
> - If the response and the cached entry have etags, the etag contents will be
> used to determine whether to invalidate the entry.
> - In all other cases, the cached entry will always be regarded as invalid.
> 
> Is that correct? In other words, the lack of handling of other caching
> mechanisms is a performance issue, not a correctness issue.

Comment extended. And yes this is correct, the cache is correct, but could cache
more.

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:15: // Given a
URLResponse, returns a pair of path. |file_path| is a file
On 2015/05/07 11:29:21, blundell wrote:
> nit: s/path/paths

Done.

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:15: // Given a
URLResponse, returns a pair of path. |file_path| is a file
On 2015/05/07 11:29:21, blundell wrote:
> s/path/paths

Done.

https://codereview.chromium.org/1088533003/diff/200001/mojo/services/service_...
mojo/services/service_cache/public/interfaces/service_cache.mojom:17: // the
applicaton can use to store content. This service guarantee that
On 2015/05/07 11:29:21, blundell wrote:
> Since it's a little opaque why |cache_dir_path| is involved, I would give an
> example usage of this ("e.g., a content handler that is backed by a VM that
> compiles files could have the VM use this directory to cache the compiled
> files").

Done.

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
File services/service_cache/service_cache_impl.cc (right):

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:33: Serialize_(input.Pass(), &buf,
&data_type);
On 2015/05/07 11:29:21, blundell wrote:
> The usage of these internal SDK functions is a little suspicious. Is there a
> particular reason that the cache entry has to be a mojom struct? For example,
> what about using //base/pickle?

 Versioning might be useful.

 Also, we have plan to make serializing/deserializing mojo struct more a first
class notion. I would move this code to use whatever comes from there when it
exists.

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:102: std::string serialize_entry;
On 2015/05/07 11:29:21, blundell wrote:
> nit: s/serialize_entry/serialized_entry

Done.

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:125: while (begin < end && *begin
== ' ')
On 2015/05/07 11:29:21, blundell wrote:
> Can you use //base/string_util's TrimString() function here to make this
> cleaner?

 I could. But this would be a lot less efficient, as I would need to create a
bunch of intermediary strings with copy all over the place...

 To be noted -> jam is planning to change this API so that headers will be a
struct with a name and a value. When this happens, this code will change for the
better.

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:167: // Looking for the first etag
header.
On 2015/05/07 11:29:21, blundell wrote:
> Will there ever be more than 1 etag header? What's the semantics in that case?

 There is no defined semantic when there is more than one etag header. Chrome is
also looking for the first one.

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:202: if (dir.empty()) {
On 2015/05/07 11:33:20, blundell wrote:
> On 2015/05/07 11:29:21, blundell wrote:
> > Why won't this be the case the first time that this method is called for
> > |response|?
> 
> OK, in the case of a content-handled application it won't since the shell
> already fetched the initial URLResponse (i.e., with the shbang prepended) via
> GetFile(). That doesn't seem like it generalizes though?

Removed this test as this cannot happen. empty() has not the semantic you think
-> it just check that the path itself is not empty, not that the path exists on
disk.

https://codereview.chromium.org/1088533003/diff/200001/services/service_cache...
services/service_cache/service_cache_impl.cc:202: if (dir.empty()) {
On 2015/05/07 11:33:20, blundell wrote:
> On 2015/05/07 11:29:21, blundell wrote:
> > Why won't this be the case the first time that this method is called for
> > |response|?
> 
> OK, in the case of a content-handled application it won't since the shell
> already fetched the initial URLResponse (i.e., with the shbang prepended) via
> GetFile(). That doesn't seem like it generalizes though?

Acknowledged.

https://codereview.chromium.org/1088533003/diff/200001/shell/android/android_...
File shell/android/android_handler.cc (right):

https://codereview.chromium.org/1088533003/diff/200001/shell/android/android_...
shell/android/android_handler.cc:97: base::MessageLoop loop;
On 2015/05/07 11:29:22, blundell wrote:
> Any reason not to put the stuff from line 110 on in a separate method and use
> PostTaskAndReply()?

Because the runloop must die after line 108. We do not want an active runloop
after this, as the thread must be given to the android application.

https://codereview.chromium.org/1088533003/diff/200001/shell/android/apk/src/...
File shell/android/apk/src/org/chromium/mojo/shell/AndroidHandler.java (right):

https://codereview.chromium.org/1088533003/diff/200001/shell/android/apk/src/...
shell/android/apk/src/org/chromium/mojo/shell/AndroidHandler.java:83: // Rename
the bootstrap library to prevent dlopen to think it is alread opened.
On 2015/05/07 11:29:22, blundell wrote:
> Why is this necessary now?

Because you can have multiple android application running in parallel, and they
all need their own version of bootstrap.so, and if you try to dlopen a second
file with the same basename as one that is already loaded, ldopen will gladly
give you back a reference to the first one, which will be a bug in our case.

https://codereview.chromium.org/1088533003/diff/200001/shell/application_mana...
File shell/application_manager/local_fetcher.cc (right):

https://codereview.chromium.org/1088533003/diff/200001/shell/application_mana...
shell/application_manager/local_fetcher.cc:67: "ETag: \"%" PRId64 "-%" PRId64
"\"", stat_result.st_ino,
On 2015/05/07 11:29:22, blundell wrote:
> Should this include st_dev as well for completeness?
> 
> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html

mtime is already a little overkill, but I guess this doesn't cost that much.
Done

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

https://codereview.chromium.org/1088533003/diff/200001/shell/application_mana...
shell/application_manager/network_fetcher.cc:184: // TODO(qsr) Test is not
enough anymore -> we do not have anything while
On 2015/05/07 11:29:22, blundell wrote:
> This seems like a bug waiting to happen, unless it's somehow an expected part
of
> the contract that this function will only be called once (which it doesn't
seem
> to be). Maybe add a DCHECK that this function isn't called more than once and
> place this TODO on that DCHECK?

 Removed the TODO, and added a DCHECK.

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

https://codereview.chromium.org/1088533003/diff/200001/shell/application_mana...
shell/application_manager/network_fetcher.h:55: void CacheFileRetrieved(
On 2015/05/07 11:29:22, blundell wrote:
> nit: I would call this |OnFileRetrievedFromCache| to avoid any ambiguity as to
> what this function is doing.

Done.

[blundell, 2015-05-07 13:57:59]

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

https://codereview.chromium.org/1088533003/diff/240001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:13:
//            either the cached version of the given response do not contain
nit: s/of/or

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

https://codereview.chromium.org/1088533003/diff/240001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_apptest.cc:182:
url_response->url = "http://www.example.com/3";
You should add tests that the cached version is blown away when either there's
no etag cached or there's no etag in the new response.

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

https://codereview.chromium.org/1088533003/diff/240001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:194: // Returns
|entry| is requested.
nit: s/is/if

[DaveMoore, 2015-05-07 14:05:34]

LGTM once moved to mojo namespace

[qsr, 2015-05-07 14:18:07]

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

https://codereview.chromium.org/1088533003/diff/240001/mojo/services/url_resp...
mojo/services/url_response_disk_cache/public/interfaces/url_response_disk_cache.mojom:13:
//            either the cached version of the given response do not contain
On 2015/05/07 13:57:59, blundell wrote:
> nit: s/of/or

Done.

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

https://codereview.chromium.org/1088533003/diff/240001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_apptest.cc:182:
url_response->url = "http://www.example.com/3";
On 2015/05/07 13:57:59, blundell wrote:
> You should add tests that the cached version is blown away when either there's
> no etag cached or there's no etag in the new response.

Done.

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

https://codereview.chromium.org/1088533003/diff/240001/services/url_response_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:194: // Returns
|entry| is requested.
On 2015/05/07 13:57:59, blundell wrote:
> nit: s/is/if

Done.

[blundell, 2015-05-07 14:20:34]

LGTM

[qsr, 2015-05-07 14:21:04]

Committed patchset #14 (id:260001) manually as
5d429aa56783d74fc94ec9dcb26e95c0d346ec33 (presubmit successful).