Bug 55226 - Add appropriate UA string tags for running on Windows x64
Summary: Add appropriate UA string tags for running on Windows x64
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on: 55979
Blocks: 54556
  Show dependency treegraph
 
Reported: 2011-02-25 09:35 PST by Peter Kasting
Modified: 2011-03-09 22:31 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (3.98 KB, patch)
2011-03-09 21:26 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Patch v2 (4.06 KB, patch)
2011-03-09 21:50 PST, Peter Kasting
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2011-02-25 09:35:35 PST
This matches IE and Firefox.
Comment 1 Peter Kasting 2011-02-25 10:21:49 PST
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.
Comment 2 Peter Kasting 2011-03-09 21:26:37 PST
Created attachment 85282 [details]
Patch v1
Comment 3 WebKit Review Bot 2011-03-09 21:29:28 PST
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.
Comment 4 Ryosuke Niwa 2011-03-09 21:38:27 PST
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.
Comment 5 Peter Kasting 2011-03-09 21:50:09 PST
Created attachment 85284 [details]
Patch v2

Fixes style errors and other problems.
Comment 6 Ryosuke Niwa 2011-03-09 22:03:42 PST
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.
Comment 7 Peter Kasting 2011-03-09 22:11:29 PST
(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.
Comment 8 Peter Kasting 2011-03-09 22:31:51 PST
Fixed in r80693.