Expose DPI helper functions for Runner apps to use #16313

franciscojma86 · 2020-02-01T00:55:42Z

Moves the Win32DpiHelper class into dpi_utils.cc as a detail implementation, exposing GetDpi and EnableNonClientDpiScaling. This way, runner apps will be able to make use of the helper, avoiding the need of duplicating DPI related code.

Also, added a check for nullptr when calling GetDpiForWindow, to use GetDpiForMonitor instead. There are two reasons for this:

The runner app will have to call GetDpi without an HWND to get the scale factor when creating the top-level window.
When the app is set for Per-Monitor V1 awareness, GetDpiForWindow returns a different value than GetDpiForMonitor when using more than one monitor.

Also removed the logic for window resize since WN_DPICHANGE is only called for top-level windows.

flutter/flutter#38599
Part of flutter/flutter#40855.

shell/platform/windows/dpi_utils.cc

shell/platform/windows/dpi_utils.h

shell/platform/windows/public/flutter_windows.h

shell/platform/windows/dpi_utils.cc

stuartmorgan-g

I almost forgot: can you add some simple tests here? I'm not sure if the library lookups will succeed or fail, or if we even have monitors on the CI machines, but we can at least verify that the calls a) don't crash, and b) don't return 0.

shell/platform/windows/dpi_utils.cc

shell/platform/windows/dpi_utils.h

shell/platform/windows/public/flutter_windows.h

shell/platform/windows/flutter_windows.cc

clarkezone

LGTM

shell/platform/windows/testing/win32_window_test.h

franciscojma86 · 2020-02-07T16:54:10Z

shell/platform/windows/win32_flutter_window_unittests.cc

@@ -0,0 +1,21 @@
+#include "flutter/shell/platform/windows/testing/win32_flutter_window_test.h"


This test class is actually not changed. It was originally named win32_window_unittests.cc, but I changed it to the current name since it's testing the Win32FlutterWindow class.

shell/platform/windows/dpi_utils.cc

stuartmorgan-g · 2020-02-10T17:03:48Z

shell/platform/windows/dpi_utils.cc

+    UINT dpi_x = 0, dpi_y = 0;
+    HRESULT result =
+        get_dpi_for_monitor_(monitor, kEffectiveDpiMonitorType, &dpi_x, &dpi_y);
+    return SUCCEEDED(result) ? dpi_x : kDefaultDpi;


Consider:

if (SUCCEEDED(result)) { return dpi_x; }

so that you're not duplicating the fallback return in two places (which could get out of sync in the future.)

shell/platform/windows/dpi_utils.h

shell/platform/windows/testing/win32_window_test.h

stuartmorgan-g · 2020-02-10T17:11:14Z

shell/platform/windows/win32_window.cc

+Win32Window::Win32Window() {
+  // Get the DPI of the primary monitor as the initial DPI. If Per-Monitor V2 is
+  // supported, |current_dpi_| should be updated in the
+  // kWmDpiChangedBeforeParent message.


Is kWmDpiChangedBeforeParent always called once then? (I.e., if a window is created on a non-primary monitor, the right thing will still happen?)

Yeah, if the primary monitor's DPI is different from the one the app is running on, DPI_CHANGED will be called. This is only on V2 since kWmDpiChangedBeforeParent is not called in V1.

shell/platform/windows/win32_window.cc

shell/platform/windows/dpi_utils.h

stuartmorgan-g

LGTM with some small final changes

stuartmorgan-g · 2020-02-11T16:58:31Z

shell/platform/windows/dpi_utils_unittests.cc

+};
+
+TEST(DpiUtilsTest, EqualDpis) {
+  ASSERT_EQ(GetDpiForHWND(nullptr), GetDpiForMonitor(nullptr));


I don't think calling GetDpiForMonitor is valid with a nullptr; I was envisioning using MonitorFromPoint(origin, MONITOR_DEFAULTTOPRIMARY); to get the primary monitor in the test, and calling GetDpiForMonitor with that. In addition to not being a likely abuse of the underlying API, using the primary monitor directly here makes it actually assert what it's supposed to assert.

Sounds good. I also changed dpi_utils.cc to create the monitor from the 0,0 point when monitor is null.

shell/platform/windows/dpi_utils_unittests.cc

shell/platform/windows/testing/win32_window_test.h

stuartmorgan-g · 2020-02-11T17:03:38Z

shell/platform/windows/testing/win32_window_test.h

+  void OnFontChange() override;
+
+ private:
+  FML_DISALLOW_COPY_AND_ASSIGN(Win32WindowTest);


Sorry, missed this before. We don't use FML in the desktop embedding since they aren't part of the public API surface. Also, this macro violates current Google style guide (it's mirroring an old, since-deprecated macro that used to be recommended). Please remove the include of fml, and use direct deletion to avoid copying as is done elsewhere in the desktop embeddings (exampe; note that it should be up with the constructor/destructor, not private as with the macro).

Done, changed in win32_flutter_window_test.h as well

stuartmorgan-g

Sorry, meant to press Approve on the last review since it was an LGTM with small changes.

stuartmorgan-g · 2020-02-11T19:47:35Z

shell/platform/windows/dpi_utils.cc

+    if (monitor == nullptr) {
+      const POINT target_point = {0, 0};
+      monitor = MonitorFromPoint(target_point, MONITOR_DEFAULTTOPRIMARY);
+    }


I don't know that we need this functionality, but I guess it's better for API consistency.

…ngine#16313)

* f49a8b6 Roll src/third_party/skia c03e6982f96f..465864cad5d2 (14 commits) (flutter/engine#16524) * c477c06 Enable verbose logging for shell unittests on Fuchsia (flutter/engine#16526) * a662579 Clear frame references at the end of every CanvasKit frame (flutter/engine#16525) * 3f31ea3 Roll src/third_party/skia 465864cad5d2..21f382c19d76 (6 commits) (flutter/engine#16528) * 38fb6b1 Roll fuchsia/sdk/core/linux-amd64 from 8L7NY... to Bmq1m... (flutter/engine#16529) * 9c0168a Roll fuchsia/sdk/core/mac-amd64 from PMcw3... to 7JkB7... (flutter/engine#16530) * e8a888d Roll src/third_party/skia 21f382c19d76..f83d0346c06a (2 commits) (flutter/engine#16532) * 1e8b331 Roll src/third_party/dart 5244d99a5d4e..5fc031ebc1d7 (42 commits) (flutter/engine#16533) * c4e3ae6 Roll src/third_party/skia f83d0346c06a..88c3793a4eaa (1 commits) (flutter/engine#16534) * 6cdb14e Roll src/third_party/skia 88c3793a4eaa..abefc9c170c9 (1 commits) (flutter/engine#16535) * 975acd8 Roll src/third_party/skia abefc9c170c9..4fe89b4d871d (2 commits) (flutter/engine#16536) * b7424d0 Roll src/third_party/dart 5fc031ebc1d7..30151a654151 (2 commits) (flutter/engine#16537) * 25e8127 Roll src/third_party/skia 4fe89b4d871d..dc2782c380f6 (1 commits) (flutter/engine#16538) * 74fa10c Roll src/third_party/dart 30151a654151..76b18c455e2c (1 commits) (flutter/engine#16539) * 91b8e40 Roll src/third_party/skia dc2782c380f6..cdf2491afa04 (1 commits) (flutter/engine#16540) * 5acf9b1 Roll src/third_party/skia cdf2491afa04..50a490a1a4fb (2 commits) (flutter/engine#16541) * 9897777 Roll src/third_party/skia 50a490a1a4fb..c3b67eb988c8 (4 commits) (flutter/engine#16542) * 78a8909 Use os_log instead of syslog on Apple platforms (flutter/engine#13487) * ea56ad2 libtxt: use a fixture in the benchmarks (flutter/engine#16531) * a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (#13487)" (flutter/engine#16546) * 539f64f [fuchsia] Disable retained layers (flutter/engine#16548) * c3b5072 Expose DPI helper functions for Runner apps to use (flutter/engine#16313) * 5041ff1 support endless trace buffer (flutter/engine#16520) * 6aacf5e Re-land: Use os_log instead of syslog on Apple platforms (flutter/engine#16549) * a5736b8 Roll src/third_party/skia c3b67eb988c8..b1525c721ea6 (4 commits) (flutter/engine#16543) * 49a370f Roll src/third_party/dart 76b18c455e2c..e4c39721c473 (6 commits) (flutter/engine#16544) * 270421c Fix ensureInitializationCompleteAsync callback when already initialized. (#39675) (flutter/engine#16503) * ca02b91 Prevent long flash when switching to Flutter app. (#47903) (flutter/engine#16527) * 44e80fd skiping tests in Safari. LUCI recipe for Mac is ready. this is the only step left for stopping us running unit tests in Safari (flutter/engine#16550) * 5fb0116 iOS platform view gesture blocking policy. (flutter/engine#15940) * e0ebaea Revert "Re-land: Use os_log instead of syslog on Apple platforms (#16549)" (flutter/engine#16558)

…ngine#16313)

* f49a8b6 Roll src/third_party/skia c03e6982f96f..465864cad5d2 (14 commits) (flutter/engine#16524) * c477c06 Enable verbose logging for shell unittests on Fuchsia (flutter/engine#16526) * a662579 Clear frame references at the end of every CanvasKit frame (flutter/engine#16525) * 3f31ea3 Roll src/third_party/skia 465864cad5d2..21f382c19d76 (6 commits) (flutter/engine#16528) * 38fb6b1 Roll fuchsia/sdk/core/linux-amd64 from 8L7NY... to Bmq1m... (flutter/engine#16529) * 9c0168a Roll fuchsia/sdk/core/mac-amd64 from PMcw3... to 7JkB7... (flutter/engine#16530) * e8a888d Roll src/third_party/skia 21f382c19d76..f83d0346c06a (2 commits) (flutter/engine#16532) * 1e8b331 Roll src/third_party/dart 5244d99a5d4e..5fc031ebc1d7 (42 commits) (flutter/engine#16533) * c4e3ae6 Roll src/third_party/skia f83d0346c06a..88c3793a4eaa (1 commits) (flutter/engine#16534) * 6cdb14e Roll src/third_party/skia 88c3793a4eaa..abefc9c170c9 (1 commits) (flutter/engine#16535) * 975acd8 Roll src/third_party/skia abefc9c170c9..4fe89b4d871d (2 commits) (flutter/engine#16536) * b7424d0 Roll src/third_party/dart 5fc031ebc1d7..30151a654151 (2 commits) (flutter/engine#16537) * 25e8127 Roll src/third_party/skia 4fe89b4d871d..dc2782c380f6 (1 commits) (flutter/engine#16538) * 74fa10c Roll src/third_party/dart 30151a654151..76b18c455e2c (1 commits) (flutter/engine#16539) * 91b8e40 Roll src/third_party/skia dc2782c380f6..cdf2491afa04 (1 commits) (flutter/engine#16540) * 5acf9b1 Roll src/third_party/skia cdf2491afa04..50a490a1a4fb (2 commits) (flutter/engine#16541) * 9897777 Roll src/third_party/skia 50a490a1a4fb..c3b67eb988c8 (4 commits) (flutter/engine#16542) * 78a8909 Use os_log instead of syslog on Apple platforms (flutter/engine#13487) * ea56ad2 libtxt: use a fixture in the benchmarks (flutter/engine#16531) * a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (#13487)" (flutter/engine#16546) * 539f64f [fuchsia] Disable retained layers (flutter/engine#16548) * c3b5072 Expose DPI helper functions for Runner apps to use (flutter/engine#16313) * 5041ff1 support endless trace buffer (flutter/engine#16520) * 6aacf5e Re-land: Use os_log instead of syslog on Apple platforms (flutter/engine#16549) * a5736b8 Roll src/third_party/skia c3b67eb988c8..b1525c721ea6 (4 commits) (flutter/engine#16543) * 49a370f Roll src/third_party/dart 76b18c455e2c..e4c39721c473 (6 commits) (flutter/engine#16544) * 270421c Fix ensureInitializationCompleteAsync callback when already initialized. (#39675) (flutter/engine#16503) * ca02b91 Prevent long flash when switching to Flutter app. (#47903) (flutter/engine#16527) * 44e80fd skiping tests in Safari. LUCI recipe for Mac is ready. this is the only step left for stopping us running unit tests in Safari (flutter/engine#16550) * 5fb0116 iOS platform view gesture blocking policy. (flutter/engine#15940) * e0ebaea Revert "Re-land: Use os_log instead of syslog on Apple platforms (#16549)" (flutter/engine#16558) * 8a6b949 [Fuchsia] Dump syslog output after tests have run (flutter/engine#16561) * bca879c Roll src/third_party/dart e4c39721c473..0299903f3e78 (31 commits) (flutter/engine#16553) * cd11d7a Roll fuchsia/sdk/core/mac-amd64 from 7JkB7... to t4kck... (flutter/engine#16555) * 99a265b [web] Fix edge cases in Paragraph.getPositionForOffset to match Flutter (flutter/engine#16557) * 8f8af1f Update felt documentation (flutter/engine#16559) * 13dce50 Roll src/third_party/skia b1525c721ea6..67da665c27ff (32 commits) (flutter/engine#16562) * 7c67573 Fix multiline Javadoc code blocks (flutter/engine#16565) * aece5ad Move log_listener call into the reboot trap (flutter/engine#16564) * 42f18d9 Roll src/third_party/skia 67da665c27ff..886e8500a9f2 (3 commits) (flutter/engine#16566) * c4c6ef6 Samsung keyboard duplication workaround: updateSelection (flutter/engine#16547) * 15062ca Revert "Re-arm timer as necessary in MessageLoopFuchsia" (flutter/engine#16568) * 8802a1d Roll src/third_party/skia 886e8500a9f2..9102c86a81ad (1 commits) (flutter/engine#16570) * dbdcae4 Roll src/third_party/skia 9102c86a81ad..6029cbd560b7 (2 commits) (flutter/engine#16575) * f39bc73 Exposes FlutterSurfaceView, and FlutterTextureView to FlutterActivity and FlutterFragment. (#41984, #47557) (flutter/engine#16552) * db030ec Roll src/third_party/skia 6029cbd560b7..1a733b5b760a (1 commits) (flutter/engine#16577) * 050d29d Roll src/third_party/skia 1a733b5b760a..1d1333fcedf8 (3 commits) (flutter/engine#16578) * 97fd898 Roll fuchsia/sdk/core/mac-amd64 from t4kck... to oHa-O... (flutter/engine#16581) * 2e67866 Roll src/third_party/skia 1d1333fcedf8..3bf3b92dfab0 (1 commits) (flutter/engine#16584)

…6313)" This reverts commit bac380c.

franciscojma86 added 8 commits January 31, 2020 11:31

Refactor helper and expose it

4afaff0

mostly works but with two helpers

f133955

Now with scaling

098a89b

Now with scaling

0ac5ce5

Format

3bc93ab

Clean

1990c7b

Commetns

5d53cb2

Clean

869725a

franciscojma86 requested review from stuartmorgan-g and clarkezone February 1, 2020 00:55

auto-assign bot requested a review from gw280 February 1, 2020 00:55

googlebot added cla: yes labels Feb 1, 2020

franciscojma86 mentioned this pull request Feb 1, 2020

Use DPI utils provided by embedder google/flutter-desktop-embedding#658

Merged

Comment

e6aa604

stuartmorgan-g suggested changes Feb 3, 2020

View reviewed changes

franciscojma86 added 2 commits February 3, 2020 14:45

Address comments

ed6d9a2

Comment

186f56c

franciscojma86 requested a review from stuartmorgan-g February 3, 2020 22:52

stuartmorgan-g reviewed Feb 4, 2020

View reviewed changes

franciscojma86 added 2 commits February 4, 2020 13:10

Review comments

7954dbd

GN

c883807

clarkezone approved these changes Feb 5, 2020

View reviewed changes

franciscojma86 added 5 commits February 5, 2020 14:13

Add test

6762376

Clean

3b1ffc5

Clean

31c39d2

Merge tests

fc9c47c

Comments and format

226581e

franciscojma86 commented Feb 5, 2020

View reviewed changes

shell/platform/windows/testing/win32_window_test.h Show resolved Hide resolved

franciscojma86 requested a review from stuartmorgan-g February 5, 2020 23:46

franciscojma86 commented Feb 7, 2020

View reviewed changes

space

688580c

franciscojma86 requested a review from stuartmorgan-g February 7, 2020 16:56

franciscojma86 added 2 commits February 7, 2020 09:40

Default to primary correctly

838a9c2

License

2a98f8c

stuartmorgan-g suggested changes Feb 10, 2020

View reviewed changes

franciscojma86 added 5 commits February 10, 2020 13:43

Most comments

4ba92ef

Comment

57c11aa

Add test

20dcea8

Add test

6b06bb2

Remove test line

c379a98

franciscojma86 requested a review from stuartmorgan-g February 10, 2020 22:41

Format

d8bce83

stuartmorgan-g suggested changes Feb 11, 2020

View reviewed changes

Clean tests

86ebe6c

franciscojma86 requested a review from stuartmorgan-g February 11, 2020 17:35

stuartmorgan-g approved these changes Feb 11, 2020

View reviewed changes

franciscojma86 added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 11, 2020

fluttergithubbot merged commit c3b5072 into flutter:master Feb 11, 2020

engine-flutter-autoroll mentioned this pull request Feb 12, 2020

Roll engine 0e8ed2715a35..5041ff1b1e37 (23 commits) flutter/flutter#50667

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 12, 2020

c3b5072 Expose DPI helper functions for Runner apps to use (flutter/e…

7111ff3

…ngine#16313)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 12, 2020

c3b5072 Expose DPI helper functions for Runner apps to use (flutter/e…

610cd73

…ngine#16313)

engine-flutter-autoroll mentioned this pull request Feb 12, 2020

Roll engine 0e8ed2715a35..e0ebaea59071 (31 commits) flutter/flutter#50671

Merged

engine-flutter-autoroll mentioned this pull request Feb 14, 2020

Roll engine 0e8ed2715a35..2e6786621f7b (49 commits) flutter/flutter#50803

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 14, 2020

c3b5072 Expose DPI helper functions for Runner apps to use (flutter/e…

0d1ef5a

…ngine#16313)

franciscojma86 mentioned this pull request Feb 19, 2020

Embedding doesn't run on Windows 7 and crashes with an unclear missing DLL message. flutter/flutter#40855

Closed

NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Expose DPI helper functions for Runner apps to use (flutter#16313)

bac380c

NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Revert "Expose DPI helper functions for Runner apps to use (flutter#1…

7740028

…6313)" This reverts commit bac380c.

		@@ -0,0 +1,21 @@
		#include "flutter/shell/platform/windows/testing/win32_flutter_window_test.h"

Expose DPI helper functions for Runner apps to use #16313

Expose DPI helper functions for Runner apps to use #16313

Uh oh!

Conversation

franciscojma86 commented Feb 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clarkezone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

franciscojma86 commented Feb 1, 2020 •

edited

Loading