Fill in some of ServiceWorkerFetchStores.

Fills in ServiceWorkerFetchStores.  Adds a stub ServiceWorkerFetchStore class which will need to be filled in. Also adds some tests.

A CL to refactor ServiceWorkerFetchStores -> ServiceWorkerCacheStorage is upcoming.

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

[jkarlin, 2014-08-01 18:42:45]

michaeln: Please review everything. Thanks!

[jkarlin, 2014-08-01 18:52:18]

On 2014/08/01 18:42:45, jkarlin wrote:
> michaeln: Please review everything. Thanks!

gavinp: FYI

[jkarlin, 2014-08-05 12:00:38]

On 2014/08/01 18:52:18, jkarlin wrote:
> On 2014/08/01 18:42:45, jkarlin wrote:
> > michaeln: Please review everything. Thanks!
> 
> gavinp: FYI

+falken: Please review everything, thanks!

[michaeln, 2014-08-05 23:34:31]

The .cc code looks pretty reasonable but i'm lacking the bigger picture.

My biggest concern is with the naming convention where a HexEncode'ing of user
input for 'name' is the directory name, for a couple reasons.

1) path and element length restrictions
2) the rename or moveto functions

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores.cc (right):

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:75:
ServiceWorkerFetchStore::CreatePersistentStore(path, name));
This assumes that directories in here are all legitimately named fetch stores
and does no verification, which is probably not a safe assumption. Verify with a
round trip encode/decode?

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:85:
base::HexEncode(store_name.c_str(), store_name.length()));
I'm not sure that having the length of store_name be so directly related to the
length of the file path element is a good idea. I see you're relying on the
encoding to be reversible.

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:231:
store->set_id(id);
What is the purpose if the store id and having two maps? Seems like you could
have a map<key, store*>. I see, you're sending the id value back to the renderer
and presumably other interfaces will come later that take these values as
inputs.

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores.h (right):

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.h:24: // The set of
stores for a given ServiceWorker. It is owned by the
s/b The set of stores for a given Origin.

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.h:55: // Get the
cache id for the given key.  If not found returns
nit: extra space after the first sentence

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.h:85: StoreMap
loaded_store_ids_;
i think the name of this data member is misleading, i was surprised to see this
owned the ptrs to the store instances, the StoreMap type is accurate, maybe call
this store_map_

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores_manager.cc
(right):

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores_manager.cc:162:
service_worker_fetch_stores_.insert(std::make_pair(origin, fetch_stores));
nothing is ever removed from this collection, is that ok?

[jkarlin, 2014-08-06 18:56:00]

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores.cc (right):

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:75:
ServiceWorkerFetchStore::CreatePersistentStore(path, name));
On 2014/08/05 23:34:30, michaeln wrote:
> This assumes that directories in here are all legitimately named fetch stores
> and does no verification, which is probably not a safe assumption. Verify with
a
> round trip encode/decode?

I'm not aware of a definition of legitimate store names so we'll accept anything
for now.  This update converts the filename to a string of a hash of the cache
name so regardless of the cache name the result is a fixed length string.

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:85:
base::HexEncode(store_name.c_str(), store_name.length()));
On 2014/08/05 23:34:30, michaeln wrote:
> I'm not sure that having the length of store_name be so directly related to
the
> length of the file path element is a good idea. I see you're relying on the
> encoding to be reversible.

Yes, the plan was to patch that up after MVP but have done so now instead by
putting the names of the stores in an index file.  On next load only stores with
names in the index file are loaded.

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:231:
store->set_id(id);
On 2014/08/05 23:34:30, michaeln wrote:
> What is the purpose if the store id and having two maps? Seems like you could
> have a map<key, store*>. I see, you're sending the id value back to the
renderer
> and presumably other interfaces will come later that take these values as
> inputs.

Yep yep.

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores.h (right):

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.h:24: // The set of
stores for a given ServiceWorker. It is owned by the
On 2014/08/05 23:34:30, michaeln wrote:
> s/b The set of stores for a given Origin.

Done.

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.h:55: // Get the
cache id for the given key.  If not found returns
On 2014/08/05 23:34:30, michaeln wrote:
> nit: extra space after the first sentence

Done.

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.h:85: StoreMap
loaded_store_ids_;
On 2014/08/05 23:34:30, michaeln wrote:
> i think the name of this data member is misleading, i was surprised to see
this
> owned the ptrs to the store instances, the StoreMap type is accurate, maybe
call
> this store_map_

Done.

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores_manager.cc
(right):

https://codereview.chromium.org/424863002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores_manager.cc:162:
service_worker_fetch_stores_.insert(std::make_pair(origin, fetch_stores));
On 2014/08/05 23:34:30, michaeln wrote:
> nothing is ever removed from this collection, is that ok?

Added a TODO to the header file.

[jkarlin, 2014-08-06 20:58:32]

PTAL.  Thanks!

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_store.h (right):

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_store.h:31: // called before
the other public member functions.
Really this is about initializing the backend (simplecache or memory cache) and
will be renamed to InitializeBackend().  I'll update the comment to reflect that
only the backend functions are affected.

[michaeln, 2014-08-07 00:10:14]

I think this lgtm as an interim or stepping stone.

> Yes, the plan was to patch that up after MVP but have done so now instead by
> putting the names of the stores in an index file.  On next load only stores
with
> names in the index file are loaded.

Is the post MVP plan written down anywhere? In looking at this, it's hard to
know what is temporary to bootstrap things vs meant to stick.

> I'm not aware of a definition of legitimate store names so we'll accept
anything
> for now.  This update converts the filename to a string of a hash of the cache
> name so regardless of the cache name the result is a fixed length string.

I was thinking about the user having gone into there and putting .bak on the end
of one of the existing directories for example. The old cl would  put a Store in
the list for that directory.

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores.cc (right):

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:82:
scoped_ptr<Pickle> pickle(new Pickle());
We prefer to use formats on disk that are endian agnostic. Protobufs are
currently being used for structured stuff deposited into the
ServiceWorkerStorage part of things and i think that would be a better choice
here too.

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:106:
base::MemoryMappedFile index_file_map;
memmapped files seems like an odd choice here, is it intentional? otherwise
seems like base::ReadFileToString would be more straightforward

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:191:
callback_loop_->PostTask(
i think this is an extra call to 'callback' that probably shouldn't be here

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:196: void
ServiceWorkerFetchStores::Get(const std::string& key,
Another thing that could help with Store vs Stores similarity is in method
naming?

stores->GetStore(name, ...)
store->GetResource(key, ...)

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores_manager.h
(right):

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores_manager.h:26: //
longer in active use.
Representing a store with a primitive 'int' at this level might make that more
challenging to accomplish.

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores_manager.h:42: const
std::string& key,
It's real easy to get mixed up about the names of things in here, FetchStores vs
FetchStore both of which are containers. It might help to consistently refer to
Stores as having a 'name' rather than a 'key'.

[jkarlin, 2014-08-07 14:49:27]

sky@chromium.org: Please review changes in content/browser/BUILD.gn

[jkarlin, 2014-08-07 16:02:35]

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_store.h (right):

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_store.h:31: // called before
the other public member functions.
On 2014/08/06 20:58:32, jkarlin wrote:
> Really this is about initializing the backend (simplecache or memory cache)
and
> will be renamed to InitializeBackend().  I'll update the comment to reflect
that
> only the backend functions are affected.

Done.

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores.cc (right):

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:82:
scoped_ptr<Pickle> pickle(new Pickle());
On 2014/08/07 00:10:13, michaeln wrote:
> We prefer to use formats on disk that are endian agnostic. Protobufs are
> currently being used for structured stuff deposited into the
> ServiceWorkerStorage part of things and i think that would be a better choice
> here too.

Done. Note I renamed the protobuf target from 'service_worker:proto_database' to
'service_worker:proto' since it includes database and cache now.

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:106:
base::MemoryMappedFile index_file_map;
On 2014/08/07 00:10:13, michaeln wrote:
> memmapped files seems like an odd choice here, is it intentional? otherwise
> seems like base::ReadFileToString would be more straightforward

Yes, ReadFileToString is better.  Thanks. Done.

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:191:
callback_loop_->PostTask(
On 2014/08/07 00:10:13, michaeln wrote:
> i think this is an extra call to 'callback' that probably shouldn't be here

Done.

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores.cc:196: void
ServiceWorkerFetchStores::Get(const std::string& key,
On 2014/08/07 00:10:13, michaeln wrote:
> Another thing that could help with Store vs Stores similarity is in method
> naming?
> 
> stores->GetStore(name, ...)
> store->GetResource(key, ...)

I renamed key -> store_name in the stores* classes.  Will do the resources thing
you mention in store when I fill it out.  On the plus side we're refactoring
back to CacheStorage and Cache which is easier to read as well.

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_stores_manager.h
(right):

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores_manager.h:26: //
longer in active use.
On 2014/08/07 00:10:13, michaeln wrote:
> Representing a store with a primitive 'int' at this level might make that more
> challenging to accomplish.

Acknowledged.

https://codereview.chromium.org/424863002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_fetch_stores_manager.h:42: const
std::string& key,
On 2014/08/07 00:10:13, michaeln wrote:
> It's real easy to get mixed up about the names of things in here, FetchStores
vs
> FetchStore both of which are containers. It might help to consistently refer
to
> Stores as having a 'name' rather than a 'key'.

Done.

[sky, 2014-08-07 16:23:52]

LGTM

[jkarlin, 2014-08-08 14:30:05]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-08-08 14:31:07]

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

[commit-bot: I haz the power, 2014-08-08 14:38:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-08 14:42:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-08 14:42:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jkarlin, 2014-08-08 16:35:17]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-08-08 16:35:44]

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

[commit-bot: I haz the power, 2014-08-08 16:50:48]

Change committed as 288379