[fuchsia] Expose view_ref as part of dart:fuchsia initialization #15958

iskakaushik · 2020-01-24T02:27:17Z

iskakaushik · 2020-01-24T02:33:00Z

@chinmaygarde , @arbreng I will add tests for these, wanted to give you a heads-up on the approach i'm taking.

arbreng

What happened to exposing through the Window? Window definitely seems to be the most appropriate place according to: https://github.com/flutter/engine/blob/master/lib/ui/window.dart#L518

This would also avoid the issue you have where pure dart apps have to use a bogus/null ViewRef

arbreng · 2020-01-24T03:04:17Z

shell/platform/fuchsia/dart-pkg/fuchsia/lib/fuchsia.dart

    return handle;
  }
+
+  static Handle takeViewRef() {


Add // TODO: refactor Handle to an EventPair

Also, is there any issue open for those TODOs?

I don't think there is, I will create one.

arbreng · 2020-01-24T03:06:06Z

shell/platform/fuchsia/dart-pkg/fuchsia/sdk_ext/fuchsia.cc

 void Initialize(fidl::InterfaceHandle<fuchsia::sys::Environment> environment,
-                zx::channel directory_request) {
+                zx::channel directory_request,
+                zx::eventpair view_ref) {


Can we make this view_ref optional? Then you can set it to null in dart and avoid the dummy_view_ref below

arbreng · 2020-01-24T03:08:06Z

shell/platform/fuchsia/flutter/isolate_configurator.h

      fidl::InterfaceHandle<fuchsia::sys::Environment> environment,
-      zx::channel directory_request);
+      zx::channel directory_request,
+      zx::eventpair view_ref);


Same here, I would make the view_ref optional

Good call, I will make the optional change.

iskakaushik

What happened to exposing through the Window? Window definitely seems to be the most appropriate place according to: https://github.com/flutter/engine/blob/master/lib/ui/window.dart#L518

This would also avoid the issue you have where pure dart apps have to use a bogus/null ViewRef

As we discussed, I was considering exposing these on window. window would've been the perfect place if we decided to expose some kind of view_id across all platforms. Given that we have this use-case only on Fuchsia currently, I'm inclined to expose this on fuchsia.dart as a way to incubate this api. If at some point we decide that this is universal across all platforms, we can move it to window.

gw280 · 2020-01-24T19:23:24Z

LGTM modulo David's comments

arbreng

LGTM

iskakaushik · 2020-01-24T21:41:56Z

Landing this with the intent of adding tests once we have the test framework ready. Filed: flutter/flutter#49447

…ion (flutter/engine#15958)

flutter/engine@51a7964...4218f80 git log 51a7964..4218f80 --first-parent --oneline 2020-01-25 [email protected] Roll fuchsia/sdk/core/mac-amd64 from 7fqYj... to 35pbn... (flutter/engine#15984) 2020-01-25 [email protected] updating the versions of the browsers for flutter web engine unit tests (flutter/engine#15977) 2020-01-25 [email protected] Eliminate unused import in Android embedding (flutter/engine#15975) 2020-01-24 [email protected] Prevent duplicate plugin registration in FlutterEnginePluginRegistry. (#49365) (flutter/engine#15956) 2020-01-24 [email protected] retry logic for another cipd upload (flutter/engine#15974) 2020-01-24 [email protected] Ensure GetFixturesPath works on Fuchsia (flutter/engine#15978) 2020-01-24 [email protected] [web] Calling platform message callback after copy (flutter/engine#15950) 2020-01-24 [email protected] [fuchsia] Expose view_ref as part of dart:fuchsia initialization (flutter/engine#15958) 2020-01-24 [email protected] Refactor ShellTest to allow for different ShellTestPlatformViews (flutter/engine#15972) 2020-01-24 [email protected] Cache computed window.physicalSize in a FrameReference (flutter/engine#15955) 2020-01-24 [email protected] the the fix (flutter/engine#15973) 2020-01-24 [email protected] Optimize drawRRect to use dom_canvas (flutter/engine#15970) 2020-01-24 [email protected] Remove paint apply in draw image (flutter/engine#15969) 2020-01-24 [email protected] Roll fuchsia/sdk/core/mac-amd64 from 6_pZp... to 7fqYj... (flutter/engine#15966) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

…tter#15958) fixes flutter/flutter#47178

…ion (flutter#15958)" This reverts commit 5d7c2a9.

auto-assign bot requested a review from gaaclarke January 24, 2020 02:27

googlebot added the cla: yes label Jan 24, 2020

iskakaushik requested review from chinmaygarde and removed request for gaaclarke January 24, 2020 02:27

[fuchsia] Expose view_ref as part of dart:fuchsia initialization

fa4b341

iskakaushik force-pushed the expose_viewref branch from 7970bf8 to fa4b341 Compare January 24, 2020 02:28

iskakaushik requested a review from arbreng January 24, 2020 02:32

iskakaushik requested a review from gw280 January 24, 2020 02:35

arbreng reviewed Jan 24, 2020

View reviewed changes

iskakaushik commented Jan 24, 2020

View reviewed changes

arbreng approved these changes Jan 24, 2020

View reviewed changes

Use std::optional and file an issue for TODOs in fuchsia.dart

3bd932d

iskakaushik merged commit bd8c955 into flutter:master Jan 24, 2020

iskakaushik deleted the expose_viewref branch January 24, 2020 21:42

iskakaushik mentioned this pull request Jan 24, 2020

Access to own ViewRef for Flutter on Fuschia flutter/flutter#48969

Closed

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

Roll engine 51a79649662f..9ee917c26dfc (10 commits) flutter/flutter#49467

Closed

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

bd8c955 [fuchsia] Expose view_ref as part of dart:fuchsia initializat…

03ac0e9

…ion (flutter/engine#15958)

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

bd8c955 [fuchsia] Expose view_ref as part of dart:fuchsia initializat…

87b2861

…ion (flutter/engine#15958)

This was referenced Jan 25, 2020

Roll engine 51a79649662f..2415e9654b2a (13 commits) flutter/flutter#49468

Closed

Roll engine 51a79649662f..4218f8026f29 (14 commits) flutter/flutter#49469

Closed

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

bd8c955 [fuchsia] Expose view_ref as part of dart:fuchsia initializat…

74ace16

…ion (flutter/engine#15958)

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

bd8c955 [fuchsia] Expose view_ref as part of dart:fuchsia initializat…

dc72c15

…ion (flutter/engine#15958)

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

Roll engine 51a79649662f..4218f8026f29 (14 commits) flutter/flutter#49470

Merged

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

[fuchsia] Expose view_ref as part of dart:fuchsia initialization (flu…

5d7c2a9

…tter#15958) fixes flutter/flutter#47178

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

Revert "[fuchsia] Expose view_ref as part of dart:fuchsia initializat…

3ae70af

…ion (flutter#15958)" This reverts commit 5d7c2a9.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[fuchsia] Expose view_ref as part of dart:fuchsia initialization #15958

[fuchsia] Expose view_ref as part of dart:fuchsia initialization #15958

Uh oh!

iskakaushik commented Jan 24, 2020 •

edited

Loading

Uh oh!

iskakaushik commented Jan 24, 2020

Uh oh!

arbreng left a comment

Uh oh!

arbreng Jan 24, 2020

Uh oh!

iskakaushik Jan 24, 2020

Uh oh!

arbreng Jan 24, 2020

Uh oh!

arbreng Jan 24, 2020

Uh oh!

iskakaushik Jan 24, 2020

Uh oh!

iskakaushik left a comment

Uh oh!

gw280 commented Jan 24, 2020

Uh oh!

arbreng left a comment

Uh oh!

iskakaushik commented Jan 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[fuchsia] Expose view_ref as part of dart:fuchsia initialization #15958

[fuchsia] Expose view_ref as part of dart:fuchsia initialization #15958

Uh oh!

Conversation

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

Uh oh!

Uh oh!

iskakaushik commented Jan 24, 2020

Uh oh!

arbreng left a comment

Choose a reason for hiding this comment

Uh oh!

arbreng Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

iskakaushik Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

arbreng Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

arbreng Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

iskakaushik Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

gw280 commented Jan 24, 2020

Uh oh!

arbreng left a comment

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Jan 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iskakaushik commented Jan 24, 2020 •

edited

Loading