[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