Introduce url_list to the Response scheme of CacheStorage.

Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process sends the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache.match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249
Committed: https://crrev.com/756089ad4639bb917faac4b2f6a83c16370f10b6
Cr-Commit-Position: refs/heads/master@{#437241}

[horo, 2016-11-21 10:45:20]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 10:45:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 11:00:03]

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

[commit-bot: I haz the power, 2016-11-21 11:00:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2016-11-21 13:29:33]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 13:29:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 14:29:57]

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

[commit-bot: I haz the power, 2016-11-21 14:29:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2016-11-22 05:59:01]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 05:59:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 07:31:05]

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

[commit-bot: I haz the power, 2016-11-22 07:31:06]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-25 05:23:20]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-25 05:23:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-25 06:11:34]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-25 06:11:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-25 06:21:45]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-25 06:21:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-25 06:26:18]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-25 06:26:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-25 08:19:53]

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

[commit-bot: I haz the power, 2016-11-25 08:19:54]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-28 01:47:21]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 01:47:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 03:21:09]

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

[commit-bot: I haz the power, 2016-11-28 03:21:11]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-29 06:50:18]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 06:50:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-29 06:51:54]

Patchset #7 (id:120001) has been deleted

[horo, 2016-11-29 06:51:57]

Patchset #6 (id:100001) has been deleted

[horo, 2016-11-29 06:52:00]

Patchset #5 (id:80001) has been deleted

[horo, 2016-11-29 06:52:04]

Patchset #4 (id:60001) has been deleted

[horo, 2016-11-29 06:52:06]

Patchset #3 (id:40001) has been deleted

[horo, 2016-11-29 06:52:10]

Patchset #1 (id:1) has been deleted

[horo, 2016-11-29 06:52:12]

Patchset #1 (id:20001) has been deleted

[horo, 2016-11-29 07:19:08]

Description was changed from

==========
store url list

BUG=
==========

to

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread,FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process send the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache,match(request) returns the response to the rendere process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
rendere process.

BUG=658249
==========

[horo, 2016-11-29 07:19:50]

Description was changed from

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread,FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process send the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache,match(request) returns the response to the rendere process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
rendere process.

BUG=658249
==========

to

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process send the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache,match(request) returns the response to the rendere process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
rendere process.

BUG=658249
==========

[horo, 2016-11-29 07:20:41]

Description was changed from

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process send the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache,match(request) returns the response to the rendere process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
rendere process.

BUG=658249
==========

to

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process send the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache,match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249
==========

[horo, 2016-11-29 07:20:58]

Description was changed from

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process send the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache,match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249
==========

to

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process sends the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache,match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249
==========

[horo, 2016-11-29 07:55:37]

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

[horo, 2016-11-29 07:55:38]

falken@
Could you please review this?

[horo, 2016-11-29 07:58:35]

Description was changed from

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process sends the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache,match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249
==========

to

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process sends the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache.match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249
==========

[commit-bot: I haz the power, 2016-11-29 08:52:10]

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

[commit-bot: I haz the power, 2016-11-29 08:52:11]

Dry run: This issue passed the CQ dry run.

[falken, 2016-11-30 14:57:38]

Overall question: Why does cache storage need the whole |url_list| instead of
just |redirected|? Is it needed for .redirected and the security restriction or
are you adding it here in anticipation of some future use? I see the CL
description links to comments in the github issue but they don't really explain
why.

https://codereview.chromium.org/2516353002/diff/140001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2516353002/diff/140001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:1407:
UMA_HISTOGRAM_BOOLEAN("ServiceWorkerCache.Response.HasDepricatedURL",
s/Depricated/Deprecated

Given the size of this CL, maybe add UMA in a different patch? I tend to do that
anyway since I believe UMA can't be deleted from histograms.xml, it can only be
deprecated. So if this CL needs to be reverted, it's easier.

https://codereview.chromium.org/2516353002/diff/140001/content/common/service...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/2516353002/diff/140001/content/common/service...
content/common/service_worker/service_worker_types.cc:79:
std::unique_ptr<std::vector<GURL>> url_list,
just to be sure: the reason for changing the const& to unique_ptr here and below
is to reduce copying?

https://codereview.chromium.org/2516353002/diff/140001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/2516353002/diff/140001/content/public/common/...
content/public/common/resource_response_info.h:119: // The URL list of the
response which was fetched by the ServiceWorker.
change fetched to served?

https://codereview.chromium.org/2516353002/diff/140001/content/public/common/...
content/public/common/resource_response_info.h:120: // This may be empty if the
response was created inside the ServiceWorker.
s/may be/is/ and add "or if the response was not served by a service worker"

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/cache...
File content/renderer/cache_storage/cache_storage_dispatcher.cc (left):

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/cache...
content/renderer/cache_storage/cache_storage_dispatcher.cc:98: }
nice cleanup

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:699:
GetServiceWorkerResponseFromWebResponse(web_response),
ah this is nicer

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_type_util.cc (right):

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_type_util.cc:65: const
blink::WebVector<blink::WebURL> web_url_list) {
can this be const&

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_type_util.h (right):

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_type_util.h:9: #include <vector>
why are these needed in the .h file now?

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_type_util.h:12: #include
"url/gurl.h"
ditto

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:334:
DCHECK(response.url() == m_urlList.back());
nit: DCHECK_EQ

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:439: // case it will
only be empty if it was created through new Response().
nit: "This list" could plausibly mean |urlListViaServiceWorker| or |m_urlList|.

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Response.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Response.cpp:50: [](const blink::WebURL&
url) { return url; });
we shouldn't need all the blink:: here, no?

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ResponseTest.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:22: #include
"wtf/PtrUtil.h"
#include "wtf/Vector.h"?

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:72: ASSERT(response);
should we do ASSERT_EQ(1, webResponse->urlList()->size())?

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:127:
Vector<blink::KURL> urlList(1);
Sorry for the repeat comment aren't we in blink namespace here? Why do we need
blink:: everywhere?

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:384:
blink::WebURL WebURLResponse::originalURLViaServiceWorker() const {
Do we need all the blink::'s in the above lines?

[horo, 2016-12-01 07:42:14]

> Overall question: Why does cache storage need the whole |url_list| instead of
> just |redirected|? Is it needed for .redirected and the security restriction
or
> are you adding it here in anticipation of some future use? I see the CL
> description links to comments in the github issue but they don't really
explain
> why.

Good question. I believe we don't need to store the whole URL list for the
current spec. What we need is only the redirected flag. But it is possible that
the future spec change will require the URL list. So I want to store the URL
list to the CacheStorage.

https://codereview.chromium.org/2516353002/diff/140001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2516353002/diff/140001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:1407:
UMA_HISTOGRAM_BOOLEAN("ServiceWorkerCache.Response.HasDepricatedURL",
On 2016/11/30 14:57:37, falken ooo on Dec 1 wrote:
> s/Depricated/Deprecated
> 
> Given the size of this CL, maybe add UMA in a different patch? I tend to do
that
> anyway since I believe UMA can't be deleted from histograms.xml, it can only
be
> deprecated. So if this CL needs to be reverted, it's easier.

Done.
Added TODO.

https://codereview.chromium.org/2516353002/diff/140001/content/common/service...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/2516353002/diff/140001/content/common/service...
content/common/service_worker/service_worker_types.cc:79:
std::unique_ptr<std::vector<GURL>> url_list,
On 2016/11/30 14:57:37, falken ooo on Dec 1 wrote:
> just to be sure: the reason for changing the const& to unique_ptr here and
below
> is to reduce copying?

yes.

https://codereview.chromium.org/2516353002/diff/140001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/2516353002/diff/140001/content/public/common/...
content/public/common/resource_response_info.h:119: // The URL list of the
response which was fetched by the ServiceWorker.
On 2016/11/30 14:57:37, falken ooo on Dec 1 wrote:
> change fetched to served?

Done.

https://codereview.chromium.org/2516353002/diff/140001/content/public/common/...
content/public/common/resource_response_info.h:120: // This may be empty if the
response was created inside the ServiceWorker.
On 2016/11/30 14:57:37, falken ooo on Dec 1 wrote:
> s/may be/is/ and add "or if the response was not served by a service worker"

Done.

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_type_util.cc (right):

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_type_util.cc:65: const
blink::WebVector<blink::WebURL> web_url_list) {
On 2016/11/30 14:57:37, falken ooo on Dec 1 wrote:
> can this be const&

Done.

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_type_util.h (right):

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_type_util.h:9: #include <vector>
On 2016/11/30 14:57:37, falken ooo on Dec 1 wrote:
> why are these needed in the .h file now?

Done.

https://codereview.chromium.org/2516353002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_type_util.h:12: #include
"url/gurl.h"
On 2016/11/30 14:57:37, falken ooo on Dec 1 wrote:
> ditto

Done.

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:334:
DCHECK(response.url() == m_urlList.back());
On 2016/11/30 14:57:37, falken ooo on Dec 1 wrote:
> nit: DCHECK_EQ

DCHECK_EQ with KURL causes compile failure.

../../base/logging.h:587:3: error: no matching function for call to
'MakeCheckOpValueString'
  MakeCheckOpValueString(&ss, v2);
  ^~~~~~~~~~~~~~~~~~~~~~

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:439: // case it will
only be empty if it was created through new Response().
On 2016/11/30 14:57:37, falken ooo on Dec 1 wrote:
> nit: "This list" could plausibly mean |urlListViaServiceWorker| or
|m_urlList|.

Done.

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Response.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Response.cpp:50: [](const blink::WebURL&
url) { return url; });
On 2016/11/30 14:57:38, falken ooo on Dec 1 wrote:
> we shouldn't need all the blink:: here, no?

Done.

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ResponseTest.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:22: #include
"wtf/PtrUtil.h"
On 2016/11/30 14:57:38, falken ooo on Dec 1 wrote:
> #include "wtf/Vector.h"?

Done.

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:72: ASSERT(response);
On 2016/11/30 14:57:38, falken ooo on Dec 1 wrote:
> should we do ASSERT_EQ(1, webResponse->urlList()->size())?

Done.

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp:127:
Vector<blink::KURL> urlList(1);
On 2016/11/30 14:57:38, falken ooo on Dec 1 wrote:
> Sorry for the repeat comment aren't we in blink namespace here? Why do we need
> blink:: everywhere?

Done.

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):

https://codereview.chromium.org/2516353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:384:
blink::WebURL WebURLResponse::originalURLViaServiceWorker() const {
On 2016/11/30 14:57:38, falken ooo on Dec 1 wrote:
> Do we need all the blink::'s in the above lines?

Done.

[horo, 2016-12-01 08:39:01]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 08:39:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 11:11:48]

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

[commit-bot: I haz the power, 2016-12-01 11:11:49]

Dry run: This issue passed the CQ dry run.

[falken, 2016-12-05 01:22:34]

Round of comments. Regarding storing the url_list, it looks like Mozilla had a
similar question and decided to store it because of "expected future CSP
features" may need it:
https://bugzilla.mozilla.org/show_bug.cgi?id=1243792#c16
So I agree it makes sense to store it.

General nits throughout the patch:
- Apparently for initialisation:
 type xxx = base::MakeUnique<>();
is preferred over:
 type xxx(base::MakeUnique<>());
- I wonder if you could save lines of code using:
 std::vector<> xxx = {yyy};
(it's optional though, I wouldn't block the patch on this)

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:958: new
ServiceWorkerResponse(operation.response));
nit: this should probably be MakeUnique to be consistent with the other lines of
code this patch is adding in this file.

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:1405:
base::MakeUnique<std::vector<GURL>>());
nit: It looks like the codebase likes:
  std::unique_ptr<std::vector<GURL>> url_list =
base::MakeUnique<std::vector<GURL>>();

over 
  std::unique_ptr<std::vector<GURL>>
url_list(base::MakeUnique<std::vector<GURL>>());

See also docu at:
https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?q=makeunique&sq=p...

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:412: 
For consistency, the blank line should be between 408 and 409 instead of here.

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:422:
ServiceWorkerHeaderList>() /* cors_exposed_header_names */);
Can we factor out a function MakeResponse for creating body_response_,
body_response_query_, no_body_response_? This code is pretty dense; it's hard to
see which class members are initialized here.

https://codereview.chromium.org/2516353002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_response_info.h (right):

https://codereview.chromium.org/2516353002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_response_info.h:67: //
respondWith() to create this response. This is empty if the response was
I can see this being confusing. Maybe add as the second sentence: "For example,
if the service worker calls respondWith(fetch('http://example.com/a')) and
http://example.com/a redirects to http://example.net/b which redirects to
http://example.org/c, the URL list is the vector <"http://example.com/a",
"http://example.net/b", "http://example.org/c">."

https://codereview.chromium.org/2516353002/diff/160001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/2516353002/diff/160001/content/public/common/...
content/public/common/resource_response_info.h:121: // response was not served
by a service worker
Oops, can we just say "See
ServiceWorkerResponseInfo::url_list_via_service_worker()." as the second
sentence here, so the documentation doesn't need to be repeated everywhere? I
should have done this for this file in my comment patch.

https://codereview.chromium.org/2516353002/diff/160001/media/blink/multibuffe...
File media/blink/multibuffer_data_source_unittest.cc (right):

https://codereview.chromium.org/2516353002/diff/160001/media/blink/multibuffe...
media/blink/multibuffer_data_source_unittest.cc:773: std::vector<blink::WebURL>
urlList;
url_list (throughout this file)

Optional suggestion, could this just be:
std::vector<blink::WebURL> url_list = {GURL(kHttpUrl)};

throughout this patch? It's fairly common:
$ git gs "std::vector.* = {.*};"

https://codereview.chromium.org/2516353002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchResponseDataTest.cpp (right):

https://codereview.chromium.org/2516353002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchResponseDataTest.cpp:11: #include
"testing/gtest/include/gtest/gtest.h"
#include "wtf/Vector.h"

[horo, 2016-12-05 03:38:33]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 03:38:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-12-05 03:39:49]

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:958: new
ServiceWorkerResponse(operation.response));
On 2016/12/05 01:22:33, falken wrote:
> nit: this should probably be MakeUnique to be consistent with the other lines
of
> code this patch is adding in this file. 

Done.

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:1405:
base::MakeUnique<std::vector<GURL>>());
On 2016/12/05 01:22:33, falken wrote:
> nit: It looks like the codebase likes:
>   std::unique_ptr<std::vector<GURL>> url_list =
> base::MakeUnique<std::vector<GURL>>();
> 
> over 
>   std::unique_ptr<std::vector<GURL>>
> url_list(base::MakeUnique<std::vector<GURL>>());
> 
> See also docu at:
>
https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?q=makeunique&sq=p...

Done.

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:412: 
On 2016/12/05 01:22:33, falken wrote:
> For consistency, the blank line should be between 408 and 409 instead of here.

Done.

https://codereview.chromium.org/2516353002/diff/160001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:422:
ServiceWorkerHeaderList>() /* cors_exposed_header_names */);
On 2016/12/05 01:22:33, falken wrote:
> Can we factor out a function MakeResponse for creating body_response_,
> body_response_query_, no_body_response_? This code is pretty dense; it's hard
to
> see which class members are initialized here.

Done.

https://codereview.chromium.org/2516353002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_response_info.h (right):

https://codereview.chromium.org/2516353002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_response_info.h:67: //
respondWith() to create this response. This is empty if the response was
On 2016/12/05 01:22:33, falken wrote:
> I can see this being confusing. Maybe add as the second sentence: "For
example,
> if the service worker calls respondWith(fetch('http://example.com/a')) and
> http://example.com/a redirects to http://example.net/b which redirects to
> http://example.org/c, the URL list is the vector <"http://example.com/a",
> "http://example.net/b", "http://example.org/c">." 

Done.

https://codereview.chromium.org/2516353002/diff/160001/content/public/common/...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/2516353002/diff/160001/content/public/common/...
content/public/common/resource_response_info.h:121: // response was not served
by a service worker
On 2016/12/05 01:22:33, falken wrote:
> Oops, can we just say "See
> ServiceWorkerResponseInfo::url_list_via_service_worker()." as the second
> sentence here, so the documentation doesn't need to be repeated everywhere? I
> should have done this for this file in my comment patch.

Done.

https://codereview.chromium.org/2516353002/diff/160001/media/blink/multibuffe...
File media/blink/multibuffer_data_source_unittest.cc (right):

https://codereview.chromium.org/2516353002/diff/160001/media/blink/multibuffe...
media/blink/multibuffer_data_source_unittest.cc:773: std::vector<blink::WebURL>
urlList;
On 2016/12/05 01:22:33, falken wrote:
> url_list (throughout this file)
> 
> Optional suggestion, could this just be:
> std::vector<blink::WebURL> url_list = {GURL(kHttpUrl)};
> 
> throughout this patch? It's fairly common:
> $ git gs "std::vector.* = {.*};"

Done.

https://codereview.chromium.org/2516353002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchResponseDataTest.cpp (right):

https://codereview.chromium.org/2516353002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchResponseDataTest.cpp:11: #include
"testing/gtest/include/gtest/gtest.h"
On 2016/12/05 01:22:33, falken wrote:
> #include "wtf/Vector.h"

Done.

[falken, 2016-12-05 04:32:42]

lgtm, sorry for delays.

https://codereview.chromium.org/2516353002/diff/180001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/2516353002/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:411:
base::MakeUnique<std::vector<GURL>>());
= base::MakeUnique (and all occurrences below)

https://codereview.chromium.org/2516353002/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:412:
body_response_url_list->push_back(GURL("http://example.com/body.html"));
(optional) nit: was actually thinking CreateResponse could take the URL list
string directly (since these all just are 1 element vectors) so the function
looks like:

  body_response = CreateResponse("http://example.com/body.html", ...);

  body_response_with_query_url_list_ = CreateResponse(...);

Or it could take a std::vector for extensibility and get passed
{"http://example.com/body.html"}

Or maybe just pass base::MakeUnique<std:vector<GURL>>(1,
"http://example.com/body.html")

https://codereview.chromium.org/2516353002/diff/180001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2516353002/diff/180001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:257:
url_list.push_back(url_);
nit: could do url_list = {url_} here

https://codereview.chromium.org/2516353002/diff/180001/media/blink/multibuffe...
File media/blink/multibuffer_data_source_unittest.cc (right):

https://codereview.chromium.org/2516353002/diff/180001/media/blink/multibuffe...
media/blink/multibuffer_data_source_unittest.cc:1513:
urlList.push_back(GURL(kHttpDifferentOriginUrl));
nit: url_list. also urlList = {GURL(...)} for consistency

https://codereview.chromium.org/2516353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/ResourceResponse.h (right):

https://codereview.chromium.org/2516353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/ResourceResponse.h:474: // This may
be empty if the response was created inside the ServiceWorker.
not from your patch, but I'd prefer "is empty" instead of "may be".

[horo, 2016-12-05 04:56:25]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 04:56:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-12-05 04:57:03]

Patchset #4 (id:200001) has been deleted

[horo, 2016-12-05 04:58:12]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 04:58:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-12-05 04:58:32]

Thank you.

https://codereview.chromium.org/2516353002/diff/180001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/2516353002/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:411:
base::MakeUnique<std::vector<GURL>>());
On 2016/12/05 04:32:42, falken wrote:
> = base::MakeUnique (and all occurrences below)

Ah, sorry. I didn't save this file correctly :(.
Done.

https://codereview.chromium.org/2516353002/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:412:
body_response_url_list->push_back(GURL("http://example.com/body.html"));
On 2016/12/05 04:32:42, falken wrote:
> (optional) nit: was actually thinking CreateResponse could take the URL list
> string directly (since these all just are 1 element vectors) so the function
> looks like:
> 
>   body_response = CreateResponse("http://example.com/body.html", ...);
> 
>   body_response_with_query_url_list_ = CreateResponse(...);
> 
> Or it could take a std::vector for extensibility and get passed
> {"http://example.com/body.html"}
> 
> Or maybe just pass base::MakeUnique<std:vector<GURL>>(1,
> "http://example.com/body.html")

Done.

https://codereview.chromium.org/2516353002/diff/180001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2516353002/diff/180001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:257:
url_list.push_back(url_);
On 2016/12/05 04:32:42, falken wrote:
> nit: could do url_list = {url_} here

Done.

https://codereview.chromium.org/2516353002/diff/180001/media/blink/multibuffe...
File media/blink/multibuffer_data_source_unittest.cc (right):

https://codereview.chromium.org/2516353002/diff/180001/media/blink/multibuffe...
media/blink/multibuffer_data_source_unittest.cc:1513:
urlList.push_back(GURL(kHttpDifferentOriginUrl));
On 2016/12/05 04:32:42, falken wrote:
> nit: url_list. also urlList = {GURL(...)} for consistency

Done.

https://codereview.chromium.org/2516353002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/ResourceResponse.h (right):

https://codereview.chromium.org/2516353002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/ResourceResponse.h:474: // This may
be empty if the response was created inside the ServiceWorker.
On 2016/12/05 04:32:42, falken wrote:
> not from your patch, but I'd prefer "is empty" instead of "may be".

Done.

[horo, 2016-12-05 05:00:46]

horo@chromium.org changed reviewers:
+ nhiroki@chromium.org

[horo, 2016-12-05 05:00:46]

nhiroki@
Could you please review content/browser/cache_storage/* and
content/renderer/cache_storage/*?

[commit-bot: I haz the power, 2016-12-05 06:49:53]

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

[commit-bot: I haz the power, 2016-12-05 06:49:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nhiroki, 2016-12-05 07:21:28]

nhiroki@chromium.org changed reviewers:
+ jsbell@chromium.org

[nhiroki, 2016-12-05 07:25:13]

+jsbell@ because this changes a serialization format of the cache storage and
probably storage folks might want to check it.

[jsbell, 2016-12-05 17:21:15]

jsbell@chromium.org changed reviewers:
+ cmumford@chromium.org

[jsbell, 2016-12-05 17:21:16]

cmumford@ - can you take a look?

This may conflict with the work you and jkarlin@ are doing.

[cmumford, 2016-12-05 19:24:19]

What about adding a new tests with multiple URL's. I think all tests still have
a single request URL.

https://codereview.chromium.org/2516353002/diff/220001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2516353002/diff/220001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:1406: // TODO(horo) Add UMA
for the deprecated URL field.
So I believe the index will only be written if a cache is added/deleted to/from
the store? I wonder how often this happens once the store is establish and
initially populated? Another option would be to upgrade the index to the new
protobuf format if the old format is encountered.

https://codereview.chromium.org/2516353002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/cachestorage/CacheTest.cpp (right):

https://codereview.chromium.org/2516353002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/cachestorage/CacheTest.cpp:197:
EXPECT_EQ(expectedBatchOperations[i].response.urlList().size(),
Should this be an ASSERT_EQ to avoid possible crash in test?

https://codereview.chromium.org/2516353002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/cachestorage/CacheTest.cpp:659:
webResponses[1].setURLList(urlList2);
Do you think this is more simple, or more complex?

webResponses[1].setURLList(std::vector<KURL>({KURL(ParsedURLString, url2)}));

[horo, 2016-12-06 05:47:36]

I created a cl which add LayoutTests for URL list.
https://codereview.chromium.org/2550363002#ps1

Do you think I should merge into this CL?

https://codereview.chromium.org/2516353002/diff/220001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2516353002/diff/220001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:1406: // TODO(horo) Add UMA
for the deprecated URL field.
> So I believe the index will only be written if a cache is added/deleted
to/from
> the store?

Yes.

> I wonder how often this happens once the store is establish and
> initially populated? Another option would be to upgrade the index to the new
> protobuf format if the old format is encountered.

I prefer the current code from both performance and code complexity points of
view.

https://codereview.chromium.org/2516353002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/cachestorage/CacheTest.cpp (right):

https://codereview.chromium.org/2516353002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/cachestorage/CacheTest.cpp:197:
EXPECT_EQ(expectedBatchOperations[i].response.urlList().size(),
On 2016/12/05 19:24:19, cmumford wrote:
> Should this be an ASSERT_EQ to avoid possible crash in test?

Done.

https://codereview.chromium.org/2516353002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/cachestorage/CacheTest.cpp:659:
webResponses[1].setURLList(urlList2);
On 2016/12/05 19:24:19, cmumford wrote:
> Do you think this is more simple, or more complex?
> 
> webResponses[1].setURLList(std::vector<KURL>({KURL(ParsedURLString, url2)}));

Done.

[cmumford, 2016-12-06 17:31:46]

lgtm

[horo, 2016-12-07 01:07:45]

horo@chromium.org changed reviewers:
+ mkwst@chromium.org

[horo, 2016-12-07 01:07:46]

mkwst@
Could you please review  third_party/WebKit/*?

[horo, 2016-12-07 01:11:29]

horo@chromium.org changed reviewers:
+ hubbe@chromium.org

[horo, 2016-12-07 01:11:30]

hubbe@
Could you please review media/blink/multibuffer_data_source_unittest.cc?

[hubbe, 2016-12-07 01:13:36]

On 2016/12/07 01:11:30, horo wrote:
> hubbe@
> Could you please review media/blink/multibuffer_data_source_unittest.cc?

media/blink/multibuffer_data_source_unittest.cc LGTM

[horo, 2016-12-07 01:13:45]

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

[horo, 2016-12-07 01:13:45]

kinuko@
Could you please review content/child/web_url_loader_impl.cc
content/public/common/resource_response.cc
content/public/common/resource_response_info.h?

[nhiroki, 2016-12-07 02:05:57]

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ResponseTest.cpp (right):

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:40: std::vector<WebURL>
urlList;
WTF::Vector

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:60: Vector<KURL>
urlList;
Can you add WTF:: prefix here and elsewhere?

Context:
https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-...

[horo, 2016-12-07 03:36:10]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 03:36:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-12-07 03:37:48]

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ResponseTest.cpp (right):

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:40: std::vector<WebURL>
urlList;
On 2016/12/07 02:05:57, nhiroki wrote:
> WTF::Vector

Done.

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:60: Vector<KURL>
urlList;
On 2016/12/07 02:05:57, nhiroki wrote:
> Can you add WTF:: prefix here and elsewhere?
> 
> Context:
>
https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-...

I think the discussion is not settled yet?

[kinuko, 2016-12-07 04:08:42]

lgtm % nits

https://codereview.chromium.org/2516353002/diff/240001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2516353002/diff/240001/content/child/web_url_...
content/child/web_url_loader_impl.cc:1060:
response->setURLListViaServiceWorker(url_list_via_service_worker);
I think this can likely just take info.url_list_via_service_worker?  (WebURL <->
GURL conversion is implicit, WebVector takes vector)

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLResponse.h:262: const
WebVector<WebURL>&);
(not directly related to this patch, but at some point we might want to factor
out SW-related part into its own struct...)

[nhiroki, 2016-12-07 04:16:07]

lgtm

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ResponseTest.cpp (right):

https://codereview.chromium.org/2516353002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ResponseTest.cpp:60: Vector<KURL>
urlList;
On 2016/12/07 03:37:48, horo wrote:
> On 2016/12/07 02:05:57, nhiroki wrote:
> > Can you add WTF:: prefix here and elsewhere?
> > 
> > Context:
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-...
> 
> I think the discussion is not settled yet?

IMO, at least new code should have the prefix because it doesn't pollute
git-blame and it can automatically be replaced with the new prefix "wtf::" by
scripts. Anyway, the discussion is not settled yet and I'll leave it up to you.

[commit-bot: I haz the power, 2016-12-07 05:28:22]

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

[commit-bot: I haz the power, 2016-12-07 05:28:23]

Dry run: This issue passed the CQ dry run.

[horo, 2016-12-07 05:30:05]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 05:30:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-12-07 05:31:44]

Thank you.

https://codereview.chromium.org/2516353002/diff/240001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2516353002/diff/240001/content/child/web_url_...
content/child/web_url_loader_impl.cc:1060:
response->setURLListViaServiceWorker(url_list_via_service_worker);
On 2016/12/07 04:08:42, kinuko wrote:
> I think this can likely just take info.url_list_via_service_worker?  (WebURL
<->
> GURL conversion is implicit, WebVector takes vector)

Done.

[commit-bot: I haz the power, 2016-12-07 09:15:02]

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

[commit-bot: I haz the power, 2016-12-07 09:15:03]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-12-08 09:12:41]

LGTM (sorry for the delayed response).

[horo, 2016-12-08 12:33:21]

The CQ bit was checked by horo@chromium.org

[horo, 2016-12-08 12:33:21]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
hubbe@chromium.org, cmumford@chromium.org, nhiroki@chromium.org,
kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2516353002/#ps280001
(title: "implicit conversion WebURL <-> GURL and WebVector <- vector")

[commit-bot: I haz the power, 2016-12-08 12:33:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 14:34:07]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1481200401292520,
"parent_rev": "2fcc24d6ff6a86139c5160e79e77de52d040eef1", "commit_rev":
"37fd2f283bf8945585bfeef6cf4ace48fd16da01"}

[commit-bot: I haz the power, 2016-12-08 14:34:35]

Description was changed from

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process sends the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache.match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249
==========

to

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process sends the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache.match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249
==========

[commit-bot: I haz the power, 2016-12-08 14:34:37]

Committed patchset #7 (id:280001)

[commit-bot: I haz the power, 2016-12-08 14:37:04]

Description was changed from

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process sends the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache.match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249
==========

to

==========
Introduce url_list to the Response scheme of CacheStorage.

According to the spec, the CacheStorage has to store the URL list of responses.
https://github.com/w3c/ServiceWorker/issues/737#issuecomment-175844226
So this cl introduces url_list to the Response scheme of CacheStorage, and pass
it from FetchManager::Loader to CacheStorage and ServiceWorkerURLRequestJob and
ResourceResponse.

To let FetchManager::Loader know the URL list, this cl introduces
ThreadableLoaderClient::didReceiveRedirectTo(). DocumentThreadableLoader calls
didReceiveRedirectTo() when it received a redirect response.
- If fetch() in called on the main thread, FetchManager::Loader's
  didReceiveRedirectTo() is called directly from DocumentThreadableLoader.
- If fetch() is called on the worker thread, FetchManager::Loader's
  didReceiveRedirectTo() is called via WorkerThreadableLoader::
  MainThreadLoaderHolder.
And FetchManager::Loader::didReceiveResponse() sets the URL list to
FetchResponseData.

When FetchEvent.respondWith(response) is called in the SW, the URL list is
passed to ServiceWorkerURLRequestJob via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And when the browser process sends the response
to the controlled page, the URL list is passed via content::ResourceResponseInfo
and blink::WebURLResponse and blink::ResourceResponse.

If Cache.put(request, response) is called, the URL list is passed to
CacheStorageCache::Put() via blink::WebServiceWorkerResponse and
content::ServiceWorkerResponse. And it converts the list to the protobuf and
save to the storage.

When Cache.match(request) returns the response to the renderer process, the URL
list is read from the protobuf in the storage and passed via
content::ServiceWorkerResponse and blink::WebServiceWorkerResponse to the
renderer process.

BUG=658249

Committed: https://crrev.com/756089ad4639bb917faac4b2f6a83c16370f10b6
Cr-Commit-Position: refs/heads/master@{#437241}
==========

[commit-bot: I haz the power, 2016-12-08 14:37:06]

Patchset 7 (id:??) landed as
https://crrev.com/756089ad4639bb917faac4b2f6a83c16370f10b6
Cr-Commit-Position: refs/heads/master@{#437241}