Remove webkit_glue::BuildUserAgent(), remove windows spoofing

webkit/glue/webkit_glue.cc

 // Copyright (c) 2011 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 "webkit/glue/webkit_glue.h"
 
 #if defined(OS_WIN)
 #include <objidl.h>
 #include <mlang.h>
 #elif defined(OS_POSIX) && !defined(OS_MACOSX)
@@ skipping 325 matching lines @@
   }
 }
 
 namespace {
 
 class UserAgentState {
  public:
   UserAgentState();
   ~UserAgentState();
 
-  void Set(const std::string& user_agent);
+  void Set(const std::string& user_agent, bool overriding);
   const std::string& Get(const GURL& url) const;
 
  private:
   mutable std::string user_agent_;
-  // The UA string when we're pretending to be Windows Chrome.
-  mutable std::string mimic_windows_user_agent_;
   // The UA string when we're pretending to be Mac Safari.
   mutable std::string mimic_mac_safari_user_agent_;
 
   mutable bool user_agent_requested_;
   bool user_agent_is_overridden_;
+  bool user_agent_is_set_;

[Evan Martin, 2011/09/21 20:27:24]

Is it possible to remove this and instead use user_agent_.empty() in the tests
below?  (Along with disallowing setting an empty user agent.)

Not a suggestion, it's a question I'm asking you.  :)

[Dirk Pranke, 2011/09/21 21:02:35]

Yes, I think this is possible; it wasn't at some point during the work on this
patch because some clients had empty user agents, but I think that's no longer
true. I will attempt to make it so.

 
   // This object can be accessed from multiple threads, so use a lock around
   // accesses to the data members.
   mutable base::Lock lock_;
 };
 
 UserAgentState::UserAgentState()
     : user_agent_requested_(false),
-      user_agent_is_overridden_(false) {
+      user_agent_is_overridden_(false),
+      user_agent_is_set_(false) {
 }
 
 UserAgentState::~UserAgentState() {
 }
 
-void UserAgentState::Set(const std::string& user_agent) {
+void UserAgentState::Set(const std::string& user_agent, bool overriding) {
   base::AutoLock auto_lock(lock_);
+  if (user_agent == user_agent_) {
+    // We allow the user agent to be set multiple times as long as it
+    // is set to the same value, in order to simplify unit testing
+    // given g_user_agent is a global.
+    return;
+  }
   DCHECK(!user_agent_requested_) << "Setting the user agent after someone has "
       "already requested it can result in unexpected behavior.";
-  user_agent_is_overridden_ = true;
+  user_agent_is_set_ = true;
+  user_agent_is_overridden_ = overriding;
   user_agent_ = user_agent;
 }
 
 const std::string& UserAgentState::Get(const GURL& url) const {
   base::AutoLock auto_lock(lock_);
   user_agent_requested_ = true;
 
-  if (user_agent_.empty())
-    user_agent_ = BuildUserAgent(false);
+  DCHECK(user_agent_is_set_);
 
   // Workarounds for sites that use misguided UA sniffing.
   if (!user_agent_is_overridden_) {
-#if defined(OS_POSIX) && !defined(OS_MACOSX)
-    if (MatchPattern(url.host(), "*.mail.yahoo.com")) {
-      // mail.yahoo.com is ok with Windows Chrome but not Linux Chrome.
-      // http://bugs.chromium.org/11136
-      // TODO(evanm): remove this if Yahoo fixes their sniffing.
-      if (mimic_windows_user_agent_.empty())
-        mimic_windows_user_agent_ = BuildUserAgent(true);
-      return mimic_windows_user_agent_;
-    }
-#endif
 #if defined(OS_MACOSX)
     if (url.host() == "www.microsoft.com" &&
         StartsWithASCII(url.path(), "/getsilverlight", false)) {
       // The landing page for updating Silverlight gives a confusing experience
       // in browsers that Silverlight doesn't officially support; spoof as
       // Safari to reduce the chance that users won't complete updates.
       // Should be removed if the sniffing is removed: http://crbug.com/88211

[jam, 2011/09/21 21:27:36]

this whole overriding thing seems like a lot of complexity just to support
testing this one block of code, on mac, when a user specifies a user agent flag.
why don't we just get rid of it?

[Dirk Pranke, 2011/09/21 22:19:59]

It's a good question; I'm not sure what the impact would be, but probably almost
minimal. I will follow up on this in a separate bug if that's okay.

[jam, 2011/09/21 22:26:29]

given that it adds some complexity to ContentClient, I think it's worth it to
look into this so that we don't add a possibly unnecessary parameter

       if (mimic_mac_safari_user_agent_.empty()) {
         mimic_mac_safari_user_agent_ =
-            BuildUserAgentHelper(false, "Version/5.0.4 Safari/533.20.27");
+            BuildUserAgentHelper("Version/5.0.4 Safari/533.20.27");
       }
       return mimic_mac_safari_user_agent_;
     }
 #endif
   }
 
   return user_agent_;
 }
 
 base::LazyInstance<UserAgentState> g_user_agent(base::LINKER_INITIALIZED);
 
 }  // namespace
 
-void SetUserAgent(const std::string& new_user_agent) {
-  g_user_agent.Get().Set(new_user_agent);
+void SetUserAgent(const std::string& user_agent, bool overriding) {
+  g_user_agent.Get().Set(user_agent, overriding);
 }
 
 const std::string& GetUserAgent(const GURL& url) {
   return g_user_agent.Get().Get(url);
 }
 
 void SetForcefullyTerminatePluginProcess(bool value) {
   g_forcefully_terminate_plugin_process = value;
 }
 
@@ skipping 10 matching lines @@
   NOTIMPLEMENTED();
   return NULL;
 #endif
 }
 
 int GetGlyphPageCount() {
   return WebGlyphCache::pageCount();
 }
 
 } // namespace webkit_glue