Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

fuchsia: create new flutter_runner render path #19584

Merged
merged 4 commits into from
Aug 28, 2020

Conversation

arbreng
Copy link
Contributor

@arbreng arbreng commented Jul 7, 2020

Description

This new render path implements the ExternalViewEmbedder interface without using any legacy code, keeping all Scenic interaction contained to the embedder. I cribbed liberally from EmbedderExternalViewEmbedder when creating this.

Once stabilized, To-be-replaced by moving Fuchsia behind the formal C API in a series of follow-up CLs.

Related Issues

https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=46971

Tests

Ran workstation shell and internal tests w/ both render paths.

@arbreng arbreng requested a review from chinmaygarde July 7, 2020 18:12
@arbreng arbreng marked this pull request as draft July 7, 2020 18:12
@auto-assign auto-assign bot requested a review from gaaclarke July 7, 2020 18:12
@arbreng arbreng force-pushed the flutter-runner-next branch 2 times, most recently from c4a0ac7 to 3094fd2 Compare August 4, 2020 08:24
@arbreng arbreng marked this pull request as ready for review August 4, 2020 17:44
@auto-assign auto-assign bot requested a review from gw280 August 4, 2020 17:46
@arbreng arbreng force-pushed the flutter-runner-next branch 2 times, most recently from 2bc3b1e to 41cf2fa Compare August 6, 2020 21:36
@arbreng arbreng force-pushed the flutter-runner-next branch from 41cf2fa to 7d088fd Compare August 16, 2020 00:32
@arbreng arbreng changed the title fuchsia: Create flutter_runner_next and tests Create new flutter_runner render path Aug 16, 2020
@arbreng arbreng requested a review from dreveman August 16, 2020 00:32
@arbreng arbreng force-pushed the flutter-runner-next branch 2 times, most recently from b48198e to 66c9c08 Compare August 17, 2020 19:49
@arbreng arbreng force-pushed the flutter-runner-next branch 2 times, most recently from 1a8aadd to 5adcb37 Compare August 17, 2020 23:26
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

I gave this a glance. I'm fine with it if everyone else is. I just had some style nits for the most part, maths seems fine. The thing that bothers me about it, if I may, is that it has placed a lot of conditional compilation all over the place. Which is especially bad because it is supporting legacy features and presumably will be removed at some point?

Alternatively a bit more care could be taken to reduce the number of #ifs. For example there could be 2 different SceneUpdateContext's:

static SceneUpdateContext* SceneUpdateContext::Create() {
#if defined(LEGACY_FUCHSIA_EMBEDDER)
  return SceneUpdateContextLegacy();
#else
  return SceneUpdateContextStandard();
#endif
}

This setup has the benefit of compiling the legacy code to make sure it doesn't atrophy. Keeps the standard implementation easier to read and makes the removal of the legacy code trivial. Also for other places where the #ifs are happening, if you wrap up the code in functions, the contract of the code is explicit which makes it easier to rip out or reason about. That's my take at least.

=)

@arbreng
Copy link
Contributor Author

arbreng commented Aug 25, 2020

This setup has the benefit of compiling the legacy code to make sure it doesn't atrophy. Keeps the standard implementation easier to read and makes the removal of the legacy code trivial. Also for other places where the #ifs are happening, if you wrap up the code in functions, the contract of the code is explicit which makes it easier to rip out or reason about. That's my take at least.

=)

Newest version almost does this, although it's not quite as clean as what you suggested here. The complexity is contained to engine.cc though (which is the Fuchsia "embedder" and will be mutating rapidly).

@arbreng arbreng force-pushed the flutter-runner-next branch 2 times, most recently from 7ef99b4 to 6905ddf Compare August 25, 2020 06:58
@arbreng
Copy link
Contributor Author

arbreng commented Aug 25, 2020

If this CL lands, it puts Fuchsia on parity with iOS/Android in terms of external view embedding!

@arbreng arbreng changed the title Create new flutter_runner render path fuchsia: create new flutter_runner render path Aug 25, 2020
Copy link
Contributor

@dreveman dreveman left a comment

Choose a reason for hiding this comment

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

lgtm if gaaclarke is ok with latest version of this change

@arbreng arbreng force-pushed the flutter-runner-next branch 2 times, most recently from bc07866 to 5fed170 Compare August 25, 2020 22:47
Copy link
Contributor

@dreveman dreveman left a comment

Choose a reason for hiding this comment

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

This seems reasonable. We could clean it up more but as LEGACY_FUCHSIA_EMBEDDER is going away asap I think some extra code to keep it less risky for now is good.

@arbreng arbreng force-pushed the flutter-runner-next branch 2 times, most recently from b9bb5ed to 157a6c3 Compare August 26, 2020 06:18
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Wow, I almost don't even recognize it. FuchsiaExternalViewEmbedder worked out great. I think the #ifs are in a better position now. LGTM

@arbreng arbreng force-pushed the flutter-runner-next branch from 157a6c3 to ea552bc Compare August 27, 2020 09:02
@arbreng arbreng force-pushed the flutter-runner-next branch from ea552bc to 21e37e3 Compare August 28, 2020 19:30
@arbreng arbreng merged commit 07e2520 into flutter:master Aug 28, 2020
@arbreng arbreng deleted the flutter-runner-next branch August 28, 2020 21:17
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants