Delay generation of User-Agent header to URLRequestHttpJob and accept custom User-Agent from XHR/Fe…

Delay generation of User-Agent header to URLRequestHttpJob and accept custom User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

Chromium has been violating this requirement. Renderer generates a
User-Agent header and it's visible to the ServiceWorker which controls
the page.

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37 because
a User-Agent header appended by the controlled page is considered to be
a author provided header by fetch() in a ServiceWorker and may cause
unnecessary wrong preflight.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

The logic in this CL doesn't preserve the default User-Agent value
overridden by the Inspector for the controlled page if it's proxied by
a ServiceWorker. But this is not working even before this CL. "request"
guard and "request-no-cors" guard rejects User-Agent value added by
the controlled page on fetch() call in a ServiceWorker. See
http://crbug.com/638552.


Note about layouttest changes:

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.

print-headers.cgi should be merged with echo-headers.php. This is not
done in this CL not to increase the patch size any more. See
http://crbug.com/638553.

BUG=595993, 571722, 638552, 638553

[tyoshino (SeeGerritForStatus), 2016-08-17 08:22:26]

Description was changed from

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37.

This change fixes both of these issues.

BUG=595993,571722
==========

to

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

BUG=595993,571722
==========

[tyoshino (SeeGerritForStatus), 2016-08-17 08:51:17]

Description was changed from

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

BUG=595993,571722
==========

to

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.


BUG=595993,571722
==========

[tyoshino (SeeGerritForStatus), 2016-08-17 09:25:28]

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

[commit-bot: I haz the power, 2016-08-17 09:25:43]

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

[tyoshino (SeeGerritForStatus), 2016-08-17 09:34:35]

Description was changed from

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.


BUG=595993,571722
==========

to

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

Chromium has been violating this requirement. Renderer generates a
User-Agent header and it's visible to the ServiceWorker which controls
the page.

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37 because
a User-Agent header appended by the controlled page is considered to be
a author provided header by fetch() in a ServiceWorker and may cause
unnecessary wrong preflight.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

The logic in this patch doesn't preserve ZZ


Note about layouttest changes:

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.

print-headers.cgi should be merged with echo-headers.php. This is not
done in this CL not to increase the patch size any more.

BUG=595993,571722
==========

[commit-bot: I haz the power, 2016-08-17 10:29:00]

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

[commit-bot: I haz the power, 2016-08-17 10:29:01]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tyoshino (SeeGerritForStatus), 2016-08-17 11:13:35]

Description was changed from

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

Chromium has been violating this requirement. Renderer generates a
User-Agent header and it's visible to the ServiceWorker which controls
the page.

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37 because
a User-Agent header appended by the controlled page is considered to be
a author provided header by fetch() in a ServiceWorker and may cause
unnecessary wrong preflight.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

The logic in this patch doesn't preserve ZZ


Note about layouttest changes:

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.

print-headers.cgi should be merged with echo-headers.php. This is not
done in this CL not to increase the patch size any more.

BUG=595993,571722
==========

to

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

Chromium has been violating this requirement. Renderer generates a
User-Agent header and it's visible to the ServiceWorker which controls
the page.

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37 because
a User-Agent header appended by the controlled page is considered to be
a author provided header by fetch() in a ServiceWorker and may cause
unnecessary wrong preflight.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

The logic in this CL doesn't preserve the default User-Agent value
overridden by the Inspector for the controlled page if it's proxied by
a ServiceWorker. But this is not working even before this CL. "request"
guard and "request-no-cors" guard rejects User-Agent value added by
the controlled page on fetch() call in a ServiceWorker.


Note about layouttest changes:

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.

print-headers.cgi should be merged with echo-headers.php. This is not
done in this CL not to increase the patch size any more.

BUG=595993,571722
==========

[tyoshino (SeeGerritForStatus), 2016-08-17 11:18:03]

Description was changed from

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

Chromium has been violating this requirement. Renderer generates a
User-Agent header and it's visible to the ServiceWorker which controls
the page.

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37 because
a User-Agent header appended by the controlled page is considered to be
a author provided header by fetch() in a ServiceWorker and may cause
unnecessary wrong preflight.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

The logic in this CL doesn't preserve the default User-Agent value
overridden by the Inspector for the controlled page if it's proxied by
a ServiceWorker. But this is not working even before this CL. "request"
guard and "request-no-cors" guard rejects User-Agent value added by
the controlled page on fetch() call in a ServiceWorker.


Note about layouttest changes:

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.

print-headers.cgi should be merged with echo-headers.php. This is not
done in this CL not to increase the patch size any more.

BUG=595993,571722
==========

to

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

Chromium has been violating this requirement. Renderer generates a
User-Agent header and it's visible to the ServiceWorker which controls
the page.

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37 because
a User-Agent header appended by the controlled page is considered to be
a author provided header by fetch() in a ServiceWorker and may cause
unnecessary wrong preflight.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

The logic in this CL doesn't preserve the default User-Agent value
overridden by the Inspector for the controlled page if it's proxied by
a ServiceWorker. But this is not working even before this CL. "request"
guard and "request-no-cors" guard rejects User-Agent value added by
the controlled page on fetch() call in a ServiceWorker. See
http://crbug.com/638552.


Note about layouttest changes:

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.

print-headers.cgi should be merged with echo-headers.php. This is not
done in this CL not to increase the patch size any more. See
http://crbug.com/638553.

BUG=595993,571722,638552,638553
==========

[tyoshino (SeeGerritForStatus), 2016-08-18 06:47:59]

Description was changed from

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

Chromium has been violating this requirement. Renderer generates a
User-Agent header and it's visible to the ServiceWorker which controls
the page.

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37 because
a User-Agent header appended by the controlled page is considered to be
a author provided header by fetch() in a ServiceWorker and may cause
unnecessary wrong preflight.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

The logic in this CL doesn't preserve the default User-Agent value
overridden by the Inspector for the controlled page if it's proxied by
a ServiceWorker. But this is not working even before this CL. "request"
guard and "request-no-cors" guard rejects User-Agent value added by
the controlled page on fetch() call in a ServiceWorker. See
http://crbug.com/638552.


Note about layouttest changes:

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.

print-headers.cgi should be merged with echo-headers.php. This is not
done in this CL not to increase the patch size any more. See
http://crbug.com/638553.

BUG=595993,571722,638552,638553
==========

to

==========
Delay generation of User-Agent header to URLRequestHttpJob and accept custom
User-Agent from XHR/Fetch

To conform to the Fetch Standard, User-Agent header generation should be
done after consulting a ServiceWorker (see that the step is in
HTTP-network-or-cache fetch which is performed after ServiceWorker).

Chromium has been violating this requirement. Renderer generates a
User-Agent header and it's visible to the ServiceWorker which controls
the page.

This spec violation also prevented allowing for custom User-Agent header
which was enabled by https://github.com/whatwg/fetch/issues/37 because
a User-Agent header appended by the controlled page is considered to be
a author provided header by fetch() in a ServiceWorker and may cause
unnecessary wrong preflight.

This change fixes both of these issues while preserving the inspector's
functionality to override User-Agent, by introducing a new path in the
resource loader to pass the User-Agent string generated and added by
FrameFetchContext to net::URLRequest instead of embedding it in the
list of headers immediately in FrameFetchContext.

The logic in this CL doesn't preserve the default User-Agent value
overridden by the Inspector for the controlled page if it's proxied by
a ServiceWorker. But this is not working even before this CL. "request"
guard and "request-no-cors" guard rejects User-Agent value added by
the controlled page on fetch() call in a ServiceWorker. See
http://crbug.com/638552.


Note about layouttest changes:

User-Agent and Authorization are removed from
http/tests/xmlhttprequest/set-dangerous-headers.html as Authorization
is tested by authorization-header.html and User-Agent will be tested by
setrequestheader-useragent.html introduced by this CL.

print-headers.cgi should be merged with echo-headers.php. This is not
done in this CL not to increase the patch size any more. See
http://crbug.com/638553.

BUG=595993,571722,638552,638553
==========

[tyoshino (SeeGerritForStatus), 2016-08-18 06:47:59]

tyoshino@chromium.org changed reviewers:
+ horo@chromium.org, japhet@chromium.org

[Nate Chapin, 2016-08-19 21:29:01]

blink LGTM

[horo, 2016-08-21 08:39:37]

Do we have layouttests that check the behavior of setting custom User-Agent in a
page and:
1) The ServiceWorker doesn't call event.respondWith().
2) The ServiceWorker calls event.respondWith(event.fetch(request)).

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
net/url_request/url_request.h:770: bool has_default_user_agent_;
You can use base::Optional<std::string>.

https://codereview.chromium.org/2254693002/diff/50002/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/2254693002/diff/50002/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ResourceRequest.h:135: const
AtomicString& defaultHTTPUserAgent() const { return m_defaultHTTPUserAgent; }
please update ResourceRequestTest.cpp.

[mmenke, 2016-08-21 14:37:39]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2016-08-21 14:37:40]

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
net/url_request/url_request.h:770: bool has_default_user_agent_;
On 2016/08/21 08:39:37, horo wrote:
> You can use base::Optional<std::string>.

Given that URLRequest uses empty strings to denote arguments it should ignore,
I'd say just stick with an empty string, for consistency.

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent,
request_->default_user_agent());
I really don't think we need two different wants to set the default user agent,
and I'd rather we shrink the URLRequestJob / URLRequest dependencies, rather
than expand them.  Note, for instance, that we're not setting this one (Or the
other one!) in URLRequestFtpJob when going through an HTTP proxy, which seems
like a bug.

Is the issue that devtools also sets it, and you want a way to sneak it through
the ServiceWorker URLRequestJob subclass without actually setting the header?

[tyoshino (SeeGerritForStatus), 2016-08-23 09:42:49]

On 2016/08/21 08:39:37, horo wrote:
> Do we have layouttests that check the behavior of setting custom User-Agent in
a
> page and:
> 1) The ServiceWorker doesn't call event.respondWith().
> 2) The ServiceWorker calls event.respondWith(event.fetch(request)).

Added. For (1), is a test using the empty worker sufficient?

[tyoshino (SeeGerritForStatus), 2016-08-23 09:42:58]

https://codereview.chromium.org/2254693002/diff/50002/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/2254693002/diff/50002/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ResourceRequest.h:135: const
AtomicString& defaultHTTPUserAgent() const { return m_defaultHTTPUserAgent; }
On 2016/08/21 08:39:37, horo wrote:
> please update ResourceRequestTest.cpp.

Thanks for catching that.

Updated CrossThreadResourceRequestData test

[tyoshino (SeeGerritForStatus), 2016-08-23 10:01:47]

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent,
request_->default_user_agent());
On 2016/08/21 14:37:39, mmenke wrote:
> I really don't think we need two different wants to set the default user
agent,
> and I'd rather we shrink the URLRequestJob / URLRequest dependencies, rather
> than expand them.  Note, for instance, that we're not setting this one (Or the
> other one!) in URLRequestFtpJob when going through an HTTP proxy, which seems
> like a bug.
> 
> Is the issue that devtools also sets it, and you want a way to sneak it
through
> the ServiceWorker URLRequestJob subclass without actually setting the header?

Right. I'd like to propagate DevTools' User-Agent value override to net/ stack
but outside of the headers holding members in the request holding classes, and
have the header generated in URLRequestHttpJob (or yeah, Ftp job may, too).

The override is per request info. So, URLRequestContext is not right place to
handle it. Patching URLRequestContext for each request also doesn't sound the
right thing to do.

So, I've expanded it into the URLRequest as a member variable as well as other
per-request info such as method_, referrer_, ... and let URLRequestHttpJob to
pick it.

[mmenke, 2016-08-23 14:54:33]

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent,
request_->default_user_agent());
On 2016/08/23 10:01:47, tyoshino wrote:
> On 2016/08/21 14:37:39, mmenke wrote:
> > I really don't think we need two different wants to set the default user
> agent,
> > and I'd rather we shrink the URLRequestJob / URLRequest dependencies, rather
> > than expand them.  Note, for instance, that we're not setting this one (Or
the
> > other one!) in URLRequestFtpJob when going through an HTTP proxy, which
seems
> > like a bug.
> > 
> > Is the issue that devtools also sets it, and you want a way to sneak it
> through
> > the ServiceWorker URLRequestJob subclass without actually setting the
header?
> 
> Right. I'd like to propagate DevTools' User-Agent value override to net/ stack
> but outside of the headers holding members in the request holding classes, and
> have the header generated in URLRequestHttpJob (or yeah, Ftp job may, too).
> 
> The override is per request info. So, URLRequestContext is not right place to
> handle it. Patching URLRequestContext for each request also doesn't sound the
> right thing to do.
> 
> So, I've expanded it into the URLRequest as a member variable as well as other
> per-request info such as method_, referrer_, ... and let URLRequestHttpJob to
> pick it.

Could ServiceWorker just hide the header it passes to the Javascript API, and
then re-set it itself?  Fetch API doesn't allow setting of this header, so
there's no case in which anything else is allowed to set it.  I'm reluctant to
expand URLRequest's surface area here, and adding yet another complex and
unexpected behavior.

[mmenke, 2016-08-23 15:25:22]

On 2016/08/23 14:54:33, mmenke wrote:
>
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
> File net/url_request/url_request_http_job.cc (right):
> 
>
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
> net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent,
> request_->default_user_agent());
> On 2016/08/23 10:01:47, tyoshino wrote:
> > On 2016/08/21 14:37:39, mmenke wrote:
> > > I really don't think we need two different wants to set the default user
> > agent,
> > > and I'd rather we shrink the URLRequestJob / URLRequest dependencies,
rather
> > > than expand them.  Note, for instance, that we're not setting this one (Or
> the
> > > other one!) in URLRequestFtpJob when going through an HTTP proxy, which
> seems
> > > like a bug.
> > > 
> > > Is the issue that devtools also sets it, and you want a way to sneak it
> > through
> > > the ServiceWorker URLRequestJob subclass without actually setting the
> header?
> > 
> > Right. I'd like to propagate DevTools' User-Agent value override to net/
stack
> > but outside of the headers holding members in the request holding classes,
and
> > have the header generated in URLRequestHttpJob (or yeah, Ftp job may, too).
> > 
> > The override is per request info. So, URLRequestContext is not right place
to
> > handle it. Patching URLRequestContext for each request also doesn't sound
the
> > right thing to do.
> > 
> > So, I've expanded it into the URLRequest as a member variable as well as
other
> > per-request info such as method_, referrer_, ... and let URLRequestHttpJob
to
> > pick it.
> 
> Could ServiceWorker just hide the header it passes to the Javascript API, and
> then re-set it itself?  Fetch API doesn't allow setting of this header, so
> there's no case in which anything else is allowed to set it.  I'm reluctant to
> expand URLRequest's surface area here, and adding yet another complex and
> unexpected behavior.

Also, shouldn't this same thing be done for the Accept-Language header?  Not
sure if there are others.

[horo, 2016-08-24 01:38:33]

> Added. For (1), is a test using the empty worker sufficient?

Yes.

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fetch/resources/useragent-header-iframe.html
(right):

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/resources/useragent-header-iframe.html:5:
.then(response => {
you don't need brackets and "return".
.then(response => response.text())

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/resources/useragent-header-iframe.html:8:
.then(body => {
ditto
.then(body => parse_response(body))

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/resources/useragent-header-iframe.html:15:
.then(response => {
ditto
.then(response => response.text())

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/resources/useragent-header-iframe.html:18:
.then(body => {
ditto

.then(body => parse_response(body))

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-empty-worker.html
(right):

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-empty-worker.html:12:
return service_worker_unregister_and_register(test, SCRIPT, SCOPE)
You can't register the SCRIPT to the SCOPE.
SCOPE must be under the directory where the script exists.


FAIL Check User-Agent header generation for requests proxied by the empty worker
assert_unreached: unregister and register should not fail: Failed to register a
ServiceWorker: The path of the provided scope
('/fetch/resources/useragent-header-iframe.html') is not under the max scope
allowed ('/serviceworker/resources/'). Adjust the scope, move the Service Worker
script, or use the Service-Worker-Allowed HTTP header to allow the scope.
Reached unreachable code

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-empty-worker.html:13:
.then(r => {
ditto

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-empty-worker.html:16:
.then(() => {
ditto

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-empty-worker.html:22:
.then(() => {
ditto

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-empty-worker.html:28:
.then(() => {
ditto

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-forwarding-worker.html
(right):

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-forwarding-worker.html:8:
const SCRIPT = '../resources/forwarding-worker.js';
missing this file?

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/serviceworker-proxied/useragent-header-via-forwarding-worker.html:38:
.then(() => {
ditto

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fetch/window/useragent-header.html
(right):

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/window/useragent-header.html:8:
.then(response => {
ditto

https://codereview.chromium.org/2254693002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/window/useragent-header.html:18:
.then(response => {
ditto

[tyoshino (SeeGerritForStatus), 2016-08-25 04:11:34]

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent,
request_->default_user_agent());
On 2016/08/23 14:54:33, mmenke (busy) wrote:
> Could ServiceWorker just hide the header it passes to the Javascript API, and
> then re-set it itself?  Fetch API doesn't allow setting of this header, so
> there's no case in which anything else is allowed to set it.  I'm reluctant to
> expand URLRequest's surface area here, and adding yet another complex and
> unexpected behavior.

I think embedding everything into the header holder is also source of complexity
and bugs. But I understand your sentiment. I'll check if there's any way to make
both happy.

[mmenke, 2016-08-25 04:17:02]

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent,
request_->default_user_agent());
On 2016/08/25 04:11:34, tyoshino wrote:
> On 2016/08/23 14:54:33, mmenke (busy) wrote:
> > Could ServiceWorker just hide the header it passes to the Javascript API,
and
> > then re-set it itself?  Fetch API doesn't allow setting of this header, so
> > there's no case in which anything else is allowed to set it.  I'm reluctant
to
> > expand URLRequest's surface area here, and adding yet another complex and
> > unexpected behavior.
> 
> I think embedding everything into the header holder is also source of
complexity
> and bugs. But I understand your sentiment. I'll check if there's any way to
make
> both happy.

SGTM.  I think it's also worth noting that the behavior
URLRequest::set_default_user_agent can't really be documented, without talking
about how it's different from setting the header directly, and ServiceWorker
(Which is not a net/ layer concept)

[mmenke, 2017-06-12 15:12:20]

On 2016/08/25 04:17:02, mmenke wrote:
>
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
> File net/url_request/url_request_http_job.cc (right):
> 
>
https://codereview.chromium.org/2254693002/diff/50002/net/url_request/url_req...
> net/url_request/url_request_http_job.cc:392: HttpRequestHeaders::kUserAgent,
> request_->default_user_agent());
> On 2016/08/25 04:11:34, tyoshino wrote:
> > On 2016/08/23 14:54:33, mmenke (busy) wrote:
> > > Could ServiceWorker just hide the header it passes to the Javascript API,
> and
> > > then re-set it itself?  Fetch API doesn't allow setting of this header, so
> > > there's no case in which anything else is allowed to set it.  I'm
reluctant
> to
> > > expand URLRequest's surface area here, and adding yet another complex and
> > > unexpected behavior.
> > 
> > I think embedding everything into the header holder is also source of
> complexity
> > and bugs. But I understand your sentiment. I'll check if there's any way to
> make
> > both happy.
> 
> SGTM.  I think it's also worth noting that the behavior
> URLRequest::set_default_user_agent can't really be documented, without talking
> about how it's different from setting the header directly, and ServiceWorker
> (Which is not a net/ layer concept)

Removing myself from this CL.  Feel free to add me back if you pick it up again.

[mmenke, 2017-06-12 15:12:28]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org