[Webkit-unassigned] [Bug 27158] Add WebKit version API to Qt.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27158


Simon Hausmann <hausmann at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32737|review?(hausmann at webkit.org |review-
               Flag|)                           |




--- Comment #12 from Simon Hausmann <hausmann at webkit.org>  2009-07-15 04:42:18 PDT ---
(From update of attachment 32737)

Nice! This looks a lot simpler :)

I have a few small comments below. Otherwise I have only one suggestion:
Instead of generating the entire header file from within the .pro file simply
include the contents of the generated header file in your patch itself. For
example running your script could simply produce WebCore/WebCoreVersion.h that
defines WEBCORE_MAJOR/MINOR_VERSION. Then your patch (here) includes your
script as well as the generated header file.

Afterwards all we need is

1) The other ports including the header file and using it (very simple patch)

and

2) Whenever WebCore/Configurations/Version.xcconfig is touched someone needs to
re-run your perl script to update the header file (simple).


r- because of the above, but otherwise a great step towards cleaning this up!

[...]
>      // webkit/qt version
> -    ua.append(QLatin1String("AppleWebKit/" WEBKIT_VERSION " (KHTML, like Gecko, Safari/419.3) "));
> +    ua.append(QString(QLatin1String("AppleWebKit/%1 (KHTML, like Gecko) "))
> +                      .arg(QString(qWebKitVersion())));
>  
>      // Application name/version
>      QString appName = QCoreApplication::applicationName();
>      if (!appName.isEmpty()) {
> -        ua.append(QLatin1Char(' ') + appName);
> +        ua.append(appName);
>  #if QT_VERSION >= 0x040400
>          QString appVer = QCoreApplication::applicationVersion();
>          if (!appVer.isEmpty())
> @@ -2699,6 +2701,10 @@ QString QWebPage::userAgentForUrl(const QUrl& url) const
>          ua.append(QLatin1String("Qt/"));
>          ua.append(QLatin1String(qVersion()));
>      }
> +
> +    ua.append(QString(QLatin1String(" Safari/%1"))
> +                      .arg(qWebKitVersion()));
> +
>      return ua;
>  }

Out of curiousity, why did you decide to append the Safari bit at the end
instead of just keeping the existing
initual ua.append() line and use arg() twice?

Doesn't this patch also change the parentheses of the user agent?

I think if we change the structure of the user agent string, then we have to
adjust the API documentation of the method, too.

> +void tst_QWebView::getWebKitVersion()
> +{
> +    QVERIFY(qWebKitVersion().toDouble());
> +}
> +

I'm not sure if this test is very useful :)

Given the simplicity of the function I think it's okay to skip this "test"
alltogether if you want.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.