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

Conversation

@cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Jun 23, 2020

Description

Make the raster task runner always run on the platform thread when user enables 'io.flutter.embedded_views_preview` in Manifest.xml

Related Issues

flutter/flutter#60016

Tests

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.

Breaking Change

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

Chris Yang added 3 commits June 22, 2020 09:25
@cyanglaz cyanglaz force-pushed the android_platform_view_static_thread_merging branch from 91de367 to 86967e6 Compare June 23, 2020 17:41
result.engineCachesPath,
initTimeMillis);
initTimeMillis,
use_embedded_view);
Copy link

Choose a reason for hiding this comment

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

this also needs a change in FlutterJNI#nativeInit signature


FlutterJNI.nativeInit(
applicationContext,
shellArgs.toArray(new String[0]),
Copy link

Choose a reason for hiding this comment

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

nit: what about passing the flag in shellArgs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@cyanglaz cyanglaz marked this pull request as ready for review June 23, 2020 21:54
@fluttergithubbot
Copy link
Contributor

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.

@auto-assign auto-assign bot requested a review from jason-simmons June 23, 2020 21:55
/// to log a timeline event that tracks the latency of engine startup.
std::chrono::microseconds engine_start_timestamp = {};

/// Does the application claim that it uses the android embedded view for platform views.
Copy link

Choose a reason for hiding this comment

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

nit: is this missing a question mark?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to "Whether"

// https://github.com/flutter/flutter/issues/59930
flutter::TaskRunners task_runners(thread_label, // label
platform_runner, // platform
platform_runner, // raster
Copy link

Choose a reason for hiding this comment

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

nit: formatting

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM

@blasten
Copy link

blasten commented Jun 24, 2020

Until flutter/flutter#55326 is fixed, tests will be added to the framework where we can run e2e tests.

Copy link
Contributor Author

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Updated with review comments

/// to log a timeline event that tracks the latency of engine startup.
std::chrono::microseconds engine_start_timestamp = {};

/// Does the application claim that it uses the android embedded view for platform views.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to "Whether"


FlutterJNI.nativeInit(
applicationContext,
shellArgs.toArray(new String[0]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (use_embedded_view) {
shellArgs.add("--use-embedded-view");
if (bundle != null) {
boolean use_embedded_view = bundle.getBoolean("io.flutter.embedded_views_preview");
Copy link

Choose a reason for hiding this comment

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

minor nit: io.flutter.embeddedViewsPreview

Copy link
Contributor Author

Choose a reason for hiding this comment

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

offline discussion: We will keep the flag name same as iOS.

@cyanglaz cyanglaz 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 Jun 24, 2020
@fluttergithubbot fluttergithubbot merged commit a11c398 into flutter:master Jun 24, 2020
zhongwuzw pushed a commit to zhongwuzw/engine that referenced this pull request Jun 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-android platform-ios waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants