Add shell api to set default for windows data #14002

chunhtai · 2019-11-25T17:04:17Z

This pr enables embedding to set default values for window data.

It does following things

Make WindowData a public class in runtime controller
Add windowData a construction parameter for RuntimeController, Engine, and Shell.
If there is no windowData passed in to Shell:Create, it will use the default version of WindowData.
4, Makes Android and IOS pass in windowData with their own defaults

chunhtai · 2019-11-25T23:07:34Z

runtime/runtime_controller.cc


 namespace flutter {

-RuntimeController::RuntimeController(


RuntimeController constructor will always need window data.

chunhtai · 2019-11-26T17:57:38Z

cc @xster

chinmaygarde · 2019-11-26T18:25:35Z

Can you elaborate on the use case? The default frame size is zero and a frame wont get rendered by the rasterizer at that size. So the first frame that ends up on screen will already be the result of the setter to the viewport metrics.

chunhtai · 2019-11-26T19:07:55Z

Can you elaborate on the use case? The default frame size is zero and a frame wont get rendered by the rasterizer at that size. So the first frame that ends up on screen will already be the result of the setter to the viewport metrics.

The windows data here is the default value, those are to be set before user setting and viewport metric is flushed to windows.dart. In the most case, we don't care about the default value because those setting will be flushed during viewcontroller/activity creating. In the add to app scenario, the view controller or activity may not exist when the framework app is running, and the app is running with the default window data. For now, this pr only set applifecycle default value to be detached for android and ios. This pr will open a lot of opportunities to set those default so that the app can warm up in a more reasonable way in the case of missing viewcontroller or activity.

chunhtai · 2019-11-26T19:12:23Z

This is also a refactoring. The existing test should cover it. Except in the case of non ios and android platform it uses the 'default' window data in runtime_controller.h. I do not know how to test it. I would imagine the test would be using shell:create to create the shell and verify window.dart has the correct value. I don't know if this test is possible. @chinmaygarde do you have any suggestion?

chunhtai · 2019-12-02T23:52:52Z

This requires some more love

cbracken · 2019-12-16T18:53:24Z

@chinmaygarde this is ready for re-review.

xster · 2019-12-19T00:18:53Z

Also add @gaaclarke

gaaclarke

I would like to see the change that fixes the issue in the PR as well.

runtime/runtime_controller.h

shell/common/engine.h

shell/common/shell.h

shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm

runtime/runtime_controller.h

shell/platform/android/android_shell_holder.cc

shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm

chunhtai · 2019-12-21T00:29:16Z

I would like to see the change that fixes the issue in the PR as well.

Previously, it will set the lifecylce enum to be detached by default for all platforms, and expects the platform to update the lifecycle enum when they are ready.
This pr fixes this issue by setting the default to detached for ios and android where both platform will update the enum, and set the default to null for all the other platforms who currently do not opt in to handle lifecycle. I provide the api for each platform to set the default once they opt in to handle lifecycle enum.

I also addressed all comments, this is ready for review

gaaclarke

LGTM with a few nits. Sorry about the delay in the review, I was OOO.

gaaclarke · 2020-01-06T16:27:09Z

runtime/runtime_controller.cc

    std::string p_advisory_script_entrypoint,
    const std::function<void(int64_t)>& idle_notification_callback,
-    WindowData p_window_data,
+    const WindowData p_window_data,


Ideally this should be a const ref:
const WindowData& p_window_data

gaaclarke · 2020-01-06T16:27:26Z

runtime/runtime_controller.h

      std::string advisory_script_uri,
      std::string advisory_script_entrypoint,
      const std::function<void(int64_t)>& idle_notification_callback,
+      const WindowData data,


Const ref here too.

gaaclarke · 2020-01-06T16:28:50Z

runtime/window_data.h

+/// The struct of platform-specific data used for initializing ui.Window.
+///
+/// framework may request data from ui.Window before platform is properly
+/// configured. Uses this struct to set the desired default value for ui.Window


Who "uses this struct"? I'd mention Engine and add a link to it.

cbracken · 2020-01-09T02:10:59Z

runtime/window_data.h

+///
+///  * flutter::Shell::Create, which takes a window_data to initialize the
+///  ui.Window
+///    attached to it.


nit: needs re-formatting.

cbracken · 2020-01-09T02:13:39Z

shell/common/shell.h

+  ///             Unlike the simpler variant of this factory method, this method
+  ///             allows for specification of window data. If this is the first
+  ///             instance of a shell in the process, this
+  ///             call also bootstraps the Dart VM.


nit: line wrapping.

cbracken · 2020-01-09T02:17:45Z

shell/platform/android/android_shell_holder.cc

  FML_CHECK(pthread_key_delete(thread_destruct_key_) == 0);
 }

+WindowData AndroidShellHolder::getDefaultWindowData() {


Given that this doesn't rely on object state at all, this could be a static function local to the TU.

(Either way this function should start with a capital though)

On third thought, if you don't expect this to change significantly in the future or be called from a ton of places, I'd probably just inline the variable declaration and field initialisation at the usage site above (and similar for the obj-c version).

I separated this out to a method for readability. We might want to fill in more default value for add 2 app, for example, viewport metrics.

cbracken

lgtm modulo nits

…#14002)

This reverts commit cef181f.

chunhtai added the Work in progress (WIP) Not ready (yet) for review! label Nov 25, 2019

auto-assign bot requested a review from flar November 25, 2019 17:04

googlebot added the cla: yes label Nov 25, 2019

chunhtai force-pushed the issues/45270 branch 2 times, most recently from 75a4b80 to 1f3b56f Compare November 25, 2019 18:03

chunhtai commented Nov 25, 2019

View reviewed changes

chunhtai force-pushed the issues/45270 branch from 2190ab4 to 4aad044 Compare November 26, 2019 02:21

chunhtai requested a review from chinmaygarde November 26, 2019 17:28

chunhtai removed the Work in progress (WIP) Not ready (yet) for review! label Dec 2, 2019

xster requested a review from gaaclarke December 19, 2019 00:18

gaaclarke reviewed Dec 19, 2019

View reviewed changes

chunhtai force-pushed the issues/45270 branch 2 times, most recently from 5fd6af4 to 17c073d Compare December 21, 2019 00:18

gaaclarke approved these changes Jan 6, 2020

View reviewed changes

This was referenced Jan 6, 2020

Define a pattern for runApp when the window may have no size flutter/flutter#32871

Closed

Add2App cannot go to non-initial routes when using cached flutter engine flutter/flutter#31185

Closed

chunhtai force-pushed the issues/45270 branch from 17c073d to 38cb0ac Compare January 9, 2020 00:31

chunhtai added 5 commits January 8, 2020 18:04

Add shell api to set default for windows data

7daabf4

update

552403c

fix build

fb5d3e6

update

d022162

update

636aa1f

chunhtai force-pushed the issues/45270 branch from cd5ad83 to 366e45d Compare January 9, 2020 02:05

cbracken reviewed Jan 9, 2020

View reviewed changes

reformat

4d36fc0

cbracken reviewed Jan 9, 2020

View reviewed changes

addressing comment

641c6f0

chunhtai merged commit 7a27e75 into flutter:master Jan 9, 2020

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48587

Closed

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

7a27e75 Add shell api to set default for windows data (flutter/engine…

4f0b71c

…#14002)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48588

Closed

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

7a27e75 Add shell api to set default for windows data (flutter/engine…

4f4e1f4

…#14002)

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

7a27e75 Add shell api to set default for windows data (flutter/engine…

58f7b11

…#14002)

This was referenced Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48590

Closed

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48592

Closed

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

7a27e75 Add shell api to set default for windows data (flutter/engine…

836a604

…#14002)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48593

Closed

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

7a27e75 Add shell api to set default for windows data (flutter/engine…

49937f9

…#14002)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48594

Closed

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

7a27e75 Add shell api to set default for windows data (flutter/engine…

d249f2c

…#14002)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48595

Closed

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

7a27e75 Add shell api to set default for windows data (flutter/engine…

0ab380d

…#14002)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48596

Closed

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

7a27e75 Add shell api to set default for windows data (flutter/engine…

9dc4a3a

…#14002)

GaryQian mentioned this pull request Jan 10, 2020

Manually Roll engine to f001ea2...7ef88f8 (34 commits) to resolve goldens flutter/flutter#48598

Merged

engine-flutter-autoroll mentioned this pull request Jan 11, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48615

Closed

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

7a27e75 Add shell api to set default for windows data (flutter/engine…

8ff9ccb

…#14002)

NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Add shell api to set default for windows data (flutter#14002)

cef181f

NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Revert "Add shell api to set default for windows data (flutter#14002)"

c13bfd4

This reverts commit cef181f.

filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020

Add shell api to set default for windows data (flutter#14002)

c4b7e40

Add shell api to set default for windows data #14002

Add shell api to set default for windows data #14002

Uh oh!

Conversation

chunhtai commented Nov 25, 2019 • edited by gaaclarke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Nov 26, 2019

Uh oh!

chinmaygarde commented Nov 26, 2019

Uh oh!

chunhtai commented Nov 26, 2019

Uh oh!

chunhtai commented Nov 26, 2019

Uh oh!

chunhtai commented Dec 2, 2019

Uh oh!

cbracken commented Dec 16, 2019

Uh oh!

xster commented Dec 19, 2019

Uh oh!

gaaclarke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

chunhtai commented Dec 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke left a comment

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!

Choose a reason for hiding this comment

Uh oh!

cbracken Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chunhtai commented Nov 25, 2019 •

edited by gaaclarke

Loading

gaaclarke left a comment •

edited

Loading

chunhtai commented Dec 21, 2019 •

edited

Loading

cbracken Jan 9, 2020 •

edited

Loading