Bug 168117 – [Win] Custom scale factor is not applied in all cases.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

It should always be preferred over system scale factor.

[Per Arne Vollan]

Created attachment 301159 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 301159 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Custom scale factor should be preferred over system scale factor.

There isn't enough explanation here for me to understand the reasoning behind this change.

It's also very odd that PlatformMouseEvent needs a scale factor.

[Per Arne Vollan]

(In reply to comment #2)
> Comment on attachment 301159 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301159&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Custom scale factor should be preferred over system scale factor.
> 
> There isn't enough explanation here for me to understand the reasoning
> behind this change.
> 

The problem is that when a custom scale factor is set, there are still a few places where the system scale factor is used, causing rendering artifacts.

> It's also very odd that PlatformMouseEvent needs a scale factor.

This should perhaps be done differently, we scale the platform coordinates to logical coordinates.

Thanks for reviewing :)

[Per Arne Vollan]

rdar://problem/30435303

[Brent Fulgham]

Comment on attachment 301159 [details]
Patch

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

> Source/WebCore/platform/PlatformMouseEvent.h:119
> +        PlatformMouseEvent(HWND, UINT, WPARAM, LPARAM, float scaleFactor, bool didActivateWebView = false);

I don't think we need this (see later comments)

> Source/WebCore/platform/win/PlatformMouseEventWin.cpp:45
> +    float inverseScaleFactor = 1.0f / scaleFactor;

Since we have learned that we can't trust the system scale factor (at least under Windows 7), and we can't use the "correct" ones added in Windows 8, it seems like this whole 'positionForEvent' scaling logic could be removed.

> Source/WebCore/platform/win/PlatformMouseEventWin.cpp:85
> +PlatformMouseEvent::PlatformMouseEvent(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam, float scaleFactor, bool didActivateWebView)

So, don't pass the scaleFactor here.

> Source/WebCore/platform/win/PlatformMouseEventWin.cpp:87
> +    , m_position(positionForEvent(hWnd, lParam, scaleFactor))

... or here.

> Source/WebCore/platform/win/PopupMenuWin.cpp:963
>                      mousePoint.move(-scrollBarRect.x(), -scrollBarRect.y());

I suggest we do the scaling here:

Perhaps add this to IntPoint, or it could just be a local static function:

static LPARAM makeScaledPoint(IntPoint point, float scaleFactor) {
    float inverseScaleFactor = 1.0f / scaleFactor;
    point.scale(inverseScaleFactor, inverseScaleFactor);
    return MAKELPARAM(point.x(), point.y());
}

> Source/WebCore/platform/win/PopupMenuWin.cpp:964
> +                    PlatformMouseEvent event(hWnd, message, wParam, MAKELPARAM(mousePoint.x(), mousePoint.y()), m_scaleFactor);

Then this would become:

PlatformMouseEvent event(hWnd, message, wParam, makeScaledPoint(mousePoint, m_scaleFactor));

> Source/WebCore/platform/win/PopupMenuWin.cpp:1001
> +                    PlatformMouseEvent event(hWnd, message, wParam, MAKELPARAM(mousePoint.x(), mousePoint.y()), m_scaleFactor);

PlatformMouseEvent event(hWnd, message, wParam, makeScaledPoint(mousePoint, m_scaleFactor));

> Source/WebCore/platform/win/PopupMenuWin.cpp:1028
> +                    PlatformMouseEvent event(hWnd, message, wParam, MAKELPARAM(mousePoint.x(), mousePoint.y()), m_scaleFactor);

PlatformMouseEvent event(hWnd, message, wParam, makeScaledPoint(mousePoint, m_scaleFactor));

> Source/WebKit/win/WebView.cpp:1661
> +    PlatformMouseEvent mouseEvent(m_viewWindow, WM_RBUTTONUP, wParam, lParam, deviceScaleFactor());

PlatformMouseEvent event(m_viewWindow, WM_RBUTTONUP, wParam, makeScaledPoint(IntPoint(GET_X_LPARAM(lParam), GET_Y_LPARAM(lParam)), deviceScaleFactor()));

[Brent Fulgham]

The patch looks good, but I think we should do a few things to clean it up before calling it done.

Also: Do we have working 'custom scale factor' tests we can activate on Windows if they aren't already?

[Per Arne Vollan]

Created attachment 302125 [details]
Patch

[Per Arne Vollan]

Created attachment 302128 [details]
Patch

[Per Arne Vollan]

Thanks for reviewing :) Updated patch.

[Per Arne Vollan]

Created attachment 302131 [details]
Patch

[Per Arne Vollan]

Created attachment 302138 [details]
Patch

[Per Arne Vollan]

Created attachment 302153 [details]
Patch

[Brent Fulgham]

Comment on attachment 302153 [details]
Patch

Looks great! R=me

[Per Arne Vollan]

Committed <https://trac.webkit.org/changeset/212652>