Add plumbing for command line arguments on Windows #22094

gw280 · 2020-10-24T01:59:57Z

Description

Adds code to pass through command line arguments on Windows.

Unfortunately because of the C++ (client_wrapper) -> C (flutter_windows) -> C++ (flutter_project_bundle) -> C (embedder API) -> C++ (engine) we have to keep copying and changing between C-style and C++ style arguments :(

Related Issues

flutter/flutter#32986

Tests

None

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-24T01:59:59Z

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.

shell/platform/windows/client_wrapper/flutter_engine.cc

stuartmorgan

This needs tests. I know testing the engine part is waiting on the proctable PR, but you can test the client wrapper parts now. There's already a unit testing system in place at that layer.

shell/platform/windows/flutter_project_bundle.h

shell/platform/windows/public/flutter_windows.h

gw280 · 2020-10-27T02:39:48Z

shell/platform/windows/client_wrapper/flutter_engine_unittests.cc

@@ -20,6 +20,7 @@ class TestFlutterWindowsApi : public testing::StubFlutterWindowsApi {
  FlutterDesktopEngineRef EngineCreate(
      const FlutterDesktopEngineProperties& engine_properties) {
    create_called_ = true;
+    engine_properties_ = engine_properties;


Quick update: this is crashing on LUCI and I can reproduce it on my end. The tl;dr is that engine_properties is deep copied, but that includes just copying the dart_entrypoints_argv pointer itself, which is then invalidated when FlutterEngine's ctor returns. Figuring out a good solution now...

So I ended up just deep copying the dart_entrypoint_argv into a std::vector<std::string> in the stubbed call to EngineCreate. I considered a few alternatives but the complexity didn't seem worth it for a single test case.

We could also possibly look into pulling in FlutterProjectBundle to the test harness and initialising that from the DartProject (which is what the normal engine codepath does) but strictly speaking FlutterProjectBundle is not part of client_wrapper.

shell/platform/windows/client_wrapper/flutter_engine.cc

stuartmorgan

LGTM with nits

shell/platform/windows/client_wrapper/flutter_engine_unittests.cc

…ngine#22094)

* 53d5d68 Add dart-lang/sdk's new package:clock dependency (flutter/engine#22142) * c32e3d8 Roll Skia from 7737a5bd2510 to 5567a6091ceb (8 revisions) (flutter/engine#22146) * 376045c Roll Fuchsia Linux SDK from XYN02FThN... to UKgKCjxSA... (flutter/engine#22147) * 03395de Roll Fuchsia Mac SDK from GKPwGj1Ux... to xHjtLQPQ5... (flutter/engine#22151) * 0c7e952 Manual Dart SDK roll 6e015bd9cddb to 9c6e76468ca4 (6 revisions (flutter/engine#22153) * e5f168a Update constraint to latest integration test (flutter/engine#22169) * e61e8c2 Smooth window resizing on macOS (flutter/engine#21525) * acece00 Allow parse_and_send to use access tokens (flutter/engine#22019) * 0270632 Includes roles for links, checkboxes, and radio buttons in semantics (flutter/engine#22061) * 632354d Roll Fuchsia Linux SDK from UKgKCjxSA... to PY5hNI-wY... (flutter/engine#22175) * 62d50af Add plumbing for command line arguments on Windows (flutter/engine#22094) * 06b0910 Fix possible use of std::moved value in Rasterizer (flutter/engine#22125) * 005dec4 [web] Clean up unused previousStyle logic (flutter/engine#22150) * ca05bdc Roll Skia from 5567a6091ceb to f548a028ce70 (7 revisions) (flutter/engine#22155) * 5b07ac4 Roll Fuchsia Mac SDK from xHjtLQPQ5... to ICK_JlnKJ... (flutter/engine#22188) * d615a97 Roll Fuchsia Linux SDK from PY5hNI-wY... to Usec4YBzR... (flutter/engine#22197) * 11ed711 Invalidate the cached SkParagraph font collection when a new font manager is installed (flutter/engine#22157) * 07c780b [web] Assign default natural width/height for svgs that report 0,0 on firefox and ie11 (flutter/engine#22184) * b54bb88 Migrate runZoned to runZonedGuarded (flutter/engine#22198) * 247139a [web] Fix transform not invalidating path bounds causing debugValidate failure (flutter/engine#22172) * e4dffc1 [web] Fix scroll wheel line delta on Firefox. (flutter/engine#21928) * d6627c6 Reland [ios] Refactor IOSSurface factory and unify surface creation (flutter/engine#22016) * ce1dd11 Standardize macro names for dartdoc macros (flutter/engine#22180) * f81bc37 [profiling] Handle thread_info to account for killed threads (flutter/engine#22170) * fd94c86 Fix for firefox custom clipping (flutter/engine#22182) * 9ccf9f1 bring back build_test to ensure we validate licenses (flutter/engine#22201) * 38f6665 Set strut font to Roboto if the given font hasn't been registered (flutter/engine#22129) * caf32d5 Add a proc table version of embedder API (flutter/engine#21813) * ed0f477 Use preTranslate when applying offset to matrix (flutter/engine#21648) * b457e2d Refactor make_mock_engine into fl_test (flutter/engine#21585) * 28497c8 Fix typos in FlValue docs (flutter/engine#21875) * bb32446 Fix FlTextInputPlugin tear down (flutter/engine#22007) * 7a7804b Add "input shield" to capture pointer input for reinjection (flutter/engine#22067) * fe85f94 Update painting.dart (flutter/engine#22195) * 99cc50d [ios] Surface factory holds the canonical reference to the external view embedder (flutter/engine#22206)

…ngine#22094)

Add plumbing for command line arguments on Windows

935a3df

gw280 requested a review from stuartmorgan October 24, 2020 02:00

googlebot added the cla: yes label Oct 24, 2020

gw280 mentioned this pull request Oct 24, 2020

Send command line arguments through to the Flutter Engine on Windows flutter/flutter#68931

Merged

13 tasks

chinmaygarde approved these changes Oct 24, 2020

View reviewed changes

shell/platform/windows/client_wrapper/flutter_engine.cc Outdated Show resolved Hide resolved

stuartmorgan requested changes Oct 26, 2020

View reviewed changes

shell/platform/windows/flutter_project_bundle.h Outdated Show resolved Hide resolved

shell/platform/windows/public/flutter_windows.h Outdated Show resolved Hide resolved

George Wright added 2 commits October 26, 2020 12:48

Review updates

fd787a4

Add a unit test for setting dart entrypoint args in the client wrapper

b174d63

gw280 commented Oct 27, 2020

View reviewed changes

cbracken reviewed Oct 27, 2020

View reviewed changes

shell/platform/windows/client_wrapper/flutter_engine.cc Show resolved Hide resolved

Fix unit test

bf3dd15

gw280 force-pushed the gwright-windows-commandline branch from 0039682 to bf3dd15 Compare October 27, 2020 19:22

gw280 requested a review from stuartmorgan October 27, 2020 19:26

stuartmorgan approved these changes Oct 27, 2020

View reviewed changes

George Wright added 3 commits October 27, 2020 14:01

review updates

92ea2c9

formatting

e40e675

typo

85b86c8

gw280 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 Oct 27, 2020

fluttergithubbot merged commit 62d50af into flutter:master Oct 29, 2020

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to 005dec449a43 (13 revisions) flutter/flutter#69271

Closed

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

50a0f57

…ngine#22094)

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to d615a97a143d (16 revisions) flutter/flutter#69318

Closed

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

932e71e

…ngine#22094)

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to 07c780bd9507 (18 revisions) flutter/flutter#69324

Closed

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

c092a76

…ngine#22094)

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to e4dffc107032 (21 revisions) flutter/flutter#69331

Closed

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

332e238

…ngine#22094)

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to 9ccf9f120ddc (26 revisions) flutter/flutter#69335

Closed

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

85b6ae5

…ngine#22094)

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to fe85f946db04 (34 revisions) flutter/flutter#69343

Closed

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

c2e6b7e

…ngine#22094)

GaryQian mentioned this pull request Oct 29, 2020

Roll engine from 073263e39d1d to fe85f946db04 (34 revisions) manual flutter/flutter#69347

Closed

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to fe85f946db04 (34 revisions) flutter/flutter#69349

Closed

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

4eaf9d3

…ngine#22094)

engine-flutter-autoroll mentioned this pull request Oct 30, 2020

Roll Engine from 073263e39d1d to 99cc50dfffab (35 revisions) flutter/flutter#69353

Merged

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

dca818f

…ngine#22094)

engine-flutter-autoroll mentioned this pull request Oct 30, 2020

Roll Engine from 073263e39d1d to 7d6180c3014a (37 revisions) flutter/flutter#69357

Closed

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

80d380f

…ngine#22094)

engine-flutter-autoroll mentioned this pull request Oct 30, 2020

Roll Engine from 99cc50dfffab1 to 7d6180c3014a (2 revisions) flutter/flutter#69365

Closed

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

62d50af Add plumbing for command line arguments on Windows (flutter/e…

3ace6dd

…ngine#22094)

stuartmorgan mentioned this pull request Nov 7, 2020

flutter run fails on Windows 10 (flutter desktop) flutter/flutter#69872

Closed

gw280 added affects: desktop needs tests platform-windows labels Nov 23, 2020

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Add plumbing for command line arguments on Windows (flutter#22094)

375c414

gw280 mentioned this pull request Jul 16, 2021

Add unit tests for Dart entrypoint arguments on Windows #27497

Merged

8 tasks

gw280 removed the needs tests label Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plumbing for command line arguments on Windows #22094

Add plumbing for command line arguments on Windows #22094

gw280 commented Oct 24, 2020

flutter-dashboard bot commented Oct 24, 2020

stuartmorgan left a comment

gw280 Oct 27, 2020

gw280 Oct 27, 2020 •

edited

Loading

stuartmorgan left a comment

Add plumbing for command line arguments on Windows #22094

Add plumbing for command line arguments on Windows #22094

Conversation

gw280 commented Oct 24, 2020

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

flutter-dashboard bot commented Oct 24, 2020

stuartmorgan left a comment

Choose a reason for hiding this comment

gw280 Oct 27, 2020

Choose a reason for hiding this comment

gw280 Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

gw280 Oct 27, 2020 •

edited

Loading