Work around wrong UA sniffing by Yahoo! JAPAN

webkit/glue/webkit_glue.cc

Index: webkit/glue/webkit_glue.cc
diff --git a/webkit/glue/webkit_glue.cc b/webkit/glue/webkit_glue.cc
index 5e47fe4ee986e7a00b176f8b2b7f24db7f4e6fb7..79919e4bb834ea19d5517e2b5a519fb20d7a70a3 100644
--- a/webkit/glue/webkit_glue.cc
+++ b/webkit/glue/webkit_glue.cc
@@ -350,8 +350,11 @@ class UserAgentState {
 
  private:
   mutable std::string user_agent_;
-  // The UA string when we're pretending to be Mac Safari.
-  mutable std::string mimic_mac_safari_user_agent_;
+
+#if defined(OS_MACOSX) || defined(OS_WIN)

[stuartmorgan, 2011/11/17 09:59:17]

Don't needlessly if-def things that are meaningful-but-unused on other
platforms.

[Yuzo, 2011/11/17 10:32:02]

I have no strong opinions about this. If Tony also thinks that way, I can remove
the IFDEFs.

Just out of curiosity, in what sense is it meaningful on Linux?

[stuartmorgan, 2011/11/17 10:50:38]

The concept of a UA hack is meaningful on Linux (as evidenced by the fact that
we used to have one), it's just not currently used. This code would compile just
fine without the ifdefs, just as the old code did. Compare with, say,
gfx::CGImageToSkBitmap elsewhere in this file, which is meaningless except on
Mac because CGImage doesn't exist anywhere else.

[tony, 2011/11/17 18:13:06]

I agree with Stuart.  Fewer #ifs are better.

[Yuzo, 2011/11/18 07:31:44]

Done.

+  // The UA string when we're pretending to be Mac Safari or Win Firefox.
+  mutable std::string user_agent_for_spoofing_hack_;
+#endif
 
   mutable bool user_agent_requested_;
   bool user_agent_is_overridden_;
@@ -384,6 +387,33 @@ void UserAgentState::Set(const std::string& user_agent, bool overriding) {
   user_agent_ = user_agent;
 }
 
+#if defined(OS_MACOSX) || defined(OS_WIN)
+bool ShouldSpoofUAHack(const GURL& url) {
+  return

[stuartmorgan, 2011/11/17 09:59:17]

Breaking the statement into a bunch of ifdef'd pieces makes this harder to read;
just do a bunch of

#ifdef ...
   if (...)
     return true;
#elif
   if (...)
     return true;
#endif
return false;

[Yuzo, 2011/11/17 10:32:02]

I have no strong opinion on this either. Let's wait for Tony's words.

[tony, 2011/11/17 18:13:06]

I agree with Stuart here too.  The current form is very hard to read.

[Yuzo, 2011/11/18 07:31:44]

Done.

+#if defined(OS_MACOSX)
+    // 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
+    (url.host() == "www.microsoft.com" &&
+     StartsWithASCII(url.path(), "/getsilverlight", false)) ||
+
+    // The following Yahoo! JAPAN pages erroneously judge that Chromium does
+    // not support Silverlight. Until the pages are fixed, spoof the UA.
+    // http://crbug.com/104426
+    (url.host() == "downloads.yahoo.co.jp" &&
+     StartsWithASCII(url.path(), "/docs/silverlight/", true)) ||
+    url.host() == "gyao.yahoo.co.jp" ||
+#elif defined(OS_WIN)
+    (url.host() == "weather.yahoo.co.jp" &&
+     StartsWithASCII(url.path(), "/weather/zoomradar/", true)) ||
+    url.host() == "promotion.shopping.yahoo.co.jp" ||
+#endif
+    (url.host() == "headlines.yahoo.co.jp" &&
+     StartsWithASCII(url.path(), "/videonews/", true));

[stuartmorgan, 2011/11/17 09:59:17]

So... on Windows you spoof this page as Windows Firefox, on Mac you spoof the
same page as Mac Safari, and on Linux you don't spoof? That's really confusing.
Why such a complex system? Needs a comment.

[Yuzo, 2011/11/17 10:32:02]

Silverlight is not supported on Linux.
I don't know how widely Moonshine is used, but at least the Yahoo! JAPAN pages
don't support them.

[stuartmorgan, 2011/11/17 10:50:38]

Please add a comment explaining why you are using the UAs you are using, since
it's non-obvious why having different browser's UAs on the different platforms
is desirable.

Also please add a comment explaining why headlines.yahoo.co.jp is being treated
differently than the other yahoo pages in terms of platforms.

For something as hacky as UA spoofing, there should be a very clear explanation
in the code of why specific choices were made.

[Yuzo, 2011/11/18 07:31:44]

Done.

+}
+#endif
+
 const std::string& UserAgentState::Get(const GURL& url) const {
   base::AutoLock auto_lock(lock_);
   user_agent_requested_ = true;
@@ -392,18 +422,18 @@ const std::string& UserAgentState::Get(const GURL& url) const {
 
   // Workarounds for sites that use misguided UA sniffing.
   if (!user_agent_is_overridden_) {
+#if defined(OS_MACOSX) || defined(OS_WIN)
+    if (ShouldSpoofUAHack(url)) {

[stuartmorgan, 2011/11/17 09:59:17]

I really don't like adding code that appears to be a generic abstraction; it's
not. If we decide to spoof another site tomorrow for Windows, how do you know
Windows Firefox will be the right UA string?

We shouldn't pretend we can make a generic solution for this problem, since it
will likely lead to bad spoofing. Every spoofing case is different, and needs to
be evaluated differently, and use the right UA for that case.

[Yuzo, 2011/11/17 10:32:02]

I'm not pretending anything -- I've just extracted out a function for
refactoring. If you have better naming ideas, please comment.

I think spoofing should be rare -- there is only one spoofing case before this
change. I'd propose not worrying about other spoofing cases until they actually
appear.

[stuartmorgan, 2011/11/17 10:50:38]

My point is that the refactoring suggests that there are two completely
orthogonal concepts--"should we spoof" and "what UA should I use given
that"--when those are *not* orthogonal; on the contrary there is a *very* tight
bond between the URL being spoofed, and the UA that should be used. That's not a
naming problem, it's a fundamental issue with what you have factored apart.

If anyone needed to add more spoofing, they would almost certainly have to undo
this refactoring to do it correctly. We shouldn't do refactoring that is
misleading about the way spoofing should be done, and that makes doing it right
in the future harder.

[Yuzo, 2011/11/18 07:31:44]

Done.

+      if (user_agent_for_spoofing_hack_.empty()) {
+        user_agent_for_spoofing_hack_ =
 #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
-      if (mimic_mac_safari_user_agent_.empty()) {
-        mimic_mac_safari_user_agent_ =
             BuildUserAgentFromProduct("Version/5.0.4 Safari/533.20.27");
+#elif defined(OS_WIN)
+            "Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 "
+            "Firefox/8.0";

[tony, 2011/11/17 18:13:06]

Does Safari Win not work?

[Yuzo, 2011/11/18 07:31:44]

No, it doesn't. Added a comment.

+#endif
       }
-      return mimic_mac_safari_user_agent_;
+      return user_agent_for_spoofing_hack_;
     }
 #endif
   }