Skip to content

support device scale factor for GraphicsLayerWC #28842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

r-otaka
Copy link
Contributor

@r-otaka r-otaka commented May 21, 2024

b6d35ab

support device scale factor for GraphicsLayerWC
https://bugs.webkit.org/show_bug.cgi?id=196339

Reviewed by Fujii Hironori.

support device scale factor for GraphicsLayerWC (and WinCairo).
But, NOT using this function now because of following bugs.
so NOW, MiniBrowser sets page zoom to intrinsic device scale factor and
sets "custom device scale factor" to 1.
This simulates the correspondence of device scale factors.

WebInspector always uses intrinsic device scale factor, so if you use
fractional device scale factor, repaint noises may remain.

Remaining bugs
1. Using fractional device scale factor, rendering noises may remain when
   repaint display by JavaScript, scroll with scrollbar dragging or etc.

* Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:
* Source/WebKit/Shared/NativeWebMouseEvent.h:
* Source/WebKit/Shared/NativeWebWheelEvent.h:
* Source/WebKit/Shared/PlatformPopupMenuData.h:
* Source/WebKit/Shared/PlatformPopupMenuData.serialization.in:
* Source/WebKit/Shared/win/NativeWebMouseEventWin.cpp:
* Source/WebKit/Shared/win/NativeWebWheelEventWin.cpp:
* Source/WebKit/Shared/win/WebEventFactory.cpp:
* Source/WebKit/Shared/win/WebEventFactory.h:
* Source/WebKit/UIProcess/Automation/win/WebAutomationSessionWin.cpp:
* Source/WebKit/UIProcess/Inspector/win/WebInspectorUIProxyWin.cpp:
* Source/WebKit/UIProcess/cairo/BackingStoreCairo.cpp:
* Source/WebKit/UIProcess/wc/DrawingAreaProxyWC.cpp:
* Source/WebKit/UIProcess/wc/DrawingAreaProxyWC.h:
* Source/WebKit/UIProcess/win/PageClientImpl.cpp:
* Source/WebKit/UIProcess/win/WebContextMenuProxyWin.cpp:
* Source/WebKit/UIProcess/win/WebPopupMenuProxyWin.cpp:
* Source/WebKit/UIProcess/win/WebPopupMenuProxyWin.h:
* Source/WebKit/UIProcess/win/WebView.cpp:
* Source/WebKit/UIProcess/win/WebView.h:
* Source/WebKit/WebProcess/WebCoreSupport/win/WebPopupMenuWin.cpp:
* Source/WebKit/WebProcess/WebPage/DrawingArea.h:
* Source/WebKit/WebProcess/WebPage/DrawingArea.messages.in:
* Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.cpp:
* Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.h:
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.h:
* Source/WebKit/WebProcess/WebPage/wc/WCUpdateInfo.h:
* Source/WebKit/WebProcess/WebPage/wc/WCUpdateInfo.serialization.in:
* Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:

Canonical link: https://commits.webkit.org/279794@main

bb094f1

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2 🧪 wincairo-tests
✅ 🧪 webkitperl 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
🧪 ios-wk2-wpt 🧪 mac-wk1 ✅ 🛠 wpe-cairo
🧪 api-ios 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv 🧪 mac-AS-debug-wk2 🧪 gtk-wk2
✅ 🛠 tv-sim ❌ 🧪 mac-wk2-stress 🧪 api-gtk
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@@ -1283,7 +1283,7 @@ class WebPageProxy final : public API::ObjectImpl<API::Object::Type::Page>, publ
double viewScaleFactor() const { return m_viewScaleFactor; }
void scaleView(double scale);
void setShouldScaleViewToFitDocument(bool);

Copy link
Contributor

@khei4 khei4 May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this new line is added unintentionally, we could narrow this patch's scope into WC and Win if we remove this line. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I will delete it, thank-you.

Comment on lines 555 to 558
if (!m_page)
m_viewSize = expandedIntSize(FloatSize(LOWORD(lParam), HIWORD(lParam)) / deviceScaleFactorForWindow(hwnd));
else
m_viewSize = expandedIntSize(FloatSize(LOWORD(lParam), HIWORD(lParam)) / m_page->deviceScaleFactor());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: We can reduce redundancy by using ternary op

Suggested change
if (!m_page)
m_viewSize = expandedIntSize(FloatSize(LOWORD(lParam), HIWORD(lParam)) / deviceScaleFactorForWindow(hwnd));
else
m_viewSize = expandedIntSize(FloatSize(LOWORD(lParam), HIWORD(lParam)) / m_page->deviceScaleFactor());
float deviceScaleFactor = m_page ? m_page->deviceScaleFactor() : deviceScaleFactorForWindow(hwnd);
m_viewSize = expandedIntSize(FloatSize(LOWORD(lParam), HIWORD(lParam)) / deviceScaleFactor);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My code has fewer variables than your suggested code.
So, I think that PCs can run my code faster than yours.
Does anyone have any ideas about which is good?

Copy link
Contributor

@khei4 khei4 May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of Course this is style nits, but it seems like the code emitted by clang/gcc don't have any difference between ternary version and original one, if we use any optimization level over 1. https://godbolt.org/z/c87YTf3na

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this like yours.
Thank-you.

@@ -101,8 +101,8 @@ void BackingStore::incorporateUpdate(UpdateInfo&& updateInfo)
return;

#if ASSERT_ENABLED
IntSize updateSize = updateInfo.updateRectBounds.size();
updateSize.scale(m_deviceScaleFactor);
IntSize updateSize = { static_cast<int>(ceilf(updateInfo.updateRectBounds.size().width() * m_deviceScaleFactor)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be IntSize updateSize = expandedIntSize(updateInfo.updateRectBounds.size() * m_deviceScaleFactor);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems smart!
I will change the codes.

@r-otaka r-otaka marked this pull request as draft May 22, 2024 08:10
@@ -584,6 +591,7 @@ void GraphicsLayerWC::flushCompositingStateForThisLayerOnly()
update.background.color = backgroundColor();
if (drawsContent() && contentsAreVisible()) {
update.background.hasBackingStore = true;
update.background.backingStoreSize = WebCore::expandedIntSize(scaled(size(), deviceScaleFactor()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think size() * deviceScaleFactor() is simpler than scaled(size(), deviceScaleFactor()) .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll change it.

@r-otaka r-otaka force-pushed the eng/WebView-should-call-WebPageProxy_setIntrinsicDeviceScaleFactor-for-high-DPI branch from 0e9134b to 71654e2 Compare May 23, 2024 02:03
@r-otaka r-otaka force-pushed the eng/WebView-should-call-WebPageProxy_setIntrinsicDeviceScaleFactor-for-high-DPI branch from 71654e2 to bb094f1 Compare June 7, 2024 05:41
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for bb094f1. Live statuses available at the PR page, #28842

@r-otaka r-otaka marked this pull request as ready for review June 7, 2024 05:44
@fujii fujii added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 7, 2024
https://bugs.webkit.org/show_bug.cgi?id=196339

Reviewed by Fujii Hironori.

support device scale factor for GraphicsLayerWC (and WinCairo).
But, NOT using this function now because of following bugs.
so NOW, MiniBrowser sets page zoom to intrinsic device scale factor and
sets "custom device scale factor" to 1.
This simulates the correspondence of device scale factors.

WebInspector always uses intrinsic device scale factor, so if you use
fractional device scale factor, repaint noises may remain.

Remaining bugs
1. Using fractional device scale factor, rendering noises may remain when
   repaint display by JavaScript, scroll with scrollbar dragging or etc.

* Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:
* Source/WebKit/Shared/NativeWebMouseEvent.h:
* Source/WebKit/Shared/NativeWebWheelEvent.h:
* Source/WebKit/Shared/PlatformPopupMenuData.h:
* Source/WebKit/Shared/PlatformPopupMenuData.serialization.in:
* Source/WebKit/Shared/win/NativeWebMouseEventWin.cpp:
* Source/WebKit/Shared/win/NativeWebWheelEventWin.cpp:
* Source/WebKit/Shared/win/WebEventFactory.cpp:
* Source/WebKit/Shared/win/WebEventFactory.h:
* Source/WebKit/UIProcess/Automation/win/WebAutomationSessionWin.cpp:
* Source/WebKit/UIProcess/Inspector/win/WebInspectorUIProxyWin.cpp:
* Source/WebKit/UIProcess/cairo/BackingStoreCairo.cpp:
* Source/WebKit/UIProcess/wc/DrawingAreaProxyWC.cpp:
* Source/WebKit/UIProcess/wc/DrawingAreaProxyWC.h:
* Source/WebKit/UIProcess/win/PageClientImpl.cpp:
* Source/WebKit/UIProcess/win/WebContextMenuProxyWin.cpp:
* Source/WebKit/UIProcess/win/WebPopupMenuProxyWin.cpp:
* Source/WebKit/UIProcess/win/WebPopupMenuProxyWin.h:
* Source/WebKit/UIProcess/win/WebView.cpp:
* Source/WebKit/UIProcess/win/WebView.h:
* Source/WebKit/WebProcess/WebCoreSupport/win/WebPopupMenuWin.cpp:
* Source/WebKit/WebProcess/WebPage/DrawingArea.h:
* Source/WebKit/WebProcess/WebPage/DrawingArea.messages.in:
* Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.cpp:
* Source/WebKit/WebProcess/WebPage/wc/DrawingAreaWC.h:
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.h:
* Source/WebKit/WebProcess/WebPage/wc/WCUpdateInfo.h:
* Source/WebKit/WebProcess/WebPage/wc/WCUpdateInfo.serialization.in:
* Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:

Canonical link: https://commits.webkit.org/279794@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/WebView-should-call-WebPageProxy_setIntrinsicDeviceScaleFactor-for-high-DPI branch from bb094f1 to b6d35ab Compare June 7, 2024 05:56
@webkit-commit-queue
Copy link
Collaborator

Committed 279794@main (b6d35ab): https://commits.webkit.org/279794@main

Reviewed commits have been landed. Closing PR #28842 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 7, 2024
@webkit-commit-queue webkit-commit-queue merged commit b6d35ab into WebKit:main Jun 7, 2024
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for bb094f1. Live statuses available at the PR page, #28842

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants