[webkit-reviews] review denied: [Bug 27158] Add WebKit version API to Qt. : [Attachment 32737] More Portable Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied robert
<robert at roberthogan.net>'s request for review:
Bug 27158: Add WebKit version API to Qt.
https://bugs.webkit.org/show_bug.cgi?id=27158

Attachment 32737: More Portable Patch
https://bugs.webkit.org/attachment.cgi?id=32737&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

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.