Bug 146335 – [Win] Implement High DPI support features for non-layer drawing

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

Extend the existing Windows High DPI features so that we satisfy the elements discussed in the MSDN article <https://msdn.microsoft.com/en-us/library/windows/desktop/dd464659(v=vs.85).aspx>.

Specifically:

1. Listen for WM_DPICHANGED messages and react accordingly.
2. Check for Windows 8.1 High DPI features, linking to them dynamically so we still run on Windows 7 and Windows 8.
3. Set the correct Process DPI Awareness in our test applications to avoid scaling behavior.

[Radar WebKit Bug Importer]

<rdar://problem/21558269>

[Brent Fulgham]

Created attachment 255708 [details]
WIP Patch

[Brent Fulgham]

Comment on attachment 255708 [details]
WIP Patch

Works, except for layer-backed content.

[WebKit Commit Bot]

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


ERROR: Source/WebKit/win/WebView.cpp:965:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/win/WebView.cpp:1249:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebKit/win/WebView.cpp:1292:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/platform/win/ScrollbarThemeWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinLauncher.cpp:486:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 7 in 17 files


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

[Darin Adler]

Comment on attachment 255708 [details]
WIP Patch

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

> Source/WebCore/platform/win/ScrollbarThemeWin.cpp:124
> +    HWndDC dc(0);
> +    float scaleFactor = 96.0f / ::GetDeviceCaps(dc, LOGPIXELSX);
> +    return clampTo<int>(scaleFactor * scrollbarThicknessInPixels());

Seems like this won’t do the right thing if there are multiple screens.

> Source/WebKit/win/ChangeLog:3
> +        [Win] Implement proper High DPI support features

“proper” eh?

> Source/WebKit/win/WebView.cpp:966
> +    // DDDDDD

?

> Source/WebKit/win/WebView.cpp:1002
> +        IntSize logicalSize = newSize;
> +        logicalSize.scale(1.0f / deviceScaleFactor());

This will truncate; is that what we want rather than, say, rounding or doing a ceiling operation?

> Source/WebKit/win/WebView.cpp:1249
> +#if 1 //FLASH_BACKING_STORE_REDRAW

?

> Source/WebKit/win/WebView.cpp:1292
> +#if 1 //FLASH_WINDOW_REDRAW

?

[Brent Fulgham]

Created attachment 255945 [details]
WIP Do NOT REVIEW!

[WebKit Commit Bot]

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


ERROR: Source/WebKit/win/WebView.cpp:965:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/win/ScrollbarThemeWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.cpp:427:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.cpp:427:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Tools/WinLauncher/WinLauncher.cpp:486:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 7 in 20 files


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

[Brent Fulgham]

Created attachment 256938 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/win/WebView.cpp:967:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/win/ScrollbarThemeWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinLauncher.cpp:486:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 18 files


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

[Brent Fulgham]

Created attachment 257223 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/win/WebView.cpp:366:  WM_DPICHANGED is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/WinLauncher/WinLauncher.cpp:486:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinMain.cpp:73:  Declaration has space between type name and * in scaleFactor * s_windowPosition  [whitespace/declaration] [3]
ERROR: Tools/WinLauncher/Common.cpp:108:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 5 in 17 files


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

[Brent Fulgham]

Created attachment 257235 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/WinLauncher/WinLauncher.cpp:486:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinMain.cpp:73:  Declaration has space between type name and * in scaleFactor * s_windowPosition  [whitespace/declaration] [3]
ERROR: Tools/WinLauncher/Common.cpp:108:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 4 in 16 files


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

[Brent Fulgham]

Created attachment 257277 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/WinLauncher/Common.cpp:108:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Source/WebCore/platform/win/ScrollbarThemeWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinLauncher.cpp:486:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinMain.cpp:73:  Declaration has space between type name and * in scaleFactor * s_windowPosition  [whitespace/declaration] [3]
Total errors found: 5 in 20 files


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

[Brent Fulgham]

Created attachment 257279 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/WinLauncher/Common.cpp:108:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Tools/WinLauncher/WinLauncher.cpp:486:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinMain.cpp:73:  Declaration has space between type name and * in scaleFactor * s_windowPosition  [whitespace/declaration] [3]
Total errors found: 4 in 17 files


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

[Brent Fulgham]

Created attachment 257291 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/WinLauncher/Common.cpp:108:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Tools/WinLauncher/WinLauncher.cpp:486:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinMain.cpp:73:  Declaration has space between type name and * in scaleFactor * s_windowPosition  [whitespace/declaration] [3]
Total errors found: 4 in 17 files


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

[Darin Adler]

Comment on attachment 257291 [details]
Patch

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

How did you test this? There are lots of different changes; were they all tested?

I’d like to see us consistently use one name for the inverse scale factor. Currently the patch sometimes calls it just “scale factor”, other times “inverse scale factor”, other times “inverted scale factor”.

> Source/WebCore/platform/win/PlatformMouseEventWin.cpp:52
> +    float scaleFactor = deviceScaleFactor(hWnd);
> +    point.x = clampTo<int>(point.x / scaleFactor);
> +    point.y = clampTo<int>(point.y / scaleFactor);

In ScrollbarThemeWin.cpp you made the scale factor be the inverse of what it is here, and so were able to do more multiplication and less division. Why the different approaches in the different source files?

> Source/WebCore/platform/win/ScrollbarThemeWin.cpp:115
>      static int thickness;
>      if (!thickness)
>          thickness = ::GetSystemMetrics(SM_CXVSCROLL);

This seems strange. Why not just write this?

    static int thickness = ::GetSystemMetrics(SM_CXVSCROLL);

> Source/WebCore/platform/win/ScrollbarThemeWin.cpp:122
> +    HWndDC dc(0);

Is the 0 here really needed? How about using nothing at all? How about nullptr?

> Source/WebCore/platform/win/WheelEventWin.cpp:44
> +static float deviceScaleFactor(HWND window)
> +{
> +    HWndDC dc(window);
> +    return ::GetDeviceCaps(dc, LOGPIXELSX) / 96.0f;
> +}

Why write this function multiple times? Lets put it in a header.

> Source/WebKit/win/WebPreferences.cpp:197
> -    CFDictionaryAddValue(defaults, CFSTR(WebKitStandardFontPreferenceKey), CFSTR("Segoe UI"));
> +    CFDictionaryAddValue(defaults, CFSTR(WebKitStandardFontPreferenceKey), CFSTR("Times New Roman"));

No explanation of this in the change log.

> Source/WebKit/win/WebView.cpp:1255
> +    float inverseScaleFactor = 1.0f / scaleFactor;

Seems like this should consistently be called inverseScaleFactor in all the source files.

> Source/WebKit/win/WebView.cpp:1481
> +    POINT point = { logicalPoint.x() * scaleFactor, logicalPoint.y() * scaleFactor };

I don’t think you need the "=" here.

> Source/WebKit/win/WebView.cpp:1678
> +    IntPoint logicalGestureBeginPoint(gestureBeginPoint.x / scaleFactor, gestureBeginPoint.y / scaleFactor);

Is truncation what we want in all these places where we convert to int? Why not ceiling, rounding, or pixel snapping instead?

> Tools/WinLauncher/Common.cpp:110
> +    HDC dc = ::GetDC(window);
> +    float deviceScaleFactor = ::GetDeviceCaps(dc, LOGPIXELSX) / 96.0f;
> +    ::ReleaseDC(window, dc);

Why not use HWndDC here? Also, it’s annoying to have another duplication copy of this same function!

> Tools/WinLauncher/Common.cpp:135
> +    if (!hURLFont) {
> +        hURLFont = ::CreateFont(scaleFactor * 18, 0, 0, 0, FW_NORMAL, FALSE, FALSE, FALSE, DEFAULT_CHARSET,
> +            OUT_TT_ONLY_PRECIS, CLIP_DEFAULT_PRECIS, ANTIALIASED_QUALITY, FF_DONTCARE, L"Times New Roman");
> +    }

Why is it safe to cache this in a global and not regenerate it? What if the scale factor is different, the next time through?

> Tools/WinLauncher/Common.cpp:158
> +    float scaleFactor = deviceScaleFactorForWindow(0);

How about nullptr instead of 0?

> Tools/WinLauncher/Common.cpp:455
> +            if ((y > window.top) && (y < window.top + (dragBarHeight * gWinLauncher->deviceScaleFactor())))

Too many parentheses I think.

> Tools/WinLauncher/WinLauncher.cpp:488
> +    HDC dc = ::GetDC(m_hMainWnd);
> +    m_deviceScaleFactor = ::GetDeviceCaps(dc, LOGPIXELSX) / 96.0f;
> +    ::ReleaseDC(m_hMainWnd, dc);

Another place that could share the deviceScaleFactor function?

> Tools/WinLauncher/WinLauncherWebHost.h:79
> +    HWND m_hURLBarWnd { 0 };

nullptr

> Tools/WinLauncher/WinLauncherWebHost.h:80
> +    HGDIOBJ m_URLBarFont { 0 };

nullptr

> Tools/WinLauncher/WinLauncherWebHost.h:81
> +    HGDIOBJ m_oldFont { 0 };

nullptr

> Tools/WinLauncher/WinMain.cpp:68
> +    float scaleFactor = deviceScaleFactorForWindow(0);

nullptr

[Brent Fulgham]

Comment on attachment 257291 [details]
Patch

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

>> Source/WebCore/platform/win/PlatformMouseEventWin.cpp:52
>> +    point.y = clampTo<int>(point.y / scaleFactor);
> 
> In ScrollbarThemeWin.cpp you made the scale factor be the inverse of what it is here, and so were able to do more multiplication and less division. Why the different approaches in the different source files?

No, it should be consistent. I'll fix that.

>> Source/WebCore/platform/win/ScrollbarThemeWin.cpp:115
>>          thickness = ::GetSystemMetrics(SM_CXVSCROLL);
> 
> This seems strange. Why not just write this?
> 
>     static int thickness = ::GetSystemMetrics(SM_CXVSCROLL);

Good idea. I'll correct.

>> Source/WebCore/platform/win/ScrollbarThemeWin.cpp:122
>> +    HWndDC dc(0);
> 
> Is the 0 here really needed? How about using nothing at all? How about nullptr?

It takes a window as its argument, so it does need something. But nullptr is fine, and I'll switch to that.

>> Source/WebCore/platform/win/WheelEventWin.cpp:44
>> +}
> 
> Why write this function multiple times? Lets put it in a header.

Done.

>> Source/WebKit/win/WebPreferences.cpp:197
>> +    CFDictionaryAddValue(defaults, CFSTR(WebKitStandardFontPreferenceKey), CFSTR("Times New Roman"));
> 
> No explanation of this in the change log.

I'll correct that. This was just a drive-by fix.

>> Source/WebKit/win/WebView.cpp:1255
>> +    float inverseScaleFactor = 1.0f / scaleFactor;
> 
> Seems like this should consistently be called inverseScaleFactor in all the source files.

Agreed. Fixed.

>> Source/WebKit/win/WebView.cpp:1481
>> +    POINT point = { logicalPoint.x() * scaleFactor, logicalPoint.y() * scaleFactor };
> 
> I don’t think you need the "=" here.

Ok. I should also be using the IntPoint's scale method to make sure the right things are being done for float->int.

>> Source/WebKit/win/WebView.cpp:1678
>> +    IntPoint logicalGestureBeginPoint(gestureBeginPoint.x / scaleFactor, gestureBeginPoint.y / scaleFactor);
> 
> Is truncation what we want in all these places where we convert to int? Why not ceiling, rounding, or pixel snapping instead?

I'll make sure to use IntPoint.scale() so that it does the right thing.

>> Tools/WinLauncher/Common.cpp:110
>> +    ::ReleaseDC(window, dc);
> 
> Why not use HWndDC here? Also, it’s annoying to have another duplication copy of this same function!

HWndDC is internal to WebCore, so we can't use it in the Tools section of our sources. However, I can expose the deviceScaleFactorForWindow() as a utility function, and I'll use that instead. That function uses HWndDC internally.

>> Tools/WinLauncher/Common.cpp:135
>> +    }
> 
> Why is it safe to cache this in a global and not regenerate it? What if the scale factor is different, the next time through?

I'll correct the life cycle handling for this.

>> Tools/WinLauncher/Common.cpp:158
>> +    float scaleFactor = deviceScaleFactorForWindow(0);
> 
> How about nullptr instead of 0?

OK!

>> Tools/WinLauncher/Common.cpp:455
>> +            if ((y > window.top) && (y < window.top + (dragBarHeight * gWinLauncher->deviceScaleFactor())))
> 
> Too many parentheses I think.

OK!

>> Tools/WinLauncher/WinLauncher.cpp:488
>> +    ::ReleaseDC(m_hMainWnd, dc);
> 
> Another place that could share the deviceScaleFactor function?

Yup.

>> Tools/WinLauncher/WinLauncherWebHost.h:79
>> +    HWND m_hURLBarWnd { 0 };
> 
> nullptr

Done.

>> Tools/WinLauncher/WinLauncherWebHost.h:80
>> +    HGDIOBJ m_URLBarFont { 0 };
> 
> nullptr

Done.

>> Tools/WinLauncher/WinLauncherWebHost.h:81
>> +    HGDIOBJ m_oldFont { 0 };
> 
> nullptr

Done.

>> Tools/WinLauncher/WinMain.cpp:68
>> +    float scaleFactor = deviceScaleFactorForWindow(0);
> 
> nullptr

Done.

[Brent Fulgham]

Created attachment 257312 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/win/WebView.cpp:368:  WM_DPICHANGED is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinMain.cpp:77:  Declaration has space between type name and * in scaleFactor * s_windowPosition  [whitespace/declaration] [3]
Total errors found: 3 in 19 files


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

[Brent Fulgham]

(In reply to comment #20)
> Comment on attachment 257291 [details]
> How did you test this? There are lots of different changes; were they all
> tested?

This code is dormant until a WebKit client activates High DPI mode, so all scaling factors will be 1.0, rending all of these changes kind of useless unless you are playing with High DPI settings on Windows (and Opt-in in your application).

I've turned this on for WinLauncher, but DumpRenderTree and the other programs are still blissfully unaware of High DPI.

These changes should have no impact on any rendering behavior, so our existing regression suites cover the changes.

We will be creating new high DPI test cases for Windows once we complete the second part of this work (getting CALayer drawing working properly).

[Brent Fulgham]

Created attachment 257340 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/win/WebView.cpp:368:  WM_DPICHANGED is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/WinLauncher/WinMain.cpp:77:  Declaration has space between type name and * in scaleFactor * s_windowPosition  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/win/PlatformMouseEventWin.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/win/GDIUtilities.h:26:  #ifndef header guard has wrong style, please use: GDIUtilities_h  [build/header_guard] [5]
Total errors found: 4 in 21 files


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

[Brent Fulgham]

I confirmed that this patch passes Windows tests locally; it clearly passes the EWS.

[Brent Fulgham]

Committed r187245: <http://trac.webkit.org/changeset/187245>

[Csaba Osztrogonác]

(In reply to comment #28)
> Committed r187245: <http://trac.webkit.org/changeset/187245>

It broke the Apple Windows build.

[Brent Fulgham]

(In reply to comment #29)
> (In reply to comment #28)
> > Committed r187245: <http://trac.webkit.org/changeset/187245>
> 
> It broke the Apple Windows build.

I'm looking at this now. I'm in the process of fixing some bot configuration issues, so this build failure may not be due to the patch.

[Brent Fulgham]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > Committed r187245: <http://trac.webkit.org/changeset/187245>
> > 
> > It broke the Apple Windows build.
> 
> I'm looking at this now. I'm in the process of fixing some bot configuration
> issues, so this build failure may not be due to the patch.

This build failure is actually due to a mixture of Windows SDK's being used during the build. A clean build corrects the problem.