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

net/url_request/url_request_http_job.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/url_request/url_request_http_job.h"
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 369 matching lines @@
 
   // Our consumer should have made sure that this is a safe referrer. See for
   // instance WebCore::FrameLoader::HideReferrer.
   if (referrer.is_valid()) {
     request_info_.extra_headers.SetHeader(HttpRequestHeaders::kReferer,
                                           referrer.spec());
   }
 
   request_info_.token_binding_referrer = request_->token_binding_referrer();
 
+  if (request_->has_default_user_agent()) {
+    request_info_.extra_headers.SetHeaderIfMissing(
+        HttpRequestHeaders::kUserAgent, request_->default_user_agent());

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

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 10:01:47]

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]

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.

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

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:01]

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)

+  }
+
   request_info_.extra_headers.SetHeaderIfMissing(
       HttpRequestHeaders::kUserAgent,
       http_user_agent_settings_ ?
           http_user_agent_settings_->GetUserAgent() : std::string());
 
   AddExtraHeaders();
   AddCookieHeaderAndStart();
 }
 
 void URLRequestHttpJob::Kill() {
@@ skipping 1208 matching lines @@
   // Notify NetworkQualityEstimator.
   if (request()) {
     NetworkQualityEstimator* network_quality_estimator =
         request()->context()->network_quality_estimator();
     if (network_quality_estimator)
       network_quality_estimator->NotifyURLRequestDestroyed(*request());
   }
 }
 
 }  // namespace net