ServiceWorker: Introduce ServiceWorkerDiskCacheMigrator

ServiceWorker: Introduce ServiceWorkerDiskCacheMigrator

This introduces ServiceWorkerDiskCacheMigrator for migrating the script cache
from the BlockFile backend to the Simple backend. The migrator iterates over
resources cached in the src DiskCache and moves them into the dest DiskCache
one by one.

This just adds the migrator code and does NOT run the migration code on the
production environment yet.

BUG=487482
TEST=content_unittests --gtest_filter=ServiceWorkerDiskCacheMigratorTest.*

Committed: https://crrev.com/255913a791a398eed5c352fa3ac220be178cc772
Cr-Commit-Position: refs/heads/master@{#332603}

[nhiroki, 2015-05-25 17:07:16]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-05-25 17:13:34]

nhiroki@chromium.org changed reviewers:
+ kinuko@chromium.org, michaeln@chromium.org

[nhiroki, 2015-05-25 17:13:35]

Hi michaeln@ and kinuko@, can you review this?

This migrator will be called during the storage initialization (ie.
ServiceWorkerStorage::LazyInitialize).  You can see the entire usage in the WIP
CL [1]. Thanks!

[1] https://codereview.chromium.org/1152543002/

[nhiroki, 2015-05-26 00:56:24]

Hmmm... migrator tests are failing on linux_android_rel_ng maybe due to timeout.
I'll take a closer look...

[nhiroki, 2015-05-26 01:10:07]

On 2015/05/26 00:56:24, nhiroki wrote:
> Hmmm... migrator tests are failing on linux_android_rel_ng maybe due to
timeout.
> I'll take a closer look...

I suspect that the AppCacheDiskCache with the BlockFile backend on Android could
be slow (or cannot work???). Android has already used the Simple backend by
default, so we could just move the whole diskcache directory to new location
instead of running the migration and skip the tests.

[kinuko, 2015-05-26 03:18:17]

On 2015/05/26 01:10:07, nhiroki wrote:
> On 2015/05/26 00:56:24, nhiroki wrote:
> > Hmmm... migrator tests are failing on linux_android_rel_ng maybe due to
> timeout.
> > I'll take a closer look...
> 
> I suspect that the AppCacheDiskCache with the BlockFile backend on Android
could
> be slow (or cannot work???). Android has already used the Simple backend by
> default, so we could just move the whole diskcache directory to new location
> instead of running the migration and skip the tests.

They seem to be crashing rather than timing out? Not sure if that's because
Android-specific block file backend or not.

[nhiroki, 2015-05-26 03:35:38]

On 2015/05/26 03:18:17, kinuko wrote:
> On 2015/05/26 01:10:07, nhiroki wrote:
> > On 2015/05/26 00:56:24, nhiroki wrote:
> > > Hmmm... migrator tests are failing on linux_android_rel_ng maybe due to
> > timeout.
> > > I'll take a closer look...
> > 
> > I suspect that the AppCacheDiskCache with the BlockFile backend on Android
> could
> > be slow (or cannot work???). Android has already used the Simple backend by
> > default, so we could just move the whole diskcache directory to new location
> > instead of running the migration and skip the tests.
> 
> They seem to be crashing rather than timing out? Not sure if that's because
> Android-specific block file backend or not.

To clarify the problem, patchset2 tries to migrate from "Simple" to "Simple" on
Android. If this also crashes, the migrator may be broken.

[nhiroki, 2015-05-26 09:11:56]

Patchset #5 (id:90001) has been deleted

[nhiroki, 2015-05-26 09:16:00]

On 2015/05/26 03:35:38, nhiroki wrote:
> On 2015/05/26 03:18:17, kinuko wrote:
> > On 2015/05/26 01:10:07, nhiroki wrote:
> > > On 2015/05/26 00:56:24, nhiroki wrote:
> > > > Hmmm... migrator tests are failing on linux_android_rel_ng maybe due to
> > > timeout.
> > > > I'll take a closer look...
> > > 
> > > I suspect that the AppCacheDiskCache with the BlockFile backend on Android
> > could
> > > be slow (or cannot work???). Android has already used the Simple backend
by
> > > default, so we could just move the whole diskcache directory to new
location
> > > instead of running the migration and skip the tests.
> > 
> > They seem to be crashing rather than timing out? Not sure if that's because
> > Android-specific block file backend or not.
> 
> To clarify the problem, patchset2 tries to migrate from "Simple" to "Simple"
on
> Android. If this also crashes, the migrator may be broken.

Ok, this CL was broken. The patchset5 should work well. Can you take another
look?

[kinuko, 2015-05-27 05:45:15]

Sorry for slow review!

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
File content/browser/service_worker/service_worker_disk_cache_migrator.cc
(right):

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:19: :
public base::RefCounted<ServiceWorkerDiskCacheMigrator::Task> {
nit: It's not obvious why this needs to be ref-counted?

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:30: friend
class ServiceWorkerDiskCacheMigrator;
nit: having this Migrator class friend seems to imply that 'private:'  basically
does nothing.

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:202: // a
retry happens.
Do we really want to delete entry one by one while migration is ongoing?
I think we could still writing the same resource twice if unexpected shutdown
happens?

Could we make this migration all or nothing model?  Or how are you planning to
call this migrator to make it 'resumable'?

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:245:
scoped_refptr<Task> next_task(new Task(weak_factory_.GetWeakPtr()));
Instead of doing this here and initializing necessary fields at multiple places,
is it possible/sensible to create a new task in OnOpenNextEntry, so that
Task::entry_, task_id_, src_, dest_ all initialized in the ctor?

[nhiroki, 2015-05-27 09:23:46]

Patchset #6 (id:130001) has been deleted

[nhiroki, 2015-05-27 09:27:05]

Patchset #6 (id:150001) has been deleted

[nhiroki, 2015-05-27 10:05:28]

Thank you for reviewing! Updated.

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
File content/browser/service_worker/service_worker_disk_cache_migrator.cc
(right):

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:19: :
public base::RefCounted<ServiceWorkerDiskCacheMigrator::Task> {
On 2015/05/27 05:45:14, kinuko wrote:
> nit: It's not obvious why this needs to be ref-counted?

I thought the owner of the disk_cache::Entry* should be ref-counted to ensure
that the entry ptr is alive until OnOpenNextEntry via both sync and async
routes, but it was misconception. I made this task non-refcounted.

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:30: friend
class ServiceWorkerDiskCacheMigrator;
On 2015/05/27 05:45:14, kinuko wrote:
> nit: having this Migrator class friend seems to imply that 'private:' 
basically
> does nothing.

These 'friend' are no longer necessary. Removed.

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:202: // a
retry happens.
On 2015/05/27 05:45:15, kinuko wrote:
> Do we really want to delete entry one by one while migration is ongoing?
> I think we could still writing the same resource twice if unexpected shutdown
> happens?

Right, half-migrated resources or migrated-but-not-deleted resources could be
overwritten.

> Could we make this migration all or nothing model?

At first, I though the resumable model was more stable for (a few) powerful
users who had tons of cached resources. But, at this moment, the number of
stored resources would be small for most of users and the all-or-nothing model
would be sufficient. In addition, issuing a bunch of delete operations could
cause additional corruption...

so, I switched this CL to the all-or-nothing model.

> Or how are you planning to call this migrator to make it 'resumable'?

On the PS5, Calling Start() on newly created migrator can resume migration. The
migrator just overwrites existing resources in the dest DiskCache (see
ServiceWorkerDiskCacheMigratorTest::MigrateResponses_RetryAfterAbort).

https://codereview.chromium.org/1155063002/diff/110001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:245:
scoped_refptr<Task> next_task(new Task(weak_factory_.GetWeakPtr()));
On 2015/05/27 05:45:15, kinuko wrote:
> Instead of doing this here and initializing necessary fields at multiple
places,
> is it possible/sensible to create a new task in OnOpenNextEntry, so that
> Task::entry_, task_id_, src_, dest_ all initialized in the ctor?

Done.

[kinuko, 2015-05-27 14:12:15]

lgtm

https://codereview.chromium.org/1155063002/diff/170001/content/browser/servic...
File
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc
(right):

https://codereview.chromium.org/1155063002/diff/170001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:84:
info->response_time = base::Time::Now();
nit: should we use fixed time rather than base::Time::Now()?

https://codereview.chromium.org/1155063002/diff/170001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:89:
{
nit: are these blocks just for reusing the var name cb?

[nhiroki, 2015-05-27 14:52:39]

Thanks!

https://codereview.chromium.org/1155063002/diff/170001/content/browser/servic...
File
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc
(right):

https://codereview.chromium.org/1155063002/diff/170001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:84:
info->response_time = base::Time::Now();
On 2015/05/27 14:12:15, kinuko wrote:
> nit: should we use fixed time rather than base::Time::Now()?

Done.

https://codereview.chromium.org/1155063002/diff/170001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:89:
{
On 2015/05/27 14:12:15, kinuko wrote:
> nit: are these blocks just for reusing the var name cb?

Yes. Replaced them with cb1,2,3 to make consistent with SetUp().

[michaeln, 2015-05-27 23:09:23]

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
File content/browser/service_worker/service_worker_disk_cache_migrator.cc
(right):

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:133:
memmove(buffer->data(), http_info->metadata->data(), data_size);
This copy is unfortunate, i think there's an IOBuffer derivative than can wrap
an caller supplied ptr, WrappedIOBuffer. I think a derivative of that could hold
on to a reference to the refcounted info_buffer that owns the metadata.

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:229: if
(!base::StringToInt64((*entry)->GetKey(), &resource_id)) {
Does anything call (*entry)->Close() in this case or in the case of
OnOpenNextEntry not being called due to the weakptr becoming invalid?

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:239:
next_task->Run();
Looks like this will create and start tasks to migrate all entries in the cache
in parallel. Where each migrate tasks reads into memory all of the
header+complied script, then all of the script source. Is that right? Is there
anything that would prevent this from exceeding memory constraints on a low end
device given a large enough cache?

I know it's not in the CL yet, but I'm wondering how this is to be used?
- does regular browsing (minus sw registration/update) happen in parallel with
this migration occurring?
- does migration happen at startup and block first page load until completion?
- something in between?

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
File
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc
(right):

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:47:
class ServiceWorkerDiskCacheMigratorTest : public testing::Test {
> Hmmm... migrator tests are failing on linux_android_rel_ng maybe

Might have been because simple cache iterators are quirky. Reading or writing an
entry while iterating changes the order of iteration. I think you can end up
infinite looping. Thank goodness blockfile cache iters don't do that.

See relevant comments here
https://chromium.googlesource.com/chromium/src/+/941842cc490245e6bca0fdde1152...

[nhiroki, 2015-05-28 00:21:09]

Thank you for reviewing! Comment reply only. I'll update the patch later.

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
File content/browser/service_worker/service_worker_disk_cache_migrator.cc
(right):

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:239:
next_task->Run();
On 2015/05/27 23:09:23, michaeln wrote:
> Looks like this will create and start tasks to migrate all entries in the
cache
> in parallel. Where each migrate tasks reads into memory all of the
> header+complied script, then all of the script source. Is that right? Is there
> anything that would prevent this from exceeding memory constraints on a low
end
> device given a large enough cache?

In the current patchset, there is no mechanism to throttle the number of
inflight tasks and the migrator runs them greedily. We could throttle them
between OnOpenNextEntry and ContinueMigratingResources. I'll work on it.

> I know it's not in the CL yet, but I'm wondering how this is to be used?
> - does regular browsing (minus sw registration/update) happen in parallel with
> this migration occurring?
> - does migration happen at startup and block first page load until completion?
> - something in between?

2nd one, that is, the migration will happen during
ServiceWorkerStorage::LazyInitialize() like this:
https://codereview.chromium.org/1152543002/#ps180001

I hope this is acceptable because the migration happens only one time and
probably most of users don't have so many resources in the cache... (I'm going
to measure migration time by UMA in canary)

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
File
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc
(right):

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:47:
class ServiceWorkerDiskCacheMigratorTest : public testing::Test {
On 2015/05/27 23:09:23, michaeln wrote:
> > Hmmm... migrator tests are failing on linux_android_rel_ng maybe
> 
> Might have been because simple cache iterators are quirky. Reading or writing
an
> entry while iterating changes the order of iteration. I think you can end up
> infinite looping. Thank goodness blockfile cache iters don't do that.
> 
> See relevant comments here
>
https://chromium.googlesource.com/chromium/src/+/941842cc490245e6bca0fdde1152...

Good to know. Oh... only reading (no writing) could also cause infinite loop...?
DiskCache is so intricate :(

[nhiroki, 2015-05-28 06:18:48]

Patchset #8 (id:210001) has been deleted

[nhiroki, 2015-05-28 07:47:11]

Updated! Can you take another look?

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
File content/browser/service_worker/service_worker_disk_cache_migrator.cc
(right):

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:133:
memmove(buffer->data(), http_info->metadata->data(), data_size);
On 2015/05/27 23:09:23, michaeln wrote:
> This copy is unfortunate, i think there's an IOBuffer derivative than can wrap
> an caller supplied ptr, WrappedIOBuffer. I think a derivative of that could
hold
> on to a reference to the refcounted info_buffer that owns the metadata.

I see. I replaced memmove with WrapppedIOBuffer, and made
OnWriteResponseMetadata callback to keep |info_buffer|.

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:229: if
(!base::StringToInt64((*entry)->GetKey(), &resource_id)) {
On 2015/05/27 23:09:23, michaeln wrote:
> Does anything call (*entry)->Close() in this case or in the case of
> OnOpenNextEntry not being called due to the weakptr becoming invalid?

Good point. To fix this, I introduced WrappedEntry to hold an open entry and
ensure that the entry gets closed on the dtor.

https://codereview.chromium.org/1155063002/diff/190001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:239:
next_task->Run();
On 2015/05/28 00:21:09, nhiroki wrote:
> On 2015/05/27 23:09:23, michaeln wrote:
> > Looks like this will create and start tasks to migrate all entries in the
> cache
> > in parallel. Where each migrate tasks reads into memory all of the
> > header+complied script, then all of the script source. Is that right? Is
there
> > anything that would prevent this from exceeding memory constraints on a low
> end
> > device given a large enough cache?
> 
> In the current patchset, there is no mechanism to throttle the number of
> inflight tasks and the migrator runs them greedily. We could throttle them
> between OnOpenNextEntry and ContinueMigratingResources. I'll work on it.

Done.

> > I know it's not in the CL yet, but I'm wondering how this is to be used?
> > - does regular browsing (minus sw registration/update) happen in parallel
with
> > this migration occurring?
> > - does migration happen at startup and block first page load until
completion?
> > - something in between?
> 
> 2nd one, that is, the migration will happen during
> ServiceWorkerStorage::LazyInitialize() like this:
> https://codereview.chromium.org/1152543002/#ps180001
> 
> I hope this is acceptable because the migration happens only one time and
> probably most of users don't have so many resources in the cache... (I'm going
> to measure migration time by UMA in canary)

[nhiroki, 2015-06-01 09:28:31]

michaeln@: ping?

[kinuko, 2015-06-02 12:38:55]

kinuko@chromium.org changed reviewers:
+ falken@chromium.org

[kinuko, 2015-06-02 12:42:52]

On 2015/06/01 09:28:31, nhiroki wrote:
> michaeln@: ping?

I'll try to take one more look (maybe tomorrow), though it'd be best if michael
can review.

Adding falken@ too, would you be able to take a look when you have time? I think
we want to make extra sure if it works. (In the sense in general it'd be better
landing this asap and see if it works on canary at least 3-4 days.

[michaeln, 2015-06-03 02:13:07]

modulo the race, i think this lgtm

https://codereview.chromium.org/1155063002/diff/230001/content/browser/servic...
File content/browser/service_worker/service_worker_disk_cache_migrator.cc
(right):

https://codereview.chromium.org/1155063002/diff/230001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:184: new
net::IOBuffer(entry_->GetDataSize(0));
ah, i was wondering why u kept the entry_ ptr around

https://codereview.chromium.org/1155063002/diff/230001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:315:
Complete(SERVICE_WORKER_OK);
Is there a race here? Is it possible for task->Run() to complete faster than
iter->OpenNextEntry()? Those two are set racing on lines 309 and 310. I think if
the task wins the race, the migrator may finish w/o having migrated everything.

[nhiroki, 2015-06-03 03:58:12]

Thank you for reviewing! Updated.

https://codereview.chromium.org/1155063002/diff/230001/content/browser/servic...
File content/browser/service_worker/service_worker_disk_cache_migrator.cc
(right):

https://codereview.chromium.org/1155063002/diff/230001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:184: new
net::IOBuffer(entry_->GetDataSize(0));
On 2015/06/03 02:13:07, michaeln wrote:
> ah, i was wondering why u kept the entry_ ptr around

I see. We only have to pass the datasize... updated.

https://codereview.chromium.org/1155063002/diff/230001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:315:
Complete(SERVICE_WORKER_OK);
On 2015/06/03 02:13:07, michaeln wrote:
> Is there a race here? Is it possible for task->Run() to complete faster than
> iter->OpenNextEntry()? Those two are set racing on lines 309 and 310. I think
if
> the task wins the race, the migrator may finish w/o having migrated
everything.

Good point. The race could happen depending on the DiskCache backend
implementation. I added |is_iterating| flag to make sure the iteration is done.

[kinuko, 2015-06-03 05:57:18]

lgtm (still)

[falken, 2015-06-03 06:45:33]

Haven't read unittest yet.

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
File content/browser/service_worker/service_worker_disk_cache_migrator.cc
(right):

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:258: //
ERR_FAILED means the iterator reaches the end of the enumeration.
nit: reached

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:284:
scoped_entry->GetDataSize(0), src_, dest_,
What's the 0 from?

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:325:
return;
nit: dont' need this return

[falken, 2015-06-03 08:35:56]

I think this lgtm too

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
File
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc
(right):

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:162:
void StartMigration() {
This really doesn't return until migration finishes, right? Maybe just name it
Migrate().

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:210:
StartMigration();
EXPECT_EQ(0, GetEntryCount(dest_.get()); here?

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:228:
// Migration should hit the limit, but not crash.
Not sure I understand what's going on here, how does this test that the limit
was respected? Or this test just meant to verify that it worked (that's fine I
think)?

[nhiroki, 2015-06-03 08:51:22]

Thank you! Updated.

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
File content/browser/service_worker/service_worker_disk_cache_migrator.cc
(right):

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:258: //
ERR_FAILED means the iterator reaches the end of the enumeration.
On 2015/06/03 06:45:33, falken wrote:
> nit: reached

Done.

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:284:
scoped_entry->GetDataSize(0), src_, dest_,
On 2015/06/03 06:45:33, falken wrote:
> What's the 0 from?

This is the data type and '0' means "ResponseInfo", so '1' is the correct number
for data... :(

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ap...

Replaced it with enum.

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator.cc:325:
return;
On 2015/06/03 06:45:33, falken wrote:
> nit: dont' need this return

Done.

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
File
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc
(right):

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:162:
void StartMigration() {
On 2015/06/03 08:35:56, falken wrote:
> This really doesn't return until migration finishes, right? Maybe just name it
> Migrate().

Done.

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:210:
StartMigration();
On 2015/06/03 08:35:55, falken wrote:
> EXPECT_EQ(0, GetEntryCount(dest_.get()); here?

Done.

https://codereview.chromium.org/1155063002/diff/250001/content/browser/servic...
content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:228:
// Migration should hit the limit, but not crash.
On 2015/06/03 08:35:56, falken wrote:
> Not sure I understand what's going on here, how does this test that the limit
> was respected? Or this test just meant to verify that it worked (that's fine I
> think)?

I meant the latter. Slightly updated the comment.

[nhiroki, 2015-06-03 09:20:15]

Patchset #13 (id:330001) has been deleted

[nhiroki, 2015-06-03 10:05:07]

Patchset #13 (id:350001) has been deleted

[nhiroki, 2015-06-03 10:05:17]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-06-03 10:05:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, kinuko@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1155063002/#ps370001
(title: "add DCHECKs")

[commit-bot: I haz the power, 2015-06-03 10:06:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1155063002/370001

[nhiroki, 2015-06-03 10:07:54]

Patchset #14 (id:390001) has been deleted

[nhiroki, 2015-06-03 12:52:02]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-06-03 12:52:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, kinuko@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1155063002/#ps370001
(title: "add DCHECKs")

[commit-bot: I haz the power, 2015-06-03 12:52:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1155063002/370001

[commit-bot: I haz the power, 2015-06-03 12:56:11]

Committed patchset #13 (id:370001)

[commit-bot: I haz the power, 2015-06-03 12:57:03]

Patchset 13 (id:??) landed as
https://crrev.com/255913a791a398eed5c352fa3ac220be178cc772
Cr-Commit-Position: refs/heads/master@{#332603}