[ServiceWorker] Don't send the UMA related headers to the ServiceWorker.

[ServiceWorker] Don't send the UMA related headers to the ServiceWorker.

Chrome add "X-Chrome-UMA-Enabled" (and sometimes "X-Client-Data") headers while accessing Google servers if UMA is enabled.
This header is added before sending the request to the ServiceWorker.
So the request which is passed to the ServiceWorker contains this header.

If event.request.mode is 'cors' and event.request.headers has "x-chrome-uma-enabled", fetch(event.request) send a CORS preflight request.
This preflight request contains "Access-Control-Request-Headers: x-chrome-uma-enabled" header.
But the response from the Google server doesn't contain "Access-Control-Allow-Headers: x-chrome-uma-enabled" header.
So this fetch fails.

To avoid this problem this patch removes these headers before sending the request to the ServiceWorker.

This patch introduces ServiceWorkerContext::AddExcludedHeadersForFetchEvent() method and changes DevToolsNetworkTransactionFactory to call this method.

BUG=425649
Committed: https://crrev.com/e09b6c8616f4a8e45e91a171eedddefde9522ac2
Cr-Commit-Position: refs/heads/master@{#302379}

[horo, 2014-10-22 13:38:01]

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

[horo, 2014-10-22 13:38:01]

michaeln@
Could you please review this?

[michaeln, 2014-10-22 22:19:41]

https://codereview.chromium.org/666973003/diff/1/components/variations/variat...
File components/variations/variations_http_header_provider.cc (right):

https://codereview.chromium.org/666973003/diff/1/components/variations/variat...
components/variations/variations_http_header_provider.cc:118: :
variation_ids_cache_initialized_(false) {
Instead of hand coding matching values in two places, would it make sense for
chrome to inform the content lib about which headers to exlcude?

static voie ServiceWorkerContext::ExcludeHeadersFromFetchEvents(xxx);

https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_url_request_job.cc:42: // Keep in
sync with variations_http_header_provider.cc.
Having to manually keep these files in sysnc is probably not a good idea.

[horo, 2014-10-23 07:09:37]

Patchset #2 (id:20001) has been deleted

[horo, 2014-10-23 07:19:35]

On 2014/10/22 22:19:41, michaeln wrote:
>
https://codereview.chromium.org/666973003/diff/1/components/variations/variat...
> File components/variations/variations_http_header_provider.cc (right):
> 
>
https://codereview.chromium.org/666973003/diff/1/components/variations/variat...
> components/variations/variations_http_header_provider.cc:118: :
> variation_ids_cache_initialized_(false) {
> Instead of hand coding matching values in two places, would it make sense for
> chrome to inform the content lib about which headers to exlcude?
> 
> static voie ServiceWorkerContext::ExcludeHeadersFromFetchEvents(xxx);
> 
>
https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
> File content/browser/service_worker/service_worker_url_request_job.cc (right):
> 
>
https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
> content/browser/service_worker/service_worker_url_request_job.cc:42: // Keep
in
> sync with variations_http_header_provider.cc.
> Having to manually keep these files in sysnc is probably not a good idea.

Yes.
It is not a good idea.
But according to components/variations.gypi
https://chromium.googlesource.com/chromium/src/+/master/components/variations...
 # List of dependencies is intentionally very minimal. Please avoid
 # adding extra dependencies without first checking with OWNERS.

So I don't think that making the dependency from components/variations to
content_browser is a good idea.

[michaeln, 2014-10-23 21:27:41]

michaeln@chromium.org changed reviewers:
+ asvitkine@chromium.org, jwd@chromium.org, stevet@chromium.org

[michaeln, 2014-10-23 21:36:21]

Hmm, the comment is on a different target, we're interested in the
variations_http_provider target? Maybe it's ok to depend on the content lib
there? Lets see what the variations owners think. If duplicated the strings is
the best option, so be it, but I do see other components that have dependencies
on the content lib.

[Alexei Svitkine (slow), 2014-10-23 21:41:43]

Why is the problem unique to service workers? What about other cors requests?

Also, since these headers aren't coming from the content area, perhaps they
shouldn't be considered for Access-Control-Request-Headers?

[michaeln1, 2014-10-23 22:13:46]

On 2014/10/23 21:41:43, Alexei Svitkine wrote:
> Why is the problem unique to service workers? What about other cors requests?

ServiceWorkers siphon off outgoing URLRequests and send them to a script for
processing. The variation headers are applied prior to that interception point.

> Also, since these headers aren't coming from the content area, perhaps they
> shouldn't be considered for Access-Control-Request-Headers?

Exactly.

As currently coded, a representation of the request is sent into the
servcieworker script (a content area) with the variation headers attached. The
script then issues a new request and the variation headers are mistakenly
treated as if they're from a 'content area' at that time. The CL filters out the
var headers so they're not given to the sw script and they're not considered 
Access-Control-Request-Headers when a new request is cons'd up by the script.

Horo knows more about how CORS and ServiceWorkers are stitched together.

[horo, 2014-10-24 06:55:31]

On 2014/10/23 22:13:46, michaeln1 wrote:
> On 2014/10/23 21:41:43, Alexei Svitkine wrote:
> > Why is the problem unique to service workers? What about other cors
requests?
> 
> ServiceWorkers siphon off outgoing URLRequests and send them to a script for
> processing. The variation headers are applied prior to that interception
point.
> 
> > Also, since these headers aren't coming from the content area, perhaps they
> > shouldn't be considered for Access-Control-Request-Headers?
> 
> Exactly.
> 
> As currently coded, a representation of the request is sent into the
> servcieworker script (a content area) with the variation headers attached. The
> script then issues a new request and the variation headers are mistakenly
> treated as if they're from a 'content area' at that time. The CL filters out
the
> var headers so they're not given to the sw script and they're not considered 
> Access-Control-Request-Headers when a new request is cons'd up by the script.
> 
> Horo knows more about how CORS and ServiceWorkers are stitched together.

I think wheter the request is CORS or not, the headers which are added outside
the content area should not be passed to the ServiceWorker.

The best solution is stop calling VariationsHttpHeaderProvider::AppendHeaders()
when the request will be sent to the ServiceWorker.
But we can't know whether the request will be sent to the ServiceWorker or not
when ChromeResourceDispatcherHostDelegate::RequestBeginning() is called.

[Alexei Svitkine (slow), 2014-10-24 15:35:48]

https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_url_request_job.cc:42: // Keep in
sync with variations_http_header_provider.cc.
On 2014/10/22 22:19:40, michaeln wrote:
> Having to manually keep these files in sysnc is probably not a good idea.

Agreed.

I think we probably want to have a IsVariationsHeader(name) method or similar on
VariationsHttpHeaderProvider and call this from here (probably through an
interface, since this code is content likely shouldn't depend on the variations
component).

[michaeln, 2014-10-24 19:49:50]

> I think wheter the request is CORS or not, the headers which are added outside
> the content area should not be passed to the ServiceWorker.

That's definitely true for the uma headers.

https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_url_request_job.cc:42: // Keep in
sync with variations_http_header_provider.cc.
On 2014/10/24 15:35:48, Alexei Svitkine wrote:
> On 2014/10/22 22:19:40, michaeln wrote:
> > Having to manually keep these files in sysnc is probably not a good idea.
> 
> Agreed.
> 
> I think we probably want to have a IsVariationsHeader(name) method or similar
on
> VariationsHttpHeaderProvider and call this from here (probably through an
> interface, since this code is content likely shouldn't depend on the
variations
> component).

The content lib cannot depend on variations component, but i believe there's no
issue with variations_http_header_provider component depending on the content
lib. This is why i was suggesting an content/public interface for the
variations_http_header_provider to invoke that would inform the sw system about
which to exclude. I had in mind a one-time call that would add values to a
global map.

[horo, 2014-10-27 14:15:41]

https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/666973003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_url_request_job.cc:42: // Keep in
sync with variations_http_header_provider.cc.
On 2014/10/24 19:49:50, michaeln wrote:
> On 2014/10/24 15:35:48, Alexei Svitkine wrote:
> > On 2014/10/22 22:19:40, michaeln wrote:
> > > Having to manually keep these files in sysnc is probably not a good idea.
> > 
> > Agreed.
> > 
> > I think we probably want to have a IsVariationsHeader(name) method or
similar
> on
> > VariationsHttpHeaderProvider and call this from here (probably through an
> > interface, since this code is content likely shouldn't depend on the
> variations
> > component).
> 
> The content lib cannot depend on variations component, but i believe there's
no
> issue with variations_http_header_provider component depending on the content
> lib. This is why i was suggesting an content/public interface for the
> variations_http_header_provider to invoke that would inform the sw system
about
> which to exclude. I had in mind a one-time call that would add values to a
> global map.

Ah. I didn't notice that there is 'variations_http_provider' target in
variations.gypi.

I uploaded Patch Set 2.
Please review this.

[Alexei Svitkine (slow), 2014-10-27 15:09:00]

https://codereview.chromium.org/666973003/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/666973003/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:18:
"X-DevTools-Request-Initiator";
Why not move these to devtools_network_transaction_factory.cc where they're
actually used? Then you don't need to make them class members.

https://codereview.chromium.org/666973003/diff/40001/components/variations/ne...
File components/variations/net/variations_http_header_provider.cc (right):

https://codereview.chromium.org/666973003/diff/40001/components/variations/ne...
components/variations/net/variations_http_header_provider.cc:17: #include
"content/public/browser/service_worker_context.h"
Hmm, I don't think we want to do this.

This code is in a component, partly because it's used by the iOS port of
Chromium. Which shouldn't depend on the content API.

https://codereview.chromium.org/666973003/diff/40001/components/variations/ne...
components/variations/net/variations_http_header_provider.cc:119:
content::ServiceWorkerContext::AddExcludedHeaderNameForFetchEvent(
Since this code can't depend on content/ per my comment above, my suggestion is
to expose a getter from this class for the headers. Since this class is a
singleton, then some initialization code in chrome/browser can query that getter
and call the ServiceWorkerContext::AddExcludeHeaderNameForFetchEvent() on them.

https://codereview.chromium.org/666973003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/666973003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:292: if
(ServiceWorkerContext::IsExcludedHeaderNameForFetchEvent(it.name())) {
Nit: No {}'s since body fits one line.

[michaeln, 2014-10-27 21:50:26]

https://codereview.chromium.org/666973003/diff/40001/components/variations/ne...
File components/variations/net/variations_http_header_provider.cc (right):

https://codereview.chromium.org/666973003/diff/40001/components/variations/ne...
components/variations/net/variations_http_header_provider.cc:119:
content::ServiceWorkerContext::AddExcludedHeaderNameForFetchEvent(
On 2014/10/27 15:09:00, Alexei Svitkine wrote:
> Since this code can't depend on content/ per my comment above, my suggestion
is
> to expose a getter from this class for the headers. Since this class is a
> singleton, then some initialization code in chrome/browser can query that
getter
> and call the ServiceWorkerContext::AddExcludeHeaderNameForFetchEvent() on
them.

Sounds reasonable, maybe the ChromeResourceDispatcherHostDelegate could arrange
to do that initialization on the IO thread.

[horo, 2014-10-28 02:35:05]

https://codereview.chromium.org/666973003/diff/40001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/666973003/diff/40001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:18:
"X-DevTools-Request-Initiator";
On 2014/10/27 15:09:00, Alexei Svitkine wrote:
> Why not move these to devtools_network_transaction_factory.cc where they're
> actually used? Then you don't need to make them class members.

They are used in DevToolsNetworkTransaction::ProcessRequest().
But DevToolsNetworkTransaction is created per every network transaction.
So I think we should call AddExcludedHeaderNameForFetchEvent() in the
constructor of DevToolsNetworkTransactionFactory which is created per profiles.

https://codereview.chromium.org/666973003/diff/40001/components/variations/ne...
File components/variations/net/variations_http_header_provider.cc (right):

https://codereview.chromium.org/666973003/diff/40001/components/variations/ne...
components/variations/net/variations_http_header_provider.cc:119:
content::ServiceWorkerContext::AddExcludedHeaderNameForFetchEvent(
On 2014/10/27 21:50:26, michaeln wrote:
> On 2014/10/27 15:09:00, Alexei Svitkine wrote:
> > Since this code can't depend on content/ per my comment above, my suggestion
> is
> > to expose a getter from this class for the headers. Since this class is a
> > singleton, then some initialization code in chrome/browser can query that
> getter
> > and call the ServiceWorkerContext::AddExcludeHeaderNameForFetchEvent() on
> them.
> 
> Sounds reasonable, maybe the ChromeResourceDispatcherHostDelegate could
arrange
> to do that initialization on the IO thread.

Done.

https://codereview.chromium.org/666973003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/666973003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:292: if
(ServiceWorkerContext::IsExcludedHeaderNameForFetchEvent(it.name())) {
On 2014/10/27 15:09:00, Alexei Svitkine wrote:
> Nit: No {}'s since body fits one line.

Done.

[Alexei Svitkine (slow), 2014-10-28 13:32:51]

https://codereview.chromium.org/666973003/diff/60001/components/variations/ne...
File components/variations/net/variations_http_header_provider.h (right):

https://codereview.chromium.org/666973003/diff/60001/components/variations/ne...
components/variations/net/variations_http_header_provider.h:54:
std::vector<std::string> GetExcludedHeadersForFetchEvent() const;
I wouldn't use the term "Excluded" here, since that implies knowledge of how the
returned value is used.

In fact, this should just return the header names that this class adds without
any other meaning. So the method could be named GetVariationHeaderNames().

Please add a comment on the method and #include <vector> at the top of the file.

[horo, 2014-10-28 15:35:48]

https://codereview.chromium.org/666973003/diff/60001/components/variations/ne...
File components/variations/net/variations_http_header_provider.h (right):

https://codereview.chromium.org/666973003/diff/60001/components/variations/ne...
components/variations/net/variations_http_header_provider.h:54:
std::vector<std::string> GetExcludedHeadersForFetchEvent() const;
On 2014/10/28 13:32:50, Alexei Svitkine wrote:
> I wouldn't use the term "Excluded" here, since that implies knowledge of how
the
> returned value is used.
> 
> In fact, this should just return the header names that this class adds without
> any other meaning. So the method could be named GetVariationHeaderNames().
> 
> Please add a comment on the method and #include <vector> at the top of the
file.

Done.

[Alexei Svitkine (slow), 2014-10-28 16:47:04]

variations/ lgtm

[michaeln, 2014-10-28 20:57:22]

lgtm 2

https://codereview.chromium.org/666973003/diff/80001/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/666973003/diff/80001/content/public/browser/s...
content/public/browser/service_worker_context.h:34: const
std::vector<std::string> header_names);
can this be a const ref?

[horo, 2014-10-29 01:56:25]

Thank you!

https://codereview.chromium.org/666973003/diff/80001/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/666973003/diff/80001/content/public/browser/s...
content/public/browser/service_worker_context.h:34: const
std::vector<std::string> header_names);
On 2014/10/28 20:57:22, michaeln wrote:
> can this be a const ref?

Done.

[horo, 2014-10-29 02:01:24]

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

[horo, 2014-10-29 02:01:25]

vsevik@
Could you please review chrome/browser/devtoos/*?

Thank you!

[vsevik, 2014-10-29 09:12:40]

devtools lgtm

[horo, 2014-10-29 09:22:43]

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

[horo, 2014-10-29 09:22:43]

thestig@
Could you please review
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc?

[horo, 2014-10-29 09:23:27]

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

[horo, 2014-10-29 09:23:28]

sievers@
Could you please review content/public/browser/service_worker_context.*?

[no sievers, 2014-10-29 21:19:58]

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
File content/public/browser/service_worker_context.cc (right):

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.cc:19: 
Do you think the static functions could move to the impl class?
It's a bit discouraged by the content api guide to have .cc files for this.

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.cc:23: BrowserThread::PostTask(
Could you let the caller handle that? Then you can leave the API like it is i.e.
IO-thread only, see header comment.

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.h:8: #include <set>
set is unused

[Lei Zhang, 2014-10-29 21:47:21]

https://codereview.chromium.org/666973003/diff/100001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/666973003/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:275:
content::ServiceWorkerContext::AddExcludedHeadersForFetchEvent(
This looks like essentially a static function call. Why do we have to do it
every time a ChromeResourceDispatcherHostDelegate gets created?

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
File content/public/browser/service_worker_context.cc (right):

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.cc:16: static
base::LazyInstance<HeaderNameSet> g_excluded_header_name_set =
you: you don't need static in an anonymous namespace.

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.h:20: // the IO thread except for
AddExcludedHeadersForFetchEvent.
With the code as is, do you mean to say "except for
AddExcludedHeadersForFetchEvent() which can be called from any thread" ?

[horo, 2014-10-30 05:20:55]

Patchset #7 (id:140001) has been deleted

[horo, 2014-10-30 07:54:35]

https://codereview.chromium.org/666973003/diff/100001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/666973003/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:275:
content::ServiceWorkerContext::AddExcludedHeadersForFetchEvent(
On 2014/10/29 21:47:21, Lei Zhang wrote:
> This looks like essentially a static function call. Why do we have to do it
> every time a ChromeResourceDispatcherHostDelegate gets created?

I think ChromeResourceDispatcherHostDelegate is created only once.
But it is better to call this on IO thread.
So I moved this to IOThread::InitAsync.

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
File content/public/browser/service_worker_context.cc (right):

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.cc:16: static
base::LazyInstance<HeaderNameSet> g_excluded_header_name_set =
On 2014/10/29 21:47:21, Lei Zhang wrote:
> you: you don't need static in an anonymous namespace.

Done.

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.cc:19: 
On 2014/10/29 21:19:58, sievers wrote:
> Do you think the static functions could move to the impl class?
> It's a bit discouraged by the content api guide to have .cc files for this.

ServiceWorkerContextWrapper which is the impl class of ServiceWorkerContext is
created on UI thread per StoragePartition.
It is difficult to get ServiceWorkerContextWrapper from where I want to call
AddExcludedHeadersForFetchEven on IO thread.

Shall I move this file to content/browser/ directory like browser_context.cc?

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.cc:23: BrowserThread::PostTask(
On 2014/10/29 21:19:58, sievers wrote:
> Could you let the caller handle that? Then you can leave the API like it is
i.e.
> IO-thread only, see header comment.

Done.

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.h:8: #include <set>
On 2014/10/29 21:19:58, sievers wrote:
> set is unused

Done.

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.h:20: // the IO thread except for
AddExcludedHeadersForFetchEvent.
On 2014/10/29 21:47:21, Lei Zhang wrote:
> With the code as is, do you mean to say "except for
> AddExcludedHeadersForFetchEvent() which can be called from any thread" ?

Yes but the sentence "Must be used from the IO thread." was removed by
https://codereview.chromium.org/653633004.
So I removed this change.

[Lei Zhang, 2014-10-30 08:05:17]

https://codereview.chromium.org/666973003/diff/100001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/666973003/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:275:
content::ServiceWorkerContext::AddExcludedHeadersForFetchEvent(
On 2014/10/30 07:54:34, horo wrote:
> On 2014/10/29 21:47:21, Lei Zhang wrote:
> > This looks like essentially a static function call. Why do we have to do it
> > every time a ChromeResourceDispatcherHostDelegate gets created?
> 
> I think ChromeResourceDispatcherHostDelegate is created only once.
> But it is better to call this on IO thread.
> So I moved this to IOThread::InitAsync.

In that case, please as a chrome/browser/io_thread.cc OWNER to review. They know
the code much better than me.

[horo, 2014-10-30 08:23:10]

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

[horo, 2014-10-30 08:23:11]

cbentzel@
Could you please review chrome/browser/io_thread.cc?
Thank you

[cbentzel, 2014-10-30 17:09:27]

On 2014/10/30 08:23:11, horo wrote:
> cbentzel@
> Could you please review chrome/browser/io_thread.cc?
> Thank you

Are there any tests?

[cbentzel, 2014-10-30 17:10:52]

On 2014/10/30 17:09:27, cbentzel wrote:
> On 2014/10/30 08:23:11, horo wrote:
> > cbentzel@
> > Could you please review chrome/browser/io_thread.cc?
> > Thank you
> 
> Are there any tests?

I don't necessarily need them on io_thread.cc, but it seems like there aren't
any system level ones or even ones done on ServiceWorkerUrlRequestJob

This seems like something that would be hard to detect regressions in manually
and we'd want automated testing in place.

[cbentzel, 2014-10-30 17:25:11]

On 2014/10/30 17:10:52, cbentzel wrote:
> On 2014/10/30 17:09:27, cbentzel wrote:
> > On 2014/10/30 08:23:11, horo wrote:
> > > cbentzel@
> > > Could you please review chrome/browser/io_thread.cc?
> > > Thank you
> > 
> > Are there any tests?
> 
> I don't necessarily need them on io_thread.cc, but it seems like there aren't
> any system level ones or even ones done on ServiceWorkerUrlRequestJob
> 
> This seems like something that would be hard to detect regressions in manually
> and we'd want automated testing in place.

Going back to overall review: It seems odd to dump this into io_thread.cc to be
honest. There's nothing wrong with the details of the code though that I can
see.

Could this be done as per-instance data in the ServiceWorkerContext and created
in constructor? It looks like all concrete classes of ServiceWorkerContext are
at the content layer, but if there were one at the chrome layer it would make
sense.

[no sievers, 2014-10-30 19:35:33]

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
File content/public/browser/service_worker_context.cc (right):

https://codereview.chromium.org/666973003/diff/100001/content/public/browser/...
content/public/browser/service_worker_context.cc:19: 
On 2014/10/30 07:54:34, horo wrote:
> On 2014/10/29 21:19:58, sievers wrote:
> > Do you think the static functions could move to the impl class?
> > It's a bit discouraged by the content api guide to have .cc files for this.
> 
> ServiceWorkerContextWrapper which is the impl class of ServiceWorkerContext is
> created on UI thread per StoragePartition.
> It is difficult to get ServiceWorkerContextWrapper from where I want to call
> AddExcludedHeadersForFetchEven on IO thread.
> 
> Shall I move this file to content/browser/ directory like browser_context.cc?

Oh I meant just literally moving the definition of the function (and leaving the
declaration as is if you need it to be public). I.e. define static
Interface::Foo() in interface_impl.cc. Also see other static functions in
content/public/browser where the same is done like
RenderWidgetHostView::FromID().

[michaeln, 2014-10-30 19:56:32]

On 2014/10/30 17:25:11, cbentzel wrote:
> On 2014/10/30 17:10:52, cbentzel wrote:
> > On 2014/10/30 17:09:27, cbentzel wrote:
> > > On 2014/10/30 08:23:11, horo wrote:
> > > > cbentzel@
> > > > Could you please review chrome/browser/io_thread.cc?
> > > > Thank you
> > > 
> > > Are there any tests?
> > 
> > I don't necessarily need them on io_thread.cc, but it seems like there
aren't
> > any system level ones or even ones done on ServiceWorkerUrlRequestJob
> > 
> > This seems like something that would be hard to detect regressions in
manually
> > and we'd want automated testing in place.
> 
> Going back to overall review: It seems odd to dump this into io_thread.cc to
be
> honest. There's nothing wrong with the details of the code though that I can
> see.

This is why i originally suggested it go into the
ChromeResourceDispatcherHostDelegate which has todo with resource dispatching
and interacts with the variations header. Since there is only one resource
dispatcher host, there is only one resource dispatcher host delegate. Notice the
static ResourceDispatcherHost::Get() method.

Having the caller invoke AddExcludedHeadersForFetchEvent on the IO thread is
nice, ChromeResourceDispatcherHostDelegate could call PostTask to accomplish
that.

> Could this be done as per-instance data in the ServiceWorkerContext and
created
> in constructor?

Since the ResourceDispatcherHost and VariationsHeaderProvider are singletons all
instances of ServiceWorkerContext will end up being configured the same way. A
global collection of values fits our use case for excluded headers in a
straighforward way.

> It looks like all concrete classes of ServiceWorkerContext are
> at the content layer, but if there were one at the chrome layer it would make
> sense.

Thats not consistent with how the other storage contexts classes are handled. I
think it's jam's intention that they're not derivable by the embedder since the
functions of the ServiceWorkerContext is provided to the embedder by the content
lib.

If this is to be done on a per-instance basis, then i think the content lib
would need a way to ask the embedder what headers to exclude, maybe a virtual
ContentBrowserClient::GetHeadersToExcludeFromFetchEvent() method. I'm not sure
that's really an improvement since the method is defined someplace not
particularly nearby to serviceworkers, although it would eliminate a static
linkage. However, there are a considerable number of static methods in the
content public api so I don't think the static linkage is a concern.

[horo, 2014-10-31 14:08:00]

Patchset #8 (id:180001) has been deleted

[horo, 2014-10-31 14:19:18]

I reverted io_thread.cc and devtools_network_transaction_factory.cc and moved
ServiceWorkerContext method definition to service_worker_context_wrapper.cc.
How about patch set 8?
I'm trying to create a browsertest for this. But I couldn't find the way to
enable UMA in the browsertests.

> Oh I meant just literally moving the definition of the function (and leaving
the
> declaration as is if you need it to be public). I.e. define static
> Interface::Foo() in interface_impl.cc. Also see other static functions in
> content/public/browser where the same is done like
> RenderWidgetHostView::FromID().

Done. Thank you.


> Having the caller invoke AddExcludedHeadersForFetchEvent on the IO thread is
> nice, ChromeResourceDispatcherHostDelegate could call PostTask to accomplish
> that.

Done

[Lei Zhang, 2014-10-31 18:10:20]

Given michaeln's comments, chrome_resource_dispatcher_host_delegate.cc lgtm

[no sievers, 2014-10-31 18:41:25]

content lgtm

[cbentzel, 2014-10-31 19:55:48]

On 2014/10/31 18:41:25, sievers wrote:
> content lgtm

Thanks for making the io_thread change. Non-vote from me, but looks like the
right approach.

[cbentzel, 2014-10-31 20:00:15]

https://codereview.chromium.org/666973003/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/666973003/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:41: const
std::vector<std::string>& header_names) {
Should this argument be a set rather than a vector given that this is ultimately
stored as a set and neither duplicates nor ordering really make sense?

Same for a few other locations in this changelist.

It's not a huge issue, so only do it if you think it's cleaner.

[horo, 2014-11-01 00:23:39]

Thank you for your review!

https://codereview.chromium.org/666973003/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/666973003/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:41: const
std::vector<std::string>& header_names) {
On 2014/10/31 20:00:15, cbentzel wrote:
> Should this argument be a set rather than a vector given that this is
ultimately
> stored as a set and neither duplicates nor ordering really make sense?
> 
> Same for a few other locations in this changelist.
> 
> It's not a huge issue, so only do it if you think it's cleaner.

Done.

[horo, 2014-11-01 01:22:36]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-11-01 01:23:29]

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

[commit-bot: I haz the power, 2014-11-01 02:08:36]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2014-11-01 02:09:15]

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