[webkit-reviews] review denied: [Bug 15409] FrameLoaderClientGtk hardcodes data, including platform to Linux i686 : [Attachment 16804] Share systemName, include Windows version

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 5 08:11:19 PST 2007


Alp Toker <alp at atoker.com> has denied Christian Dywan
<christian at twotoasts.de>'s request for review:
Bug 15409: FrameLoaderClientGtk hardcodes data, including platform to Linux
i686
http://bugs.webkit.org/show_bug.cgi?id=15409

Attachment 16804: Share systemName, include Windows version
http://bugs.webkit.org/attachment.cgi?id=16804&action=edit

------- Additional Comments from Alp Toker <alp at atoker.com>
>Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog	(revision 26888)
>+++ WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,19 @@
>+2007-10-22  Christian Dywan  <christian at twotoasts.de>
>+
>+	  Reviewed by NOBODY (OOPS!).
>+
>+	  WARNING: NO TEST CASES ADDED OR CHANGED
>+
>+	  Implement systemName for use in the user agent.
>+
>+	  * WebCore.pro:
>+	  * platform/SystemName.cpp: Added.
>+	  (WebCore::systemName):
>+	  * platform/SystemName.h: Added.
>+	  * platform/win/SystemNameWin.cpp: Added.
>+	  (WebCore::windowsVersion):
>+	  * platform/win/SystemNameWin.h: Added.
>+
> 2007-10-22  Nikolas Zimmermann  <zimmermann at kde.org>
> 
>	  Reviewed by Simon.
>Index: WebCore/WebCore.pro
>===================================================================
>--- WebCore/WebCore.pro	(revision 26888)
>+++ WebCore/WebCore.pro	(working copy)
>@@ -669,6 +669,7 @@ SOURCES += \
>     platform/SharedBuffer.cpp \
>     platform/String.cpp \
>     platform/StringImpl.cpp \
>+    platform/SystemName.cpp \
>     platform/TextCodec.cpp \
>     platform/TextCodecLatin1.cpp \
>     platform/TextCodecUTF16.cpp \
>@@ -955,6 +956,9 @@ gtk-port {
>	  ../WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp \
>	  ../WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp \
>	  ../WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp
>+
>+    unix: 
>+    else: SOURCES += platform/win/SystemNameWin.cpp
> }
> 
> # ENABLE_DATABASE probably cannot be disabled without breaking things
>Index: WebCore/platform/SystemName.cpp
>===================================================================
>--- WebCore/platform/SystemName.cpp	(revision 0)
>+++ WebCore/platform/SystemName.cpp	(revision 0)
>@@ -0,0 +1,63 @@
>+/*
>+ * Copyright (C) 2007 Christian Dywan
>+ *
>+ * Redistribution and use in source and binary forms, with or without
>+ * modification, are permitted provided that the following conditions
>+ * are met:
>+ * 1. Redistributions of source code must retain the above copyright
>+ *	notice, this list of conditions and the following disclaimer.
>+ * 2. Redistributions in binary form must reproduce the above copyright
>+ *	notice, this list of conditions and the following disclaimer in the
>+ *	documentation and/or other materials provided with the distribution.
>+ *
>+ * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
>+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
>+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
>+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
>+ */
>+
>+#include "config.h"
>+#include "SystemName.h"
>+
>+#include "CString.h"
>+#include "PlatformString.h"
>+
>+#if PLATFORM(UNIX)
>+#include <sys/utsname.h>
>+#elif defined(PLATFORM(WIN))
>+#include <win/SystemNameWin.h>
>+#endif
>+
>+namespace WebCore {
>+
>+String systemName(void)
>+{
>+    #if PLATFORM(MAC)
>+	  #if PLATFORM(X86)
>+	  return "Intel Mac OS X";
>+	  #else
>+	  return "PPC Mac OS X";
>+	  #endif
>+    #elif PLATFORM(UNIX)
>+    struct utsname name;
>+    if(uname(&name) != -1){
>+	  return String::format("%s %s", name.sysname, name.machine);
>+    }
>+    else{
>+	  return "Linux i686";
>+    }

Coding style: no braces needed here. I'm pretty sure "Linux i686" is not a good
default value here. Better to either return some kind of "Unknown" or otherwise
pass up an error.

>+    #elif PLATFORM(WIN)
>+    return windowsVersion();
>+    #else
>+    return "Unknown";
>+    #endif
>+}
>+
>+}
>Index: WebCore/platform/SystemName.h
>===================================================================
>--- WebCore/platform/SystemName.h	(revision 0)
>+++ WebCore/platform/SystemName.h	(revision 0)
>@@ -0,0 +1,37 @@
>+/*
>+ * Copyright (C) 2007 Christian Dywan
>+ *
>+ * Redistribution and use in source and binary forms, with or without
>+ * modification, are permitted provided that the following conditions
>+ * are met:
>+ * 1. Redistributions of source code must retain the above copyright
>+ *	notice, this list of conditions and the following disclaimer.
>+ * 2. Redistributions in binary form must reproduce the above copyright
>+ *	notice, this list of conditions and the following disclaimer in the
>+ *	documentation and/or other materials provided with the distribution.
>+ *
>+ * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
>+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
>+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
>+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
>+ */
>+
>+#ifndef SystemName_h
>+#define SystemName_h
>+
>+namespace WebCore {
>+
>+    class String;
>+
>+    String systemName();
>+
>+}
>+
>+#endif // SystemName_h
>Index: WebCore/platform/win/SystemNameWin.cpp
>===================================================================
>--- WebCore/platform/win/SystemNameWin.cpp	(revision 0)
>+++ WebCore/platform/win/SystemNameWin.cpp	(revision 0)
>@@ -0,0 +1,66 @@
>+/*
>+ * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
>+ * Copyright (C) 2007 Christian Dywan
>+ *
>+ * Redistribution and use in source and binary forms, with or without
>+ * modification, are permitted provided that the following conditions
>+ * are met:
>+ *
>+ * 1.  Redistributions of source code must retain the above copyright
>+ *	 notice, this list of conditions and the following disclaimer. 
>+ * 2.  Redistributions in binary form must reproduce the above copyright
>+ *	 notice, this list of conditions and the following disclaimer in the
>+ *	 documentation and/or other materials provided with the distribution. 
>+ * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
>+ *	 its contributors may be used to endorse or promote products derived
>+ *	 from this software without specific prior written permission. 
>+ *
>+ * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
>+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
>+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
>+ * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
>+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES

>+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES;
>+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
AND
>+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT

>+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
>+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>+ */
>+
>+#include "config.h"
>+#include "SystemNameWin.h"
>+
>+#include "CString.h"
>+#include "PlatformString.h"
>+
>+#include <windowsx.h>
>+
>+namespace WebCore {
>+
>+// Copied from osVersion, WebKit/win/WebView.cpp, line 1593
>+String windowsVersion()
>+{
>+    String osVersion;
>+    OSVERSIONINFO versionInfo = {0};
>+    versionInfo.dwOSVersionInfoSize = sizeof(versionInfo);
>+    GetVersionEx(&versionInfo);
>+
>+    if (versionInfo.dwPlatformId == VER_PLATFORM_WIN32_WINDOWS) {
>+	  if (versionInfo.dwMajorVersion == 4) {
>+	      if (versionInfo.dwMinorVersion == 0)
>+		  osVersion = "Windows 95";
>+	      else if (versionInfo.dwMinorVersion == 10)
>+		  osVersion = "Windows 98";
>+	      else if (versionInfo.dwMinorVersion == 90)
>+		  osVersion = "Windows 98; Win 9x 4.90";
>+	  }
>+    } else if (versionInfo.dwPlatformId == VER_PLATFORM_WIN32_NT)
>+	  osVersion = String::format("Windows NT %d.%d",
versionInfo.dwMajorVersion, versionInfo.dwMinorVersion);
>+
>+    if (!osVersion.length())
>+	  osVersion = String::format("Windows %d.%d",
versionInfo.dwMajorVersion, versionInfo.dwMinorVersion);
>+
>+    return osVersion;
>+}
>+
>+} // namespace WebCore

Someone familiar with these issues on Windows should review this part.

>Index: WebCore/platform/win/SystemNameWin.h
>===================================================================
>--- WebCore/platform/win/SystemNameWin.h	(revision 0)
>+++ WebCore/platform/win/SystemNameWin.h	(revision 0)
>@@ -0,0 +1,35 @@
>+/*
>+ * Copyright (C) 2007 Christian Dywan
>+ *
>+ * Redistribution and use in source and binary forms, with or without
>+ * modification, are permitted provided that the following conditions
>+ * are met:
>+ * 1. Redistributions of source code must retain the above copyright
>+ *	notice, this list of conditions and the following disclaimer.
>+ * 2. Redistributions in binary form must reproduce the above copyright
>+ *	notice, this list of conditions and the following disclaimer in the
>+ *	documentation and/or other materials provided with the distribution.
>+ *
>+ * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
>+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
>+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
>+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
>+ */
>+
>+#ifndef SystemNameWin_h
>+#define SystemNameWin_h
>+
>+namespace WebCore {
>+
>+String windowsVersion(void);
>+
>+}
>+
>+#endif // SystemNameWin_h
>Index: WebKit/gtk/ChangeLog
>===================================================================
>--- WebKit/gtk/ChangeLog	(revision 26888)
>+++ WebKit/gtk/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2007-10-22  Christian Dywan  <christian at twotoasts.de>
>+
>+	  Reviewed by NOBODY (OOPS!).
>+
>+	  Computer a proper user agent.
>+
>+	  * ChangeLog:
>+	  * WebCoreSupport/FrameLoaderClientGtk.cpp:
>+	  (WebKit::composeUserAgent):
>+	  (WebKit::FrameLoaderClient::FrameLoaderClient):
>+	  (WebKit::FrameLoaderClient::userAgent):
>+	  * WebCoreSupport/FrameLoaderClientGtk.h:
>+
> 2007-10-22  Alp Toker  <alp at atoker.com>
> 
>	  Reviewed by Mark Rowe.
>Index: WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp
>===================================================================
>--- WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp (revision 26888)
>+++ WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp (working copy)
>@@ -40,10 +40,12 @@
> #include "HTMLFrameElement.h"
> #include "HTMLFrameOwnerElement.h"
> #include "HTMLNames.h"
>+#include "Language.h"
> #include "MIMETypeRegistry.h"
> #include "NotImplemented.h"
> #include "PlatformString.h"
> #include "ResourceRequest.h"
>+#include "SystemName.h"
> #include "CString.h"
> #include "ProgressTracker.h"
> #include "webkitgtkpage.h"
>@@ -55,16 +57,54 @@ using namespace WebCore;
> 
> namespace WebKit {
> 
>+static String composeUserAgent(String applicationName)
>+{
>+    String operatingSystem;
>+
>+    #ifdef GDK_WINDOWING_X11
>+    operatingSystem = "X11";
>+    #elif defined(GTK_WINDOWING_WIN32)
>+    operatingSystem = "Windows";
>+    #elif defined(GTK_WINDOWING_QUARTZ)
>+    operatingSystem = "Macintosh";
>+    #elif defined(GTK_WINDOWING_DIRECTFB)
>+    operatingSystem = "DirectFB";
>+    #else
>+    operatingSystem = "Unknown";
>+    #endif

Perhaps there should be a notImplemented() in the "Unknown" case.

>+
>+    // FIXME: The WebKit version is hardcoded
>+
>+    return String::format("Mozilla/5.0 (%s; U; %s; %s) AppleWebKit/%s (KHTML,
like Gecko) %s"
>+	  , operatingSystem.utf8().data()
>+	  , systemName().utf8().data()
>+	  , defaultLanguage().utf8().data()
>+	  , "523+"
>+	  , applicationName.utf8().data());
>+}

We can fix the hard coded version in a later patch, this is fine for now. bdash
has some ideas here.

>+
> FrameLoaderClient::FrameLoaderClient(WebKitFrame* frame)
>     : m_frame(frame)
>     , m_firstData(false)
> {
>     ASSERT(m_frame);
>+
>+    m_applicationNameStandard = g_get_prgname();
> }
> 
> String FrameLoaderClient::userAgent(const KURL&)
> {
>-    return "Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/420+ (KHTML,
like Gecko)";
>+    if(m_userAgentOverridden)
>+	  return m_userAgentCustom;
>+
>+    if(!m_userAgentStandard.length()){
>+	  if(m_applicationNameOverridden)
>+	      m_userAgentStandard = composeUserAgent(m_applicationNameCustom);
>+	  else
>+	      m_userAgentStandard =
composeUserAgent(m_applicationNameStandard);
>+    }
>+
>+    return m_userAgentStandard;
> }
> 
> WTF::PassRefPtr<WebCore::DocumentLoader>
FrameLoaderClient::createDocumentLoader(const WebCore::ResourceRequest&
request, const SubstituteData& substituteData)
>Index: WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.h
>===================================================================
>--- WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.h	(revision 26888)
>+++ WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.h	(working copy)
>@@ -180,6 +180,12 @@ namespace WebKit {
>     private:
>	  WebKitFrame* m_frame;
>	  WebCore::ResourceResponse m_response;
>+	  WebCore::String m_userAgentStandard;
>+	  WebCore::String m_userAgentCustom;
>+	  WebCore::String m_applicationNameStandard;
>+	  WebCore::String m_applicationNameCustom;
>+	  bool m_userAgentOverridden;
>+	  bool m_applicationNameOverridden;
>	  bool m_firstData;
>     };
> 


Can you consider splitting your patch into two parts?

I can review the GTK+ part straight away, and we can add the
platform/SystemName.h part afterwards, since that part needs wider review
particularly as ports might be able to share it.


More information about the webkit-reviews mailing list