Bug 55226 – Add appropriate UA string tags for running on Windows x64

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Peter Kasting]

This matches IE and Firefox.

[Peter Kasting]

Clarity:
When running a 32-bit browser app (via WOW64): Add "WOW64" token after OS version
When running a 64-bit browser app (natively): Add "Win64; x64" tokens after OS version

I don't know if anyone is building 64-bit WebKit variants yet.

[Peter Kasting]

Created attachment 85282 [details]
Patch v1

[WebKit Review Bot]

Attachment 85282 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/win/SystemInfo.cpp:149:  system_info is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/win/SystemInfo.cpp:162:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 85282 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=85282&action=review

> Source/WebCore/platform/win/SystemInfo.cpp:123
> +        HMODULE kernel32Module = GetModuleHandle(L"kernel32.dll");

So we always bind to 64-bit version of the API?  Shouldn't we be using TEXT macro?

> Source/WebCore/platform/win/SystemInfo.cpp:124
> +        if (kernel32Module) {

It seems like that an early exist will be cleaner.

> Source/WebCore/platform/win/SystemInfo.cpp:145
> +        if (kernel32Module) {

Ditto about early exist.

> Source/WebCore/platform/win/SystemInfo.cpp:164
> +    else if (processorArchitecture() == PROCESSOR_ARCHITECTURE_AMD64)

Nit: no else here.

[Peter Kasting]

Created attachment 85284 [details]
Patch v2

Fixes style errors and other problems.

[Ryosuke Niwa]

Comment on attachment 85284 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=85284&action=review

Looks sane to me!

> Source/WebCore/platform/win/SystemInfo.cpp:122
> +    if (!initialized) {
> +        initialized = true;

Can't we exit early here instead? as in
if (initialized)
    return wow64;
But not strongly opinionated.

> Source/WebCore/platform/win/SystemInfo.cpp:130
> +            wow64 = ((*isWOW64Process)(GetCurrentProcess(), &result) && result);

I'd prefer isWOW64Process(GetCurrentProcess(), &result) && result; but there doesn't seem to be an explicit rule stated anywhere.

[Peter Kasting]

(In reply to comment #6)
> > Source/WebCore/platform/win/SystemInfo.cpp:122
> > +    if (!initialized) {
> > +        initialized = true;
> 
> Can't we exit early here instead? as in
> if (initialized)
>     return wow64;
> But not strongly opinionated.

After we chatted I'm leaving this alone.

> > Source/WebCore/platform/win/SystemInfo.cpp:130
> > +            wow64 = ((*isWOW64Process)(GetCurrentProcess(), &result) && result);
> 
> I'd prefer isWOW64Process(GetCurrentProcess(), &result) && result; but there doesn't seem to be an explicit rule stated anywhere.

Changed.  Also changed function pointers to not use explicit-deref form ("(*ptr)(args)").  Will commit when bots are green.

[Peter Kasting]

Fixed in r80693.