Merge `Window` into `FlutterWindow` by yaakovschectman · Pull Request #45542 · flutter/engine

yaakovschectman · 2023-09-07T17:14:50Z

Merge abstract base class Window into concrete derived class FlutterWindow to simplify future development.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

shell/platform/windows/flutter_window.h

cbracken

Making the assumption that other than signatures being merged, the implementation details haven't changed in the bulk of this code.

Just a few quick initial comments.

shell/platform/windows/flutter_window.h

cbracken · 2023-09-07T17:59:33Z

shell/platform/windows/flutter_window.h

+  void HandleResize(UINT width, UINT height);
+
+  // Retrieves a class instance pointer for |window|
+  static FlutterWindow* GetThisFromHandle(HWND const window) noexcept;


style nit: We should consider gathering the static class methods together at the top of each public/protected/private section.

cbracken · 2023-09-07T18:09:23Z

shell/platform/windows/flutter_window.h

+
+  int current_dpi_ = 0;
+  int current_width_ = 0;
+  int current_height_ = 0;


We should order members in sections. I'll check the style guide but my recollection is: statics, regular methods, fields. Just make sure we keep fields ordered the same as constructor field initialisation to avoid undefined behaviour.

cbracken · 2023-09-07T18:17:24Z

shell/platform/windows/flutter_window.cc

+// input.
+// See
+// https://docs.microsoft.com/en-us/windows/win32/tablet/system-events-and-mouse-messages?redirectedfrom=MSDN#distinguishing-pen-input-from-mouse-and-touch.
+static FlutterPointerDeviceKind GetFlutterPointerDeviceKind() {


nit: For static functions (as opposed to class methods), for conistency, we should probably move these to the anonymous namespace instead of old-school static.

cbracken · 2023-09-07T20:09:15Z

shell/platform/windows/flutter_window.h

+  const static long kWmDpiChangedBeforeParent = 0x02E2;
+
+  // Timer identifier for DirectManipulation gesture polling.
+  const static int kDirectManipulationTimer = 1;


Here and above, while you're in here, let's also switch these to static const for consistency with elsewhere.

cbracken

Looks great - thanks!

dkwingsmt · 2023-09-07T21:53:54Z

Haven't really compared the merged files line by line. Are there any lines worth checking? If not, it looks good to me. :)

yaakovschectman · 2023-09-07T22:00:08Z

Are there any lines worth checking?

There's nothing that's really new code. Just existing code moved to new places and some type names updated to reflect that.

gspencergoog

shell/platform/windows/testing/mock_window.h

loic-sharma · 2023-09-07T23:07:25Z

shell/platform/windows/testing/mock_window.h


 /// Mock for the |Window| base class.
-class MockWindow : public Window {
+class MockWindow : public FlutterWindow {


Consider renaming this type to MockFlutterWindow and this file to mock_flutter_window. I don't feel strongly about this though.

loic-sharma · 2023-09-07T23:14:05Z

shell/platform/windows/flutter_window.h

+
+  // Registers a window class with default style attributes, cursor and
+  // icon.
+  WNDCLASS RegisterWindowClass(std::wstring& title);


Could you do a pass on protected members to check if they can be made private instead? It looks like RegisterWindowClass should be private, there may be more.

loic-sharma

LGTM, thanks for cleaning this up!

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

flutter/engine@06696e7...8389ad9 2023-09-11 109111084+yaakovschectman@users.noreply.github.com Merge `Window` into `FlutterWindow` (flutter/engine#45542) 2023-09-11 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 7Zk_dvFh301kgQte4... to fSmVaZwp41ZGp5hKn... (flutter/engine#45665) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from 7Zk_dvFh301k to fSmVaZwp41ZG If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

yaakovschectman added 8 commits September 1, 2023 14:19

Add members to FlutterWindow

2983953

Remove Window parent

947d820

Compiles

a6b3947

Passes unit tests

b8fadb8

Cull includes

56273df

Remove old window from BUILD

4823a50

Delete old window files

a0cee80

Formatting

4ef1f23

yaakovschectman added affects: desktop platform-windows labels Sep 7, 2023

yaakovschectman requested review from cbracken and loic-sharma September 7, 2023 17:14

yaakovschectman self-assigned this Sep 7, 2023

cbracken reviewed Sep 7, 2023

View reviewed changes

shell/platform/windows/flutter_window.h Show resolved Hide resolved

Licenses

9d5e0a8

yaakovschectman marked this pull request as ready for review September 7, 2023 18:05

cbracken reviewed Sep 7, 2023

View reviewed changes

Reorganize members

a2237e3

cbracken reviewed Sep 7, 2023

View reviewed changes

cbracken approved these changes Sep 7, 2023

View reviewed changes

PR Feedback

26db6f5

yaakovschectman requested review from dkwingsmt and gspencergoog September 7, 2023 21:18

gspencergoog approved these changes Sep 7, 2023

View reviewed changes

loic-sharma reviewed Sep 7, 2023

View reviewed changes

shell/platform/windows/testing/mock_window.h Outdated Show resolved Hide resolved

loic-sharma reviewed Sep 7, 2023

View reviewed changes

loic-sharma approved these changes Sep 7, 2023

View reviewed changes

yaakovschectman and others added 3 commits September 8, 2023 15:45

Update shell/platform/windows/testing/mock_window.h

1b9ab60

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

Privatize

de053cb

Merge branch 'main' into merge_windows

dbbc749

yaakovschectman merged commit 8389ad9 into flutter:main Sep 11, 2023

yaakovschectman deleted the merge_windows branch September 11, 2023 21:59

engine-flutter-autoroll mentioned this pull request Sep 11, 2023

Roll Flutter Engine from 06696e768c1b to 8389ad914b21 (2 revisions) flutter/flutter#134469

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2023

8389ad9 Merge Window into FlutterWindow (flutter/engine#45542)

ee674df

Conversation

yaakovschectman commented Sep 7, 2023

Pre-launch Checklist

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbracken Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Sep 7, 2023

Uh oh!

yaakovschectman commented Sep 7, 2023

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

loic-sharma Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants