[webkit-reviews] review denied: [Bug 124454] [Win] WebKit version in user agent string is incorrect. : [Attachment 217466] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied peavo at outlook.com's request for
review:
Bug 124454: [Win] WebKit version in user agent string is incorrect.
https://bugs.webkit.org/show_bug.cgi?id=124454

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

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217466&action=review


A really great idea.  I think it needs a little bit of refinement to be right.
I'd also like the Apple version to use your logic.

> Source/WebKit/ChangeLog:8
> +	   * WebKit.vcxproj/WebKit/WebKitPreBuild.cmd: Generate WebKitVersion.h


... from OS X Version.xcconfig input file.

> Source/WebKit/WebKit.vcxproj/WebKit/WebKitPreBuild.cmd:11
>  bash "%WEBKIT_LIBRARIES%\tools\scripts\auto-version.sh" "%INTDIR%"

auto-version.sh is supposed to do something clever about version, but I see
that its input file is set to 534, rather than 538 (which is what we have in
Version.xcconfig).

I much prefer your approach, which allows us to control version in one place
and keep them in sync.

> Source/WebKit/WebKit.vcxproj/WebKit/WebKitPreBuild.cmd:17
> +if not exist "%WEBKITVERSIONFILE%" bash -c 'perl "%WEBKITVERSIONSCRIPT%"
--config "%WEBKITVERSIONCONFIG%"  --outputDir "%WEBKITVERSIONDIR%"'

You need to make sure that %WEBKITVERSIONFILE% is deleted when you clean the
project, or else it will never update when the value in Version.xcconfig
changes.

We should ideally drive generation of the version file based on changes to
Version.xcconfig, but I'm not sure how we would do so in Visual Studio. We
could add Version.xcconfig to the WebKit solution with a custom build step that
would do this work, rather than having it done as part of WebKitPreBuild.cmd.

Either way, this is wrong because you will get one version number from
Version.xcconfig, and then it will never change again unless you delete the
file.

> Source/WebKit/win/WebView.cpp:2500
> +#endif

Please revise as follows:

#if !USE(CAIRO)
LPWSTR buildNumberStringPtr;
if (::LoadStringW(gInstance....)
    return buildNumberStringPtr;
#endif

    return String::format("%d.%d", WEBKIT_MAJOR_VERSION, WEBKIT_MINOR_VERSION);

}