ServiceWorker: Manipulate uncommitted/purgeable resource ids

ServiceWorker: Manipulate uncommitted/purgeable resource ids

This CL provides a way to manipulate uncommitted/purgeable resource ids
in ServiceWorkerDatabase.

- An uncommitted resource list contains resources which have been cached but a
registration associated with them hasn't been saved yet. When the registration
is saved, these are removed from the list.
- A purgeable resource list contains resources that no registration refers to.


BUG=364099
TEST=content_unittests --gtest_filter=ServiceWorkerDatabaseTest.*
R=michaeln@chromium.org

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

[nhiroki, 2014-04-29 16:45:25]

PTAL, thanks!

[michaeln, 2014-04-29 23:50:03]

couple-a nits, but looks pretty good to me

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:367: bool
ServiceWorkerDatabase::GetUncommittedResourceIds(std::set<int64>* ids) {
I think the only difference between the Uncommitted vs Purgeable methods is the
prefix used in each corresponding method body. Is that right? If so it would
make sense to have more generic private [Get|Write|Clear]ResourceIds methods
that take the prefix as a param and do like...

bool GetUncommittedResourceIds(...) {
  return GetResourceIds(kUncommittedPrefix, ...);
}

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_database.h:102: // caches
referring to it, it's added to the unclaimed list. Periodically,
the language in the comment is a little stale, 'unclaimed' s/b 'purgeable'

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_database.h:108: bool
GetUncommittedResourceIds(std::set<int64>* ids);
As a consumer of this api, i was thinking a std::vector<> would be good here
(and in the others). Is there a reason to use std::set<> instead?

[nhiroki, 2014-04-30 03:05:57]

Updated. Can you take another look? Thanks.

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:367: bool
ServiceWorkerDatabase::GetUncommittedResourceIds(std::set<int64>* ids) {
On 2014/04/29 23:50:03, michaeln wrote:
> I think the only difference between the Uncommitted vs Purgeable methods is
the
> prefix used in each corresponding method body. Is that right? If so it would
> make sense to have more generic private [Get|Write|Clear]ResourceIds methods
> that take the prefix as a param and do like...
> 
> bool GetUncommittedResourceIds(...) {
>   return GetResourceIds(kUncommittedPrefix, ...);
> }

That's right. Added Read/Write/DeleteResourceIds() and replaced.

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_database.h:102: // caches
referring to it, it's added to the unclaimed list. Periodically,
On 2014/04/29 23:50:03, michaeln wrote:
> the language in the comment is a little stale, 'unclaimed' s/b 'purgeable'

Done.

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_database.h:108: bool
GetUncommittedResourceIds(std::set<int64>* ids);
On 2014/04/29 23:50:03, michaeln wrote:
> As a consumer of this api, i was thinking a std::vector<> would be good here
> (and in the others). Is there a reason to use std::set<> instead?

This is just implementation issue. LevelDB seems not to be able to deduplicate
keys from a write batch, so if a vector contains duplicated keys, a batch issues
multiple operations for them.

If consumers prefer to use the vector version, I'll inline code for
deduplication into these functions. Wdyt?

[michaeln, 2014-04-30 03:23:56]

lgtm (modulo slightly more code sharing)

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_database.h:108: bool
GetUncommittedResourceIds(std::set<int64>* ids);
> This is just implementation issue. LevelDB seems not to be able to deduplicate
> keys from a write batch, so if a vector contains duplicated keys, a batch
issues
> multiple operations for them.

Which result in the last one value added to the batch for that key being stored,
is that right?

Ok, thnx for explaining the set<> usage.

https://codereview.chromium.org/254733002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/254733002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:408: if
(!LazyOpen(true) || is_disabled_)
These tests and the dcheck could also be in the common helper methods. Unless
there's a compelling reason not to, more sharing more better.

[nhiroki, 2014-04-30 04:04:06]

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/254733002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_database.h:108: bool
GetUncommittedResourceIds(std::set<int64>* ids);
On 2014/04/30 03:23:57, michaeln wrote:
> > This is just implementation issue. LevelDB seems not to be able to
deduplicate
> > keys from a write batch, so if a vector contains duplicated keys, a batch
> issues
> > multiple operations for them.
> 
> Which result in the last one value added to the batch for that key being
stored,
> is that right?

Yes, records having the same key are operated in FIFO order and the last one
will survive.

> Ok, thnx for explaining the set<> usage.

https://codereview.chromium.org/254733002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/254733002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:408: if
(!LazyOpen(true) || is_disabled_)
On 2014/04/30 03:23:57, michaeln wrote:
> These tests and the dcheck could also be in the common helper methods. Unless
> there's a compelling reason not to, more sharing more better.

That's just my preference. Basically I prefer such checks are located at the
beginning of public functions because sourcecode readers can easily understand
preconditions of following code.

But in this case, sharing code seems to make more maintainable and readable.
Moved those checks into the common helpers.

[nhiroki, 2014-04-30 04:04:14]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-04-30 04:05:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/254733002/120001

[nhiroki, 2014-04-30 07:29:28]

Committed patchset #3 manually as r267118 (presubmit successful).