Summary: | [Qt][WK2] fast/forms/access-key-for-all-elements.html fails | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||||||||
Component: | Tools / Tests | Assignee: | Lauro Moura Maranhao Neto <lauro.neto> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abecsi, cmarcelo, code.vineet, hausmann, kenneth, lauro.neto, marcelo.lira, menard, ossy, vestbo, webkit.review.bot, zoltan | ||||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 71854, 87008 | ||||||||||||||||
Attachments: |
|
Description
Csaba Osztrogonác
2011-12-06 06:08:11 PST
I skipped it: http://trac.webkit.org/changeset/102129 Created attachment 123218 [details]
Implement user agent string
Acces key tests were failing due to hardcoded Mac OS X in the user agent
Comment on attachment 123218 [details]
Implement user agent string
This is copy-paste from QWebPage::userAgentForUrl. Please refactor into one shared implementation.
Created attachment 143616 [details]
Add shared user agent implementation for Qt
Comment on attachment 143616 [details] Add shared user agent implementation for Qt View in context: https://bugs.webkit.org/attachment.cgi?id=143616&action=review > Source/WebCore/platform/qt/UserAgentQt.cpp:19 > + * Copyright (C) 2007 Apple Inc. All rights reserved. > + * Copyright (C) 2006, 2007 Apple Inc. All rights reserved. > + * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies) > + * Copyright (C) 2010 Sencha, Inc. All rights reserved. > + * > + * 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 This code comes from qwebpage.cpp, where the copyright notice says Nokia, Staikos and Apple. The license is LGPL 2. The file you're adding has a different set of authors and also a different license. Why is that? Created attachment 143652 [details]
Add shared user agent implementation for Qt. Fixed copyright.
Using the qwebpage copyright on the newly created files.
Comment on attachment 143652 [details] Add shared user agent implementation for Qt. Fixed copyright. View in context: https://bugs.webkit.org/attachment.cgi?id=143652&action=review > Source/WebCore/platform/qt/UserAgentQt.cpp:29 > +#include <WebCore/SystemInfo.h> This doesn't look correct... Why did you add the prefix? > Source/WebCore/platform/qt/UserAgentQt.cpp:31 > +#include <WebKitVersion.h> This I think is the main issue that's left with this patch/approach and I don't see a good way around it. It's a layer violation to call WebKit from within WebCore and in fact it's going to break the force_static_libs_as_shared build. It doesn't quite feel right to move the WebKit version stuff also into WebCore, making me think that perhaps we need another static library for code that's shared between WebKit1 and WebKit2. Comment on attachment 143652 [details] Add shared user agent implementation for Qt. Fixed copyright. View in context: https://bugs.webkit.org/attachment.cgi?id=143652&action=review > Source/WebCore/platform/qt/UserAgentQt.cpp:160 > + // TODO: using macros from WebKit1 until WebKit2 Qt port versioning is sorted out. We use FIXME not TODO :-) > Source/WebCore/platform/qt/UserAgentQt.h:28 > + static String standardUserAgent(const String &applicationNameForUserAgent); defaultUserAgentString makes more sense. The "user agent" is actually the embedding application. defaultUserAgentString(const String& userAgentName) Created attachment 145097 [details]
Patch
(In reply to comment #8) > (From update of attachment 143652 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143652&action=review > > > Source/WebCore/platform/qt/UserAgentQt.cpp:160 > > + // TODO: using macros from WebKit1 until WebKit2 Qt port versioning is sorted out. > > We use FIXME not TODO :-) > > > Source/WebCore/platform/qt/UserAgentQt.h:28 > > + static String standardUserAgent(const String &applicationNameForUserAgent); > > defaultUserAgentString makes more sense. The "user agent" is actually the embedding application. > > defaultUserAgentString(const String& userAgentName) (/me on behalf of Lauro, who is in on his holidays) Kenneth, I left UserAgentQt::standardUserAgent as it was because it follows the naming on WebPageProxy::standardUserAgent. Comment on attachment 145097 [details] Patch Attachment 145097 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12866255 Comment on attachment 145097 [details] Patch Attachment 145097 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12873001 Created attachment 145108 [details]
Patch
Comment on attachment 145108 [details]
Patch
My comment remains about the incorrect layering, so r-.
What if we moved the WebKit version parsing and also this user agent code to the WebKit2 static library and used it from WebKit1?
Created attachment 163865 [details]
Patch
Updated the patch by passing the version as argument to the useragent function.
Comment on attachment 163865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163865&action=review > Source/WebCore/platform/qt/UserAgentQt.cpp:74 > +#ifdef Q_WS_MAC > + "Macintosh; " > +#elif defined Q_WS_QWS > + "QtEmbedded; " > +#elif defined Q_WS_WIN > + // Nothing. > +#elif defined Q_WS_X11 > + "X11; " > +#else > + "Unknown; " > +#endif As a side-note, those Q_WS_* defines don't work anymore :) > Source/WebCore/platform/qt/UserAgentQt.cpp:155 > +#ifdef Q_OS_AIX > + "AIX" > +#elif defined Q_OS_WIN32 > + windowsVersionForUAString() > +#elif defined Q_OS_DARWIN > +#ifdef __i386__ || __x86_64__ > + "Intel Mac OS X" > +#else > + "PPC Mac OS X" > +#endif > + > +#elif defined Q_OS_BSDI > + "BSD" > +#elif defined Q_OS_BSD4 > + "BSD Four" > +#elif defined Q_OS_CYGWIN > + "Cygwin" > +#elif defined Q_OS_DGUX > + "DG/UX" > +#elif defined Q_OS_DYNIX > + "DYNIX/ptx" > +#elif defined Q_OS_FREEBSD > + "FreeBSD" > +#elif defined Q_OS_HPUX > + "HP-UX" > +#elif defined Q_OS_HURD > + "GNU Hurd" > +#elif defined Q_OS_IRIX > + "SGI Irix" > +#elif defined Q_OS_LINUX > + > +#if defined(__x86_64__) > + "Linux x86_64" > +#elif defined(__i386__) > + "Linux i686" > +#else > + "Linux" > +#endif > + > +#elif defined Q_OS_LYNX > + "LynxOS" > +#elif defined Q_OS_NETBSD > + "NetBSD" > +#elif defined Q_OS_OS2 > + "OS/2" > +#elif defined Q_OS_OPENBSD > + "OpenBSD" > +#elif defined Q_OS_OS2EMX > + "OS/2" > +#elif defined Q_OS_OSF > + "HP Tru64 UNIX" > +#elif defined Q_OS_QNX6 > + "QNX RTP Six" > +#elif defined Q_OS_QNX > + "QNX" > +#elif defined Q_OS_RELIANT > + "Reliant UNIX" > +#elif defined Q_OS_SCO > + "SCO OpenServer" > +#elif defined Q_OS_SOLARIS > + "Sun Solaris" > +#elif defined Q_OS_ULTRIX > + "DEC Ultrix" > +#elif defined Q_OS_UNIX > + "UNIX BSD/SYSV system" > +#elif defined Q_OS_UNIXWARE > + "UnixWare Seven, Open UNIX Eight" > +#else > + "Unknown" > +#endif We can also probably clean up this list, some of those OSes are definitely not supported anymore by Qt. > Source/WebCore/platform/qt/UserAgentQt.cpp:161 > + QString version = QString(QLatin1String("%1.%2")).arg(QString::number(webkitMajorVersion), > + QString::number(webkitMinorVersion)); This is also worth cleaing up in a separate patch, maybe using StringBuilder instead of QString(QLatin1String()).arg() just to insert a dot between two QStrings :) Comment on attachment 163865 [details] Patch Clearing flags on attachment: 163865 Committed r128472: <http://trac.webkit.org/changeset/128472> All reviewed patches have been landed. Closing bug. |