Initial implementation of ServiceWorkerCache.

Initial implementation of ServiceWorkerCache Put and Match functions.

Note that the Match() function copies the data into a memory blob.  This is of course only for demo purposes. This will soon be changed to use a streaming API. See bug 403493.

BUG=392621
Committed: https://crrev.com/b5ca81a3262238668cc4e59a90f51d7e8f6360d4
Cr-Commit-Position: refs/heads/master@{#291834}

[jkarlin, 2014-08-13 20:16:23]

Note that this is not yet cleaned up and for some reason the first few bytes
that I read back from SimpleCache are borked.  That should be clearer to me in
the morning.

Due to time constraints, I'm asking you guys to take a first look at this before
it's really ready for review so that I can pick up with your thoughts tomorrow
morning.

michaeln@chromium.org: Please review everything.

gavinp@chromium.org: Please review use of SimpleCache in service_worker_cache.cc

[michaeln, 2014-08-14 10:35:20]

some early comments

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:27: const int
kMaxCacheBytes = 5 * 1024 * 1024;
why so small?

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:30: const int kBufferSize
= 1024 * 512;
why so big :)

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:34: explicit
ResponseReadContext(scoped_refptr<net::IOBufferWithSize> buff,
is explicit needed with two args

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:47: class
ServiceWorkerCache::BlobReader : public net::URLRequest::Delegate {
this is very similar to what ServiceWorkerWriteToCacheJob does, it reads a
network URLRequest and writes it to a net::DiskCache

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:193:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::CACHE).get(),
ServiceWorkerContextCore constructor has the cachethread mlp as an input. This
class should probably use that input too.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:195: &backend_,
initialization is treacherous, i think if ServiceWorkerCache is deleted prior to
create cache completion, we could be trying to write to &backend_ after storage
for it is gone.

you've probably seen ServiceWorkerDiskCache ServiceWorkerResponseReader and
ServiceWorkerResponseWriter. these classes do what they sound like they do, are
well tested, and maybe could be applied here as well.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:282:
base::Unretained(entry),
if ServiceWorkerCache is deleted while in flight, is entry leaked?

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:429: // reference count
that?
1) the uuid is sent from browser->renderer in the MatchResponse IPC
2) the renderer takes a reference when a blink::BlobDataHandle is constructed
over there
3) the renderer sends an 'ack' to the MatchResponse
4) upon receipt of the 'ack', the browser deletes the
webkit_blob::BlobDataHandle

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:483:
scoped_refptr<net::ZeroCopyStringIOBuffer> buffer(
would it make sense for the existing StringIOBuffer class to have a ctor that
used std::string::swap on the string provided to zerocopy it into the iobuffer?

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:541:
blob_storage_context_->GetBlobDataFromUUID(response->blob_uuid);
you should get this handle up front in ServiceWorkerCache::Put() otherwise
there's no guarantee it hasn't been gc'd by the time this is reached. The
webkit_blob::BlobDataHandle should be gotten prior to return from IPC handler
for the PUT message.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:558: entry,
if ServiceWorkerCache is deleted prior to completion, entry will not be closed
(leak?)

[jkarlin, 2014-08-14 19:53:33]

Okay, PTAL!

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:27: const int
kMaxCacheBytes = 5 * 1024 * 1024;
On 2014/08/14 10:35:20, michaeln wrote:
> why so small?

Great question.  No idea what's proper.  Upped to 50MB for now.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:30: const int kBufferSize
= 1024 * 512;
On 2014/08/14 10:35:19, michaeln wrote:
> why so big :)

This is the size that we read from the cache when streaming down to Blink for
normal loads.  See async_resource_handler.cc:38

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:34: explicit
ResponseReadContext(scoped_refptr<net::IOBufferWithSize> buff,
On 2014/08/14 10:35:20, michaeln wrote:
> is explicit needed with two args

Done.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:47: class
ServiceWorkerCache::BlobReader : public net::URLRequest::Delegate {
On 2014/08/14 10:35:20, michaeln wrote:
> this is very similar to what ServiceWorkerWriteToCacheJob does, it reads a
> network URLRequest and writes it to a net::DiskCache

True, but not a straight-forward adaptation.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:193:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::CACHE).get(),
On 2014/08/14 10:35:20, michaeln wrote:
> ServiceWorkerContextCore constructor has the cachethread mlp as an input. This
> class should probably use that input too.

Agree, added a TODO to do that in another CL.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:195: &backend_,
On 2014/08/14 10:35:20, michaeln wrote:
> initialization is treacherous, i think if ServiceWorkerCache is deleted prior
to
> create cache completion, we could be trying to write to &backend_ after
storage
> for it is gone.

The ServiceWorkerCacheStorage::Delete(cache) can't be called until create cache
has completed.  Other forms of deletion (e.g., ServiceWorker closing) will cause
the ServiceWorkerCacheStorage to delete and so the callback won't run (due to
WeakPtr).

> you've probably seen ServiceWorkerDiskCache ServiceWorkerResponseReader and
> ServiceWorkerResponseWriter. these classes do what they sound like they do,
are
> well tested, and maybe could be applied here as well.

I'm all for consolidation.  Those classes rely on an (entry_name -> int64) key
conversion which I don't have or necessarily want however. Perhaps in the
future?

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:282:
base::Unretained(entry),
On 2014/08/14 10:35:20, michaeln wrote:
> if ServiceWorkerCache is deleted while in flight, is entry leaked?

Yes, thanks.  Hadn't gotten to all of the pointers yet.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:429: // reference count
that?
On 2014/08/14 10:35:20, michaeln wrote:
> 1) the uuid is sent from browser->renderer in the MatchResponse IPC
> 2) the renderer takes a reference when a blink::BlobDataHandle is constructed
> over there
> 3) the renderer sends an 'ack' to the MatchResponse
> 4) upon receipt of the 'ack', the browser deletes the
> webkit_blob::BlobDataHandle

Acknowledged.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:483:
scoped_refptr<net::ZeroCopyStringIOBuffer> buffer(
On 2014/08/14 10:35:20, michaeln wrote:
> would it make sense for the existing StringIOBuffer class to have a ctor that
> used std::string::swap on the string provided to zerocopy it into the
iobuffer?

I like the idea of using std::string:swap.  I changed the zerocopy class to use
that instead.  Given the various uses of StringIOBuffer today I'd rather
introduce this new function than change the old.  We could (and should)
deprecate the old one however.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:541:
blob_storage_context_->GetBlobDataFromUUID(response->blob_uuid);
On 2014/08/14 10:35:20, michaeln wrote:
> you should get this handle up front in ServiceWorkerCache::Put() otherwise
> there's no guarantee it hasn't been gc'd by the time this is reached. The
> webkit_blob::BlobDataHandle should be gotten prior to return from IPC handler
> for the PUT message.

Thanks! Done and added test.

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:558: entry,
On 2014/08/14 10:35:20, michaeln wrote:
> if ServiceWorkerCache is deleted prior to completion, entry will not be closed
> (leak?)

Done.

[gavinp, 2014-08-14 22:02:45]

https://codereview.chromium.org/465463002/diff/220001/net/base/io_buffer.h
File net/base/io_buffer.h (right):

https://codereview.chromium.org/465463002/diff/220001/net/base/io_buffer.h#ne...
net/base/io_buffer.h:129: class NET_EXPORT ZeroCopyStringIOBuffer : public
IOBuffer {
Firstly, this is really cool. Well thought out and well done. However, our
current STL implementation on most platforms is C++98/C++03 compliant, which
means that it actually is, behind the scenes, doing refcounting. So in practice
this class won't have any advantage over the existing StringIOBuffer
constructor, since under the covers a refcount is being added.

However, in C++11 the iterator invalidation semantics make a refcounting
implementation of strings implausible. So this is more future proof. I like the
future.

However, and this is a big however, swap is a bit subtle, I'd like to see that
advertised a bit better to callers. The other thing is, since this is basically
the same as a StringIOBuffer, I'd like to see it unified.

There's already a lot of users of the StringIOBuffer, so let's leave that
constructor the same. However, why not add a static create method with a name
that really calls out the operation... Perhaps add another constructor:

   StringIOBuffer::StringIOBuffer(scoped_ptr<std::string> blah);

And document that constructor as being very low cost.

(Note: you might object that the string you want to use is on the heap so
dynamically allocating it is a waste; well, true, but the string object itself
is pretty small, most of its allocation is on the heap anyway, and the
scoped_ptr<> argument conveys the transfer of ownership better than any name we
could come up with)

[michaeln1, 2014-08-14 22:57:09]

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:195: &backend_,
> The ServiceWorkerCacheStorage::Delete(cache) can't be called until create
cache
> has completed.  Other forms of deletion (e.g., ServiceWorker closing) will
cause
> the ServiceWorkerCacheStorage to delete and so the callback won't run (due to
> WeakPtr).

The callback won't get called but &backend_ is written to prior to invoking the
callback.

[michaeln, 2014-08-14 22:59:01]

i think this lgtm for our immediate goals (modulo the potential &backend_ memory
access at the wrong time)

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache.h (right):

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:173: BlobDataHandleMap
blob_data_handle_map_;
Can the ServiceWorkerCachebe deleted prior to receipt of the ack?

The ServiceWorkerDispatcherHost may be a better place for this collection since
it's scope to the process requesting the CacheMatch.

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.cc (right):

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:15: #include
"content/browser/fileapi/chrome_blob_storage_context.h"
is this needed or is the include for blob_storage_context.h enough?

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.h (right):

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.h:30: class
ChromeBlobStorageContext;
is this needed?

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_unittest.cc (right):

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:135:
loop->Run();
it's real common to see this one-liner to get queued tasks to execute and then
to continue, you may be able to remove some boiler plate busywork stuff?

base::RunLoop().RunUntilIdle();

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:222:
TEST_P(ServiceWorkerCacheTestP, PutNoBody) {
i'm not familiar with TEST_P?

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:246: LOG(ERROR)
<< "0";
not sure you want to keep the log lines

https://codereview.chromium.org/465463002/diff/220001/net/base/io_buffer.cc
File net/base/io_buffer.cc (right):

https://codereview.chromium.org/465463002/diff/220001/net/base/io_buffer.cc#n...
net/base/io_buffer.cc:62: data_ = NULL;
is this needed? oh i see the comment in ~StringIOBuffer applies here as well,
probably worth putting that comment here too.

[jkarlin, 2014-08-15 00:13:34]

https://codereview.chromium.org/465463002/diff/220001/net/base/io_buffer.h
File net/base/io_buffer.h (right):

https://codereview.chromium.org/465463002/diff/220001/net/base/io_buffer.h#ne...
net/base/io_buffer.h:129: class NET_EXPORT ZeroCopyStringIOBuffer : public
IOBuffer {
On 2014/08/14 22:02:45, gavinp wrote:
> Firstly, this is really cool. Well thought out and well done. However, our
> current STL implementation on most platforms is C++98/C++03 compliant, which
> means that it actually is, behind the scenes, doing refcounting. So in
practice
> this class won't have any advantage over the existing StringIOBuffer
> constructor, since under the covers a refcount is being added.
> 
> However, in C++11 the iterator invalidation semantics make a refcounting
> implementation of strings implausible. So this is more future proof. I like
the
> future.
> 
> However, and this is a big however, swap is a bit subtle, I'd like to see that
> advertised a bit better to callers. The other thing is, since this is
basically
> the same as a StringIOBuffer, I'd like to see it unified.
> 
> There's already a lot of users of the StringIOBuffer, so let's leave that
> constructor the same. However, why not add a static create method with a name
> that really calls out the operation... Perhaps add another constructor:
> 
>    StringIOBuffer::StringIOBuffer(scoped_ptr<std::string> blah);
> 
> And document that constructor as being very low cost.
> 
> (Note: you might object that the string you want to use is on the heap so
> dynamically allocating it is a waste; well, true, but the string object itself
> is pretty small, most of its allocation is on the heap anyway, and the
> scoped_ptr<> argument conveys the transfer of ownership better than any name
we
> could come up with)

Great suggestion.  Done.

[gavinp, 2014-08-15 01:27:48]

net/ lgtm

[jkarlin, 2014-08-15 11:49:44]

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/465463002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:195: &backend_,
On 2014/08/14 22:57:09, michaeln1 wrote:
> > The ServiceWorkerCacheStorage::Delete(cache) can't be called until create
> cache
> > has completed.  Other forms of deletion (e.g., ServiceWorker closing) will
> cause
> > the ServiceWorkerCacheStorage to delete and so the callback won't run (due
to
> > WeakPtr).
> 
> The callback won't get called but &backend_ is written to prior to invoking
the
> callback.

Done.

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache.h (right):

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:173: BlobDataHandleMap
blob_data_handle_map_;
On 2014/08/14 22:59:00, michaeln wrote:
> Can the ServiceWorkerCachebe deleted prior to receipt of the ack?
> 
> The ServiceWorkerDispatcherHost may be a better place for this collection
since
> it's scope to the process requesting the CacheMatch.

Done. One would expect that deleting a cache while a match() was still
unresolved would yield undefined results to pending operations.  However, we
should resolve properly.  Agree, someone else must own this pointer.  Now
passing it back with the response.

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.cc (right):

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:15: #include
"content/browser/fileapi/chrome_blob_storage_context.h"
On 2014/08/14 22:59:00, michaeln wrote:
> is this needed or is the include for blob_storage_context.h enough?

Done.

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.h (right):

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.h:30: class
ChromeBlobStorageContext;
On 2014/08/14 22:59:01, michaeln wrote:
> is this needed?

Done.

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_unittest.cc (right):

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:135:
loop->Run();
On 2014/08/14 22:59:01, michaeln wrote:
> it's real common to see this one-liner to get queued tasks to execute and then
> to continue, you may be able to remove some boiler plate busywork stuff?
> 
> base::RunLoop().RunUntilIdle();

I can do that once the cache uses the passed in MessageLoopProxy instead of the
cache thread.  TODO added.

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:222:
TEST_P(ServiceWorkerCacheTestP, PutNoBody) {
On 2014/08/14 22:59:01, michaeln wrote:
> i'm not familiar with TEST_P?

It lets you parameterize the test class.  In this case, all of the TESTP tests
are run twice.  Once with true and once with false for the parameter.  This
translates to once with memory only and once with disk backend.

See the following code snippets in the file:

class ServiceWorkerCacheTestP : public ServiceWorkerCacheTest,
                                public testing::WithParamInterface<bool> {
  virtual bool MemoryOnly() OVERRIDE { return !GetParam(); }
};

INSTANTIATE_TEST_CASE_P(ServiceWorkerCacheTest,
                        ServiceWorkerCacheTestP,
                        ::testing::Values(false, true));

https://codereview.chromium.org/465463002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:246: LOG(ERROR)
<< "0";
On 2014/08/14 22:59:01, michaeln wrote:
> not sure you want to keep the log lines

Done.

https://codereview.chromium.org/465463002/diff/220001/net/base/io_buffer.cc
File net/base/io_buffer.cc (right):

https://codereview.chromium.org/465463002/diff/220001/net/base/io_buffer.cc#n...
net/base/io_buffer.cc:62: data_ = NULL;
On 2014/08/14 22:59:01, michaeln wrote:
> is this needed? oh i see the comment in ~StringIOBuffer applies here as well,
> probably worth putting that comment here too.

No longer necessary after merge into StringIOBuffer as suggested by gavinp.

[jkarlin, 2014-08-15 13:11:41]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-08-15 13:12:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/465463002/310001

[jkarlin, 2014-08-15 18:19:17]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-08-15 18:23:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/465463002/350001

[jkarlin, 2014-08-15 18:53:57]

The CQ bit was unchecked by jkarlin@chromium.org

[jkarlin, 2014-08-20 19:20:02]

michaeln: PTAL at the changes since PS17 before I commit.  Thanks!

[jkarlin, 2014-08-20 19:20:43]

On 2014/08/20 19:20:02, jkarlin wrote:
> michaeln: PTAL at the changes since PS17 before I commit.  Thanks!

Sorry, make that since PS11.

[michaeln1, 2014-08-20 20:56:47]

> > michaeln: PTAL at the changes since PS17 before I commit.  Thanks!
> Sorry, make that since PS11.

k, i'll take a new look

[michaeln, 2014-08-21 01:46:15]

i'd like to understand more about the interface between the renderer and browser
you're working towards

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:27: const int
kMaxCacheBytes = 512 * 1024 * 1024;
there's a bug open about somebody trying to download a 15Gbyte file and it
failing when it gets to the 10Gbyte mark. in light of that, this number seems
low

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:135: if (rv !=
expected_bytes) {
in this part of the urlrequest contract or just how it works out in practice?

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:183: path_.empty() ?
net::MEMORY_CACHE : net::DISK_CACHE;
I think you want to use net::APP_CACHE here instead of DISK_CACHE. That disables
eviction logic and some book keeping in support of it, and give you more of a
passive name/value pair storage repo.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:191:
base::Bind(&ServiceWorkerCache::CreateBackendDidCreate,
nit: it might be more readable to invoke base::Bind in the arg list for the
function that consumes it, that way you read the invocation of Foo() first, then
something about DidFoo().

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:226: void
ServiceWorkerCache::Put(ServiceWorkerFetchRequest* request,
wassup with ownership of |request| and |response|?

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:326:
ServiceWorkerFetchRequest* request,
wassup with ownership of 'request'?

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:468: // TODO(jkarlin):
This copying of the the entire cache response into memory is
It's really hard to say this looks good. Every call to 'match' will read the
entire response body into memory. Honestly its not so good looking. Maybe we
should seperate querying the content of the cache (the Match method) from
retrieving data from it. Defer data retrieval for as long as we can. There are
at least two different ways the data could be accessed.

1) script invokes response.body.asBlob() (which i think returns a promise for
the blob)
2) script invokes respondWith(response)

In the latter case, the renderer process need never see a blob at all. The
response data could be streamed straight out of some diskcache w/o using blob
handling layer of indirection at all.

I want to look more closely at the ipc messages flowing between the browser and
renderer to get a better understanding of how things are piecing together. I'm
just seeing these small fragments and don't see how they're to be put together.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
File content/browser/service_worker/service_worker_cache.h (right):

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:157: void
DeleteDidOpenEntry(ServiceWorkerFetchRequest* request,
All of these DidSomething() async completion callbacks take as input raw ptr
values that were provide as input to Something(). How is that memory supposed to
be managed?

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
File content/browser/service_worker/service_worker_cache.proto (right):

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.proto:14: required int32
size = 2;
can this be an int64

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.proto:19: message
ServiceWorkerRequestResponseHeaders {
should this and the index struct have a version field?

https://codereview.chromium.org/465463002/diff/530001/net/base/io_buffer.h
File net/base/io_buffer.h (right):

https://codereview.chromium.org/465463002/diff/530001/net/base/io_buffer.h#ne...
net/base/io_buffer.h:118: // The string passed to this constructor is
std::string::swap'd with the
Given the scoped_ptr<> input, the xfer of ownership of the string is clear and
the swap method usage is an impl detail. Consider removing the comment
altogether?

[jkarlin, 2014-08-21 18:31:21]

Thanks Michael!

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:27: const int
kMaxCacheBytes = 512 * 1024 * 1024;
On 2014/08/21 01:46:15, michaeln wrote:
> there's a bug open about somebody trying to download a 15Gbyte file and it
> failing when it gets to the 10Gbyte mark. in light of that, this number seems
> low

We should talk to jsbell about what the appropriate size is but it's easy to
change and not an immediate concern.  Let's increase this size once we're not
copying the thing into a memory blob.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:135: if (rv !=
expected_bytes) {
On 2014/08/21 01:46:15, michaeln wrote:
> in this part of the urlrequest contract or just how it works out in practice?

This is how everybody uses entry->WriteData.  Either it writes all or it's an
error.  I spoke with rvargas@ (disk_cache author) about this a week ago and he
said that this is fine.

See
https://code.google.com/p/chromium/codesearch#chromium/src/net/tools/dump_cac...
for another example.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:183: path_.empty() ?
net::MEMORY_CACHE : net::DISK_CACHE;
On 2014/08/21 01:46:15, michaeln wrote:
> I think you want to use net::APP_CACHE here instead of DISK_CACHE. That
disables
> eviction logic and some book keeping in support of it, and give you more of a
> passive name/value pair storage repo.

Thanks. Done.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:191:
base::Bind(&ServiceWorkerCache::CreateBackendDidCreate,
On 2014/08/21 01:46:15, michaeln wrote:
> nit: it might be more readable to invoke base::Bind in the arg list for the
> function that consumes it, that way you read the invocation of Foo() first,
then
> something about DidFoo().

Having create_cache_callback let's me both pass it to the
disk::CreateCacheBackend call and run it in case CreateCacheBackend
synchronously completes.  This ensures that  CreateBackendDidCreate is called
consistently in either case.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:226: void
ServiceWorkerCache::Put(ServiceWorkerFetchRequest* request,
On 2014/08/21 01:46:15, michaeln wrote:
> wassup with ownership of |request| and |response|?

Owned by the caller.  The callback is now guaranteed to run. Discussed further
in the header file.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:326:
ServiceWorkerFetchRequest* request,
On 2014/08/21 01:46:15, michaeln wrote:
> wassup with ownership of 'request'?

Owned by the caller.  The callback is now guaranteed to run. Discussed further
in the header file.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.cc:468: // TODO(jkarlin):
This copying of the the entire cache response into memory is
On 2014/08/21 01:46:15, michaeln wrote:
> It's really hard to say this looks good. Every call to 'match' will read the
> entire response body into memory. Honestly its not so good looking. Maybe we
> should seperate querying the content of the cache (the Match method) from
> retrieving data from it. Defer data retrieval for as long as we can. There are
> at least two different ways the data could be accessed.
> 
> 1) script invokes response.body.asBlob() (which i think returns a promise for
> the blob)
> 2) script invokes respondWith(response)
> 
> In the latter case, the renderer process need never see a blob at all. The
> response data could be streamed straight out of some diskcache w/o using blob
> handling layer of indirection at all.
> 
> I want to look more closely at the ipc messages flowing between the browser
and
> renderer to get a better understanding of how things are piecing together. I'm
> just seeing these small fragments and don't see how they're to be put
together.
> 

Yes, this optimization needs to happen.  But we need to start somewhere.  It
will be complex to add support for streaming (what happens if somebody deletes
the cache entry after the response has been returned but before it has been
read?) and needs to be designed very carefully. Gavin, Dominic, and I all agree
that copying to memory is the best place to start.  Once MVP is working the very
next step is to focus on streaming.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
File content/browser/service_worker/service_worker_cache.h (right):

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:157: void
DeleteDidOpenEntry(ServiceWorkerFetchRequest* request,
On 2014/08/21 01:46:15, michaeln wrote:
> All of these DidSomething() async completion callbacks take as input raw ptr
> values that were provide as input to Something(). How is that memory supposed
to
> be managed?

I've reworked ServiceWorkerCache so that its public functions are guaranteed to
run their callbacks.  Therefore these inputs will not be lost.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
File content/browser/service_worker/service_worker_cache.proto (right):

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.proto:14: required int32
size = 2;
On 2014/08/21 01:46:15, michaeln wrote:
> can this be an int64

Done.  Because size has never been used no migration code is necessary.

https://codereview.chromium.org/465463002/diff/530001/content/browser/service...
content/browser/service_worker/service_worker_cache.proto:19: message
ServiceWorkerRequestResponseHeaders {
On 2014/08/21 01:46:15, michaeln wrote:
> should this and the index struct have a version field?

Protobuf tags obviate the need for version fields.

https://codereview.chromium.org/465463002/diff/530001/net/base/io_buffer.h
File net/base/io_buffer.h (right):

https://codereview.chromium.org/465463002/diff/530001/net/base/io_buffer.h#ne...
net/base/io_buffer.h:118: // The string passed to this constructor is
std::string::swap'd with the
On 2014/08/21 01:46:15, michaeln wrote:
> Given the scoped_ptr<> input, the xfer of ownership of the string is clear and
> the swap method usage is an impl detail. Consider removing the comment
> altogether?

Done.

[jkarlin, 2014-08-22 16:22:14]

michaeln: PTAL. Thanks so much.

[jkarlin, 2014-08-22 23:49:13]

On 2014/08/22 16:22:14, jkarlin wrote:
> michaeln: PTAL. Thanks so much.

Ping :)

[jkarlin, 2014-08-25 19:09:52]

On 2014/08/22 23:49:13, jkarlin wrote:
> On 2014/08/22 16:22:14, jkarlin wrote:
> > michaeln: PTAL. Thanks so much.
> 
> Ping :)

PTAL.  Thanks!

[michaeln, 2014-08-25 21:16:49]

lgtm

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
File content/browser/service_worker/service_worker_cache.h (right):

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:80: // ErrorTypeOK. The
callback will always be called. |request| must remain valid
nit: linelength

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
File content/browser/service_worker/service_worker_cache_unittest.cc (right):

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:211:
scoped_refptr<webkit_blob::BlobData> blob_data_;
i was looking around for where else this is used and why but it looks like
blob_data_ is only needed locally in CreateRequest. if that's right maybe
eliminate the datamember

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:212:
scoped_ptr<webkit_blob::BlobDataHandle> blob_handle1_;
nit: since this has a trailing 1 on it, i was wondering where the #2 was

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:226:
EXPECT_TRUE(Put(no_body_request_.get(), no_body_response_.get()));
nit: if Put returned ServiceWorkerCache::ErrorType, might help to diagnose
problems if ever the expectation fails: EXPECT_EQ(ErrorTypeOK, Put(...));

[jkarlin, 2014-08-25 23:51:46]

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
File content/browser/service_worker/service_worker_cache.h (right):

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:80: // ErrorTypeOK. The
callback will always be called. |request| must remain valid
On 2014/08/25 21:16:48, michaeln wrote:
> nit: linelength

Done.

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
File content/browser/service_worker/service_worker_cache_unittest.cc (right):

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:211:
scoped_refptr<webkit_blob::BlobData> blob_data_;
On 2014/08/25 21:16:48, michaeln wrote:
> i was looking around for where else this is used and why but it looks like
> blob_data_ is only needed locally in CreateRequest. if that's right maybe
> eliminate the datamember

Done.

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:212:
scoped_ptr<webkit_blob::BlobDataHandle> blob_handle1_;
On 2014/08/25 21:16:48, michaeln wrote:
> nit: since this has a trailing 1 on it, i was wondering where the #2 was

Done.

https://codereview.chromium.org/465463002/diff/630001/content/browser/service...
content/browser/service_worker/service_worker_cache_unittest.cc:226:
EXPECT_TRUE(Put(no_body_request_.get(), no_body_response_.get()));
On 2014/08/25 21:16:48, michaeln wrote:
> nit: if Put returned ServiceWorkerCache::ErrorType, might help to diagnose
> problems if ever the expectation fails: EXPECT_EQ(ErrorTypeOK, Put(...));

Acknowledged.

[jkarlin, 2014-08-25 23:51:54]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-08-25 23:53:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/465463002/670001

[jkarlin, 2014-08-26 00:45:10]

The CQ bit was unchecked by jkarlin@chromium.org

[jkarlin, 2014-08-26 01:52:41]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-08-26 01:53:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/465463002/690001

[commit-bot: I haz the power, 2014-08-26 03:59:44]

Committed patchset #27 (690001) as 297ad0dc13be3e4b0c3f51c2e1eb2cf6ae161e1b

[commit-bot: I haz the power, 2014-09-10 02:40:16]

Patchset 27 (id:??) landed as
https://crrev.com/b5ca81a3262238668cc4e59a90f51d7e8f6360d4
Cr-Commit-Position: refs/heads/master@{#291834}