Add a setting to CF to remove 'chromeframe' from the UserAgent on a per-pattern basis.

chrome_frame/http_negotiate.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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 "chrome_frame/http_negotiate.h"
 
 #include <atlbase.h>
 #include <atlcom.h>
 #include <htiframe.h>
 
 #include "base/logging.h"
@@ skipping 64 matching lines @@
   STDMETHOD(OnDataAvailable)(DWORD flags, DWORD size, FORMATETC* formatetc,
     STGMEDIUM* storage) {
     return E_NOTIMPL;
   }
   STDMETHOD(OnObjectAvailable)(REFIID iid, IUnknown* object) {
     return E_NOTIMPL;
   }
 };
 }  // end namespace
 
-std::string AppendCFUserAgentString(LPCWSTR headers,
-                                    LPCWSTR additional_headers) {
+
+std::string ExtractUserAgentFromHeaders(LPCWSTR headers,

[grt (UTC plus 2), 2012/03/20 17:27:45]

the name of this method doesn't make its behavior obvious.  does "extract" mean
"remove from" or just "get"?  would you add a comment?  also, it appears to only
consider |headers| if |additional_headers| doesn't contain a User-Agent field. 
seems that that should be mentioned in a comment, too.

[robertshield, 2012/03/26 02:43:33]

Done.

+                                        LPCWSTR additional_headers) {
   using net::HttpUtil;
 
   std::string ascii_headers;
   if (additional_headers) {
     ascii_headers = WideToASCII(additional_headers);
   }
 
   // Extract "User-Agent" from |additional_headers| or |headers|.
   HttpUtil::HeadersIterator headers_iterator(ascii_headers.begin(),
                                              ascii_headers.end(), "\r\n");
   std::string user_agent_value;
   if (headers_iterator.AdvanceTo(kLowerCaseUserAgent)) {
     user_agent_value = headers_iterator.values();
   } else if (headers != NULL) {
     // See if there's a user-agent header specified in the original headers.
     std::string original_headers(WideToASCII(headers));
     HttpUtil::HeadersIterator original_it(original_headers.begin(),
         original_headers.end(), "\r\n");
     if (original_it.AdvanceTo(kLowerCaseUserAgent))
       user_agent_value = original_it.values();
   }
 
+  return user_agent_value;
+}
+
+std::string FilterHeaders(const std::string& old_headers,

[grt (UTC plus 2), 2012/03/20 17:27:45]

i find "filter" a bit vague.  how about ExcludeFieldFromHeaders(const
std::string& headers, const char* field)
also, is it required that |field| be lower-case?  please document if so.

[robertshield, 2012/03/26 02:43:33]

Done.

+                          const char* exclude) {
+  using net::HttpUtil;
+  std::string new_headers;
+  HttpUtil::HeadersIterator headers_iterator(old_headers.begin(),
+                                             old_headers.end(), "\r\n");
+  while (headers_iterator.GetNext()) {
+    std::string name(headers_iterator.name());
+    if (!LowerCaseEqualsASCII(name, exclude)) {
+      new_headers += name + ": " + headers_iterator.values() + "\r\n";

[grt (UTC plus 2), 2012/03/20 17:27:45]

these three uses of operator+ involve lots of temporaries.  please use += or
append as you see fit

[robertshield, 2012/03/26 02:43:33]

Done.

+    }
+  }
+
+  return new_headers;
+}
+
+std::string AppendCFUserAgentString(LPCWSTR headers,
+                                    LPCWSTR additional_headers) {
+  std::string user_agent_value(ExtractUserAgentFromHeaders(headers,
+                                                           additional_headers));
+
   // Use the default "User-Agent" if none was provided.
   if (user_agent_value.empty())
     user_agent_value = http_utils::GetDefaultUserAgent();
 
   // Now add chromeframe to it.
-  user_agent_value = http_utils::AddChromeFrameToUserAgentValue(
-      user_agent_value);
+  user_agent_value =
+      http_utils::AddChromeFrameToUserAgentValue(user_agent_value);
 
-  // Build new headers, skip the existing user agent value from
-  // existing headers.
+  // Build a new set of additional headers, skipping the existing user agent
+  // value if present.
   std::string new_headers;
-  headers_iterator.Reset();
-  while (headers_iterator.GetNext()) {
-    std::string name(headers_iterator.name());
-    if (!LowerCaseEqualsASCII(name, kLowerCaseUserAgent)) {
-      new_headers += name + ": " + headers_iterator.values() + "\r\n";
-    }
+  if (additional_headers) {
+    std::string ascii_headers = WideToASCII(additional_headers);

[grt (UTC plus 2), 2012/03/20 17:27:45]

nit: use () rather than =

[robertshield, 2012/03/26 02:43:33]

Done.

+    new_headers = FilterHeaders(ascii_headers, kLowerCaseUserAgent);
   }
 
   new_headers += "User-Agent: " + user_agent_value;

[grt (UTC plus 2), 2012/03/20 17:27:45]

i'd advocate for .append or += three times here, too

[robertshield, 2012/03/26 02:43:33]

Done.

   new_headers += "\r\n";
   return new_headers;
 }
 
+// Unconditionally adds the specified |user_agent_value| to the given set of
+// |headers|.
 std::string ReplaceOrAddUserAgent(LPCWSTR headers,
                                   const std::string& user_agent_value) {
-  using net::HttpUtil;
-
   std::string new_headers;
   if (headers) {
     std::string ascii_headers(WideToASCII(headers));
-
-    // Extract "User-Agent" from the headers.
-    HttpUtil::HeadersIterator headers_iterator(ascii_headers.begin(),
-                                               ascii_headers.end(), "\r\n");
-
     // Build new headers, skip the existing user agent value from
     // existing headers.
-    while (headers_iterator.GetNext()) {
-      std::string name(headers_iterator.name());
-      if (!LowerCaseEqualsASCII(name, kLowerCaseUserAgent)) {
-        new_headers += name + ": " + headers_iterator.values() + "\r\n";
-      }
-    }
+    new_headers = FilterHeaders(ascii_headers, kLowerCaseUserAgent);
   }
   new_headers += "User-Agent: " + user_agent_value;

[grt (UTC plus 2), 2012/03/20 17:27:45]

use += instead of +

[robertshield, 2012/03/26 02:43:33]

Done.

   new_headers += "\r\n";
   return new_headers;
 }
 
+// Removes CF from any user agent header found in |headers| or
+// |additional_headers| and

[grt (UTC plus 2), 2012/03/20 17:27:45]

and...?

[robertshield, 2012/03/26 02:43:33]

... and this is where I fell asleep :-)

+std::string RemoveCFUserAgentString(LPCWSTR headers,
+                                    LPCWSTR additional_headers) {
+  std::string user_agent_value(ExtractUserAgentFromHeaders(headers,
+                                                           additional_headers));
+
+  // Use the default "User-Agent" if none was provided.
+  if (user_agent_value.empty())
+    user_agent_value = http_utils::GetDefaultUserAgent();
+
+  // Now remove chromeframe from it.
+  user_agent_value =
+      http_utils::RemoveChromeFrameFromUserAgentValue(user_agent_value);
+
+  // Build a new set of additional headers, skipping the existing user agent
+  // value if present.
+  std::string new_headers;
+  if (additional_headers) {
+    std::string ascii_headers = WideToASCII(additional_headers);
+    new_headers = FilterHeaders(ascii_headers, kLowerCaseUserAgent);
+  }
+
+  new_headers += "User-Agent: " + user_agent_value;

[grt (UTC plus 2), 2012/03/20 17:27:45]

use += rather than +

[robertshield, 2012/03/26 02:43:33]

Done.

+  new_headers += "\r\n";
+  return new_headers;
+}
+
 HttpNegotiatePatch::HttpNegotiatePatch() {
 }
 
 HttpNegotiatePatch::~HttpNegotiatePatch() {
 }
 
 // static
 bool HttpNegotiatePatch::Initialize() {
   if (IS_PATCHED(IHttpNegotiate)) {
     DLOG(WARNING) << __FUNCTION__ << " called more than once.";
@@ skipping 41 matching lines @@
     DLOG_IF(ERROR, FAILED(hr))
         << base::StringPrintf("HttpNegotiate patch failed 0x%08X", hr);
   } else {
     DLOG(WARNING)
         << base::StringPrintf("IHttpNegotiate not supported 0x%08X", hr);
   }
   return hr;
 }
 
 // static
 HRESULT HttpNegotiatePatch::BeginningTransaction(

[grt (UTC plus 2), 2012/03/20 17:27:45]

is it possible to make a test for the changes of behavior in this function?

[robertshield, 2012/03/26 02:43:33]

It is! Test added.

     IHttpNegotiate_BeginningTransaction_Fn original, IHttpNegotiate* me,
     LPCWSTR url, LPCWSTR headers, DWORD reserved, LPWSTR* additional_headers) {
   DVLOG(1) << __FUNCTION__ << " " << url << " headers:\n" << headers;
 
   HRESULT hr = original(me, url, headers, reserved, additional_headers);
 
   if (FAILED(hr)) {
     DLOG(WARNING) << __FUNCTION__ << " Delegate returned an error";
     return hr;
   }
   if (modify_user_agent_) {
     std::string updated_headers;
+
     if (IsGcfDefaultRenderer() &&
         RendererTypeForUrl(url) == RENDERER_TYPE_CHROME_DEFAULT_RENDERER) {
       // Replace the user-agent header with Chrome's.
       updated_headers = ReplaceOrAddUserAgent(*additional_headers,
                                               http_utils::GetChromeUserAgent());
+    } else if (ShouldRemoveUAForUrl(url) == USER_AGENT_REMOVE) {
+      updated_headers = RemoveCFUserAgentString(headers, *additional_headers);
     } else {
       updated_headers = AppendCFUserAgentString(headers, *additional_headers);
     }
+
     *additional_headers = reinterpret_cast<wchar_t*>(::CoTaskMemRealloc(
         *additional_headers,
         (updated_headers.length() + 1) * sizeof(wchar_t)));
     lstrcpyW(*additional_headers, ASCIIToWide(updated_headers).c_str());

[grt (UTC plus 2), 2012/03/20 17:27:45]

the various UserAgentString functions appear to convert wide to ascii, and this
converts the ascii back to wide.  is there a reason to not do all manipulations
on wide strings to avoid the pair of conversions?

[robertshield, 2012/03/26 02:43:33]

The functions in this file use UA code from \net (specifically the
HeadersIterator) which is ascii only.

   } else {
     // TODO(erikwright): Remove the user agent if it is present (i.e., because
     // of PostPlatform setting in the registry).
   }
   return S_OK;
 }