Bug 146335

<rdar://problem/21558269>

Created attachment 255708 [details] WIP Patch

Comment on attachment 255708 [details] WIP Patch Works, except for layer-backed content.

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.

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 ?

Created attachment 255945 [details] WIP Do NOT REVIEW!

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.

Created attachment 256938 [details] Patch

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.

Created attachment 257223 [details] Patch

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.

Created attachment 257235 [details] Patch

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.

Created attachment 257277 [details] Patch

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.

Created attachment 257279 [details] Patch

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.

Created attachment 257291 [details] Patch

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.

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

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.

Created attachment 257312 [details] Patch

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.

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

Created attachment 257340 [details] Patch

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.

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

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

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

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

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