Windows: Add UWP target stub [Flutter#14697] #21754

clarkezone · 2020-10-10T18:50:18Z

Description

This is the first in a series of patches to add support for a Windows UWP target, embedder implementation and runner per flutter/flutter#14967.

This change adds a new build target which is selected with target os winuwp for example python .\flutter\tools\gn --runtime-mode=debug --unoptimized --winuwp and will build a Windows binary flutter_windows_winuwp.dll. The implementation is stub only but the important aspect of this change is that all win32 specifics are now compiled behind an #ifdef which is only active for the win32 target. Once this PR has landed, the intent is to enable the target in CI to give some level of protection for the UWP target as new win32 functionality and os calls are added.

This change cannot be landed until a small 5 line patch is made to dart to support target_os == "winuwp" which will be opened in due course and referenced here. Until that has happened this change will not build successfully in CI and should be consider WIP although it is ready for review now.

Related Issues

flutter/flutter#14967

Tests

Since this change is a stub, testing has not been added in anything but placeholder form.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-10-10T18:50:20Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

stuartmorgan-g

🤦 I just discovered that this has been sitting here as a draft review for over a week. I thought it was waiting for you to update based on feedback.

BUILD.gn

shell/platform/windows/BUILD.gn

shell/platform/windows/flutter_windows.cc

stuartmorgan-g · 2020-10-13T21:56:33Z

shell/platform/windows/flutter_windows_engine.cc

      switches.begin(), switches.end(), std::back_inserter(argv),
      [](const std::string& arg) -> const char* { return arg.c_str(); });

+#ifndef FLUTTER_WINUWP


For cases where we know we are going to need an equivalent and there's nothing in the interface that's Win32-specific, we should really make an interface class--even if we currently don't have a UWP implementation-rather than scattering ifdefs.

Appols, please can you be more specific?

Anything to do here?

Sorry I missed this. In particular, I think we should make an abstract TaskRunner class that the current Win32 version inherits from, and with a temporarily-no-op UWP subclass, and then most of the ifdefs about task runners in this file go away (in fact, if we declare a factory method in the abstract class's header, and implement it in each of the subclass files, then we don't need any ifdefs for task runner at all I think.)

I have added an abstract taskrunner, made the win32 version inherit and add the winrt version as well (bonus.. it actually works ;-)). However, I'm not sure how to implement the factory pattern in a way that would eliminate the ifdef from flutter_windows_engine. Since c++ doesn't support virtual static inheritance i assume you are implying using a template function for the factory? If yes, that will not eliminate the need for an ifdef at the calling site. Also, note that there is another ifdef in that file so even if the factory is possible, that file won't be clean. Moving the ifdefs in question up into the caller will merely add ifdefs to a file that doesn't have any currently. So not quite sure what to do about the last aspect of the request here.

Please reopen if you have more thoughts.

I was envisioning a more low-tech solution:

Declare a freestanding CreateTaskRunner function in task_runner.h

Implement that function twice, once in the Win32 .cc file, and once in the WinRT .cc file

Since only one of the files will be linked, only one implementation will be present.

clarkezone · 2020-11-05T15:07:26Z

One remaining clarifying question. This change is now dependent on a buildroot update: flutter/buildroot#409

stuartmorgan-g

Mostly just some more small structural cleanup so we're not adding more technical debt; overall seems like a good foundation.

stuartmorgan-g · 2020-11-18T21:07:06Z

shell/platform/windows/win32_task_runner.h

 // host app can own its own message loop and flutter still gets to process
 // tasks on a timely basis.
-class Win32TaskRunner {
+class Win32TaskRunner : public TaskRunner {


We should really be calling these classes things like TaskRunnerWin32/TaskRunnerWinRt rather than the reverse. Let's fix that for this class in this PR (since it's adding a new file with the naming we should move away from), and I can do a follow-up to clean up the others after it lands.

BUILD.gn

shell/platform/windows/BUILD.gn

shell/platform/windows/flutter_windows_view.h

shell/platform/windows/flutter_windows_view.cc

shell/platform/windows/winrt_task_runner.cc

shell/platform/windows/winrt_task_runner.h

tools/gn

stuartmorgan-g

I've addressed the comments from my last round of feedback.

@cbracken Since I've taken over the last part of this PR, could you review? I've made significant enough changes that it should get an approval from someone other than me.

clarkezone · 2020-12-01T03:41:32Z

@cbracken do you think you might get to this relatively soon? I'm holding off on the next UWP change until this is merged and I can cleanly rebuild the UWP prototype on ToT.

cbracken

LGTM with one minor question/suggestion.

cbracken · 2020-12-04T21:50:33Z

shell/platform/windows/platform_handler.h

+  // value in a dictionary with the given |key|.
+  virtual void GetPlainText(
+      std::unique_ptr<MethodResult<rapidjson::Document>> result,
+      const char* key) = 0;


Why const char* here rather than std::string_view? Would allow for more flexibility at the call-site. Similarly SetPlainText could take a string_view.

clarkezone · 2020-12-04T22:32:41Z

Awesome, thx, I’ll address that, update to ToT and submit tomorrow. Thx so much Chris!

…21754)

clarkezone requested a review from stuartmorgan-g October 10, 2020 18:50

google-cla bot added the cla: yes label Oct 10, 2020

clarkezone added the Work in progress (WIP) Not ready (yet) for review! label Oct 10, 2020

auto-assign bot requested a review from jason-simmons October 10, 2020 18:51

clarkezone mentioned this pull request Oct 12, 2020

Windows: Add support for a UWP target dart-lang/sdk#43762

Closed

Add UWP Target

6d9defb

clarkezone force-pushed the add-winuwp-target branch from 0625b9b to 6d9defb Compare October 17, 2020 13:50

stuartmorgan-g suggested changes Oct 27, 2020

View reviewed changes

clarkezone added 4 commits October 31, 2020 07:08

Merge remote-tracking branch 'upstream/master' into add-winuwp-target

b0940b7

CR feedback

df7af06

Update licenses

925c571

Move the flutter_windows_winuwp_defs to buildroot

8ec92be

clarkezone mentioned this pull request Oct 31, 2020

Add UWP Config flutter/buildroot#409

Merged

clarkezone added 2 commits November 5, 2020 06:11

CR Feedback: UWP ifdef applied automatically by buildroot

642332d

CR feedback: split flutter_windows.cc

0607fa0

clarkezone removed the Work in progress (WIP) Not ready (yet) for review! label Nov 5, 2020

clarkezone added 2 commits November 7, 2020 06:14

CR Feedback

28cfc6a

Merge remote-tracking branch 'upstream/master' into add-winuwp-target

a4d28f4

stuartmorgan-g mentioned this pull request Nov 10, 2020

Allow building a skeleton UWP embedding flutter/flutter#70196

Closed

10 tasks

Merge remote-tracking branch 'upstream/master' into add-winuwp-target

8cafe53

stuartmorgan-g suggested changes Nov 18, 2020

View reviewed changes

stuartmorgan-g added 7 commits November 19, 2020 11:18

Rename task runners

287029c

Minor cleanup

fd8b690

TaskRunner factory

b6fa938

Abstract PlatformHandler. Minor fixes

9576f5c

Do some testing backfill on the refactored PlatformHandler

e4c8484

Formatting

895088c

Formatting again, but with the right gn

111063c

stuartmorgan-g requested a review from cbracken November 24, 2020 17:39

stuartmorgan-g approved these changes Nov 24, 2020

View reviewed changes

cbracken approved these changes Dec 4, 2020

View reviewed changes

clarkezone added 4 commits December 5, 2020 05:51

Merge remote-tracking branch 'upstream/master' into add-winuwp-target

a3eba8c

Fix UWP build

26f2b55

Fix the win32 build

59d6d53

CR feedback

81b31d7

clarkezone 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 Dec 5, 2020

clarkezone added 2 commits December 5, 2020 07:48

Make licenses happy again

15d6fb6

Make licenses happy again take 2

a4a43c6

fluttergithubbot merged commit 3295beb into flutter:master Dec 6, 2020

engine-flutter-autoroll mentioned this pull request Dec 6, 2020

Roll Engine from 6491c7518f34 to 26c209286675 (3 revisions) flutter/flutter#71812

Closed

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

3295beb Windows: Add UWP target stub [flutter#14697] (flutter/engine#…

1b1c3fc

…21754)

engine-flutter-autoroll mentioned this pull request Dec 7, 2020

Roll Engine from 6491c7518f34 to 69dacc4138d1 (5 revisions) flutter/flutter#71858

Merged

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

3295beb Windows: Add UWP target stub [flutter#14697] (flutter/engine#…

82e41ce

…21754)

clarkezone mentioned this pull request Dec 17, 2020

☂ Add support for UWP flutter/flutter#14967

Closed

Windows: Add UWP target stub [Flutter#14697] #21754

Windows: Add UWP target stub [Flutter#14697] #21754

Uh oh!

Conversation

clarkezone commented Oct 10, 2020

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Oct 10, 2020

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

clarkezone commented Nov 5, 2020

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!

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!

clarkezone commented Dec 1, 2020

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarkezone commented Dec 4, 2020 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clarkezone commented Dec 4, 2020 via email •

edited

Loading