Bug 27158 – Add WebKit version API to Qt.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Robert Hogan]

Get the current 'Safari' version of WebKit from Version.xcconfig
        at compile time and make it available to Qt applications through
        webKitVersion().

        Also amend the User Agent string to place the Safari clause outside
        the final bracket and to the end of the UA string.

[Mark Rowe (bdash)]

FWIW, the .xcconfig files do not contain Safari version information.

[Robert Hogan]

Created attachment 32583 [details]
Initial Patch

The approach is to treat WebCore/Configurations/Version.xcconfig as the definitive place for version information. If parsing that file fails, Qt will default to a version number hardcode in WebKit.pri, and failing that a version hardcoded in qwebkitversion.cpp.

Things I'm not sure about yet:

- Choice of hard-coded version number if parsing Version.xcconfig fails - currently 531.3.
- Have only tested the following qmake macro on Linux. It should work on all other platforms, but not 100% certain.

+MAJOR_VERSION = $$system(perl WebKitTools/Scripts/get-webkit-version --major \
+                         WebCore/Configurations/Version.xcconfig)

[Robert Hogan]

Comment on attachment 32583 [details]
Initial Patch

ack, this has a couple of silly errors:

- the warning text is mangled in WebKit.pri
- two of the unit tests won't work

Please ignore those for now.

[Robert Hogan]

(In reply to comment #1)
> FWIW, the .xcconfig files do not contain Safari version information.

Yes, misleading changelog, it's really the WebKit version according to the Apple build of WebKit. I'll make that clearer in the next revision.

[Mark Rowe (bdash)]

If you're going to parse a .xcconfig for the WebKit version, then parsing WebKit's one makes more sense than WebCore.

[Robert Hogan]

Created attachment 32612 [details]
Updated patch

Updated re comments.

[Simon Hausmann]

Robert, I think your idea is great!

A slightly more pragmatic and easier approach would be to create a header file (WebKitVersion.h?) that we can share across the ports who would like to. (That file could be generated with your script for example?)

CC'ing Darin Fisher from the Chromium team and Gustavo from the Gtk+ port. I can imagine that they would be interested in sharing :)

[Simon Hausmann]

Comment on attachment 32612 [details]
Updated patch

r- on the patch for now, I think with little effort this can be even better and simpler, without the need to modify the build system :)

On the Qt side please prefix the functions with 'q', i.e. qWebKitVersion().

[Robert Hogan]

(In reply to comment #8)
> (From update of attachment 32612 [details])
> r- on the patch for now, I think with little effort this can be even better and
> simpler, without the need to modify the build system :)
> 

A generic WebKitVersion.h sounds like a good idea. So generate-bindings.pl or equivalent would generate it and write to 'WebKit/WebKitBuild/Debug|Release/WebCore/generated/debug|release/', avoiding the need to modify the build system as you suggest. I'll give that a go.

> On the Qt side please prefix the functions with 'q', i.e. qWebKitVersion().
Righto!

[Robert Hogan]

Created attachment 32737 [details]
More Portable Patch

Is this more like it?

[Adam Treat]

Personally, I think the thing to do is not to generate it at all.  Rather, I'd have a static regular file in top-level WebKit directory called WebKitVersion.h.  Then modify the Mac port, the Qt port, and whatever other port to use it that wants to.  Do away with the version information in the .xcconfig file altogether in favor of this new header.

This file can then be linked to directly by whatever code needs to use it.

As for who bumps it?  We're all following the Mac ports lead so they can be the ones to bump it.  After all, the whole reason for other ports to use it is to masquerade as Mac WebKit version so websites don't fail.

Cheers,
Adam

[Simon Hausmann]

Comment on attachment 32737 [details]
More Portable Patch


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.

[Robert Hogan]

> 
> 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.

My intention was to remove the need for manual intervention.  I don't understand why keeping version updates manual is necessary, but happy to do what is required. I'll provide an updated patch on this basis.

> 
> Afterwards all we need is
> 
> 1) The other ports including the header file and using it (very simple patch)
> 

No problem. I guess the other ports can chime in at that point.

> and
> 
> 2) Whenever WebCore/Configurations/Version.xcconfig is touched someone needs to
> re-run your perl script to update the header file (simple).
> 
Still not sure why this can't be automated.

> 
> 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?
> 

To bring Qt's user agent string into line with the format adopted by other ports:

http://www.mattcutts.com/blog/google-chrome-user-agent/

http://209.85.229.132/search?q=cache:fd3ykESxOycJ:www.useragentstring.com/pages/Safari/+safari+user+agent&cd=3&hl=en&ct=clnk

http://gitorious.org/qtwebkit/qtwebkit/blobs/1a9740a524a3cc5958c996f945a99ddad144f3f4/WebKit/gtk/webkit/webkitwebsettings.cpp (line 182)

> 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.

Yes, missed that one.

> 
> > +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.

It's intended to catch situations where the auto-generated header file is so broken the defines are not numeric. It should really be toDouble() > 0 in that case.

[Robert Hogan]

> 
> 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?
> 

Actually, should the first version number in the UA string be from WebCore/Configurations/Versions.xcconfig (i.e. the WebKit version) and the second one from WebKit/Configurations/Versions.xcconfig (i.e. the Safari port version)?

In practice they are very often the same, but there have been occasional differences. Would be ideal to mirror all cases. Will verify I have it the right way round before updating.

[Robert Hogan]

Created attachment 32804 [details]
Updated patch for generating WebKitVersion.h manually

Updated re Simon's comments. Haven't touched anything in other ports - don't have their build set up. Can certainly do gtk as a separate job of work though.

Have placed WebKitVersion.h in WebCore/platform/ .. couldn't think of a better place. Also note that I use the Version.xcconfig in WebKit/mac/Configurations as per Mark's advice. It appears that the 'second' occurence of the webkit version number in the UA string is maintained internally by Safari, so standard approach is just to reuse the same webkit version number in both positions.

[Simon Hausmann]

(In reply to comment #13)
> > 
> > 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.
> 
> My intention was to remove the need for manual intervention.  I don't
> understand why keeping version updates manual is necessary, but happy to do
> what is required. I'll provide an updated patch on this basis.

It just seems a whole lot easier for the ports to use if all they have to do is to include a header file, instead of calling scripts in their build system.

Mark, do you remember why the windows port uses a version text file and auto-generates the header file from it, in contracts to a regular header file?

> > 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?
> > 
> 
> To bring Qt's user agent string into line with the format adopted by other
> ports:
> 
> http://www.mattcutts.com/blog/google-chrome-user-agent/
> 
> http://209.85.229.132/search?q=cache:fd3ykESxOycJ:www.useragentstring.com/pages/Safari/+safari+user+agent&cd=3&hl=en&ct=clnk
> 
> http://gitorious.org/qtwebkit/qtwebkit/blobs/1a9740a524a3cc5958c996f945a99ddad144f3f4/WebKit/gtk/webkit/webkitwebsettings.cpp
> (line 182)

Ahh, I see. That's a good point :)


One additional comment: I realize that if we take the version from WebKit/mac/Configurations/Version.xcconfig, then we should put the header file probably into WebKit/WebKitVersion.h, instead of taking the version from WebKit and creating a header file in WebCore.


Robert, perhaps at this point the easiest would be to split up your patch into two parts:

1) One that deals with WebKitVersion.h, and we can figure out among the ports how to best handle and generate it.

2) The bigger part of your current patch that adds the API to the Qt Port, with the fallback. That also changes the user agent and its documentation.

[Robert Hogan]

(In reply to comment #16)
> It just seems a whole lot easier for the ports to use if all they have to do is
> to include a header file, instead of calling scripts in their build system.
> 
> Mark, do you remember why the windows port uses a version text file and
> auto-generates the header file from it, in contracts to a regular header file?
> 

FWIW, the gtk port does the same as Windows. They have a webkitversion.h.in which configure.ac populates from values manually updated (AFAICT) in configure.ac.

Generating a version header file from the Version.xcconfig at build time would avoid the need for this individual effort, and would avoid the need for any additional manual intervention for version tracking. It would also mean that the webkit version in the UA string would always mean the same thing for all webkit ports.

Additionally, nothing fancy should be required to generate the header file in other ports build systems, since it can just be added to the same list of calls currently generating source from *.idl files at build.

> 
> One additional comment: I realize that if we take the version from
> WebKit/mac/Configurations/Version.xcconfig, then we should put the header file
> probably into WebKit/WebKitVersion.h, instead of taking the version from WebKit
> and creating a header file in WebCore.
> 
Yes, was conscious WebCore/ seemed a funny place to put it, but WebKit/ root has no other source files in it at all - so it felt the lesser of two evils.

> 
> Robert, perhaps at this point the easiest would be to split up your patch into
> two parts:
> 
> 1) One that deals with WebKitVersion.h, and we can figure out among the ports
> how to best handle and generate it.
> 
> 2) The bigger part of your current patch that adds the API to the Qt Port, with
> the fallback. That also changes the user agent and its documentation.

NP. Will await a final direction on whether to go with the manual or auto-generated WebKitVersion.h before submitting the first one. Also, can you confirm that the root of WebKit/ is the right place to put it?

[Robert Hogan]

IRC conversation related to bug:
[20:15] <manyoso> bdash: re: #27158
[20:17] <manyoso> bdash: i think the ideal solution would be to have a WebKitVersion.h file somewhere in the source repository that only you guys would bump.  Then ports that wished to could use this header file for UA strings and so on and so forth.  That way no files need be generated.  Would you be open to a patch which modified your build files to get version info from such a canonical WebKitVersion.h file?
[20:18] <bdash> manyoso: no
[20:18] <bdash> manyoso: i don't see how that can work
[20:18] <bdash> manyoso: build systems dont understand header files.
[20:19] <bdash> manyoso: and it also wouldn't fit with our build process
[20:19] <manyoso> bdash: how about a .txt file with two lines for major and minor version?
[20:20] <bdash> manyoso: a) there's more than two components to the version number
[20:20] * manyoso listens
[20:20] <bdash> manyoso: b) it still wouldn't work.  we need to be able to build each project independently
[20:20] <bdash> manyoso: and c) the build system doesn't understand text files either.
[20:20] <manyoso> ok, the qmake build system could easily deal with it...
[20:20] <manyoso> hmm
[20:23] <manyoso> mwenge: ok, given bdash answer i don't know enough about the build systems involved.  your generation script might be way to go.
[20:23] <manyoso> mwenge: i'll let simon and you sort it out...

[Kenneth Rohde Christiansen]

Regarding:

+#ifndef WEBKITVERSION_H
+#define WEBKITVERSION_H

The coding style rule 14 says: #define, #ifdef "header guards" should be named exactly the same as the file (including case), replacing the '.' with a '_'.

Thus, this should be #define WebKitVersion_h, etc.

[Simon Hausmann]

(In reply to comment #17)
> Generating a version header file from the Version.xcconfig at build time would
> avoid the need for this individual effort, and would avoid the need for any
> additional manual intervention for version tracking. It would also mean that
> the webkit version in the UA string would always mean the same thing for all
> webkit ports.
> 
> Additionally, nothing fancy should be required to generate the header file in
> other ports build systems, since it can just be added to the same list of calls
> currently generating source from *.idl files at build.

That is a very good point actually :)

> NP. Will await a final direction on whether to go with the manual or
> auto-generated WebKitVersion.h before submitting the first one. Also, can you
> confirm that the root of WebKit/ is the right place to put it?

You're actually right with regards to auto-generation. Generating the header at build time is not as fancy as I thought. Ok, let's with your approach in attachment 3 (to generate the header into the generated sources directory).

In addition to Kenneth's comment about the header guards (small nitpick) the only other thing I can think of right now is to include config.h as first header file in qwebkitversion.cpp, followed by "qwebkitversion.h" and the generated header.

I'm sorry for the back and forth :(. You've definitely convinced me now about generating the header at build time (no need to check it in or manually update it), and it seems you reached that conclusion on IRC, too.

[Mark Rowe (bdash)]

Comment on attachment 32804 [details]
Updated patch for generating WebKitVersion.h manually

This header file should be generated at build time.  Checking in a generated source file doesn't make a lot of sense.

[Robert Hogan]

Created attachment 33559 [details]
Updated patch

Sorry for the delay in responding - was away for a week. Patch updated per Simon and Kenny's comments.

[Simon Hausmann]

Comment on attachment 33559 [details]
Updated patch

r=me. Thanks!

I'll move the script to WebKit/scripts when landing, as that's a slightly more appropriate place I realize for a script that parses stuff from WebKit/ instead of WebCore/ :)

[Simon Hausmann]

Landed in r46485 with a few small compile tweaks :)

[Robert Agoston]

Created attachment 33651 [details]
proposed patch

[Simon Hausmann]

Comment on attachment 33651 [details]
proposed patch

r=me. Mea culpa, I was sure I had that fixed when landing, but looks like it slipped ;(

[Simon Hausmann]

Comment on attachment 33559 [details]
Updated patch

Clearing review, as it needs more work. Will look at the dll linkage warning and remove the macros from the enums.

[Simon Hausmann]

Comment on attachment 33651 [details]
proposed patch

Clearing for this one, too.

[Kenneth Rohde Christiansen]

Comment on attachment 33559 [details]
Updated patch

Clearing review, as it needs more work. Will look at the dll linkage warning
and remove the macros from the enums.

???? 

You landed it and now you clear review as it needs more work? I'm a bit confused; did you comment on the right bug?

[Simon Hausmann]

(In reply to comment #29)
> (From update of attachment 33559 [details] [details])
> Clearing review, as it needs more work. Will look at the dll linkage warning
> and remove the macros from the enums.
> 
> ???? 
> 
> You landed it and now you clear review as it needs more work? I'm a bit
> confused; did you comment on the right bug?

Yes, wrong bug by accident :)