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

Fire PlatformViewController FlutterView callbacks #13015

Merged
mklim merged 2 commits into from
Oct 14, 2019
Merged

Fire PlatformViewController FlutterView callbacks #13015

mklim merged 2 commits into from
Oct 14, 2019

Conversation

@mklim
Copy link
Contributor

@mklim mklim commented Oct 9, 2019 •
edited
Loading

Fixes a bug where PlatformViewController was not being notified of FlutterView attachment changes.

Related to flutter/flutter#41851.

@mklim
Copy link
Contributor Author

mklim commented Oct 9, 2019

@amirh this still needs a test before it can land but if you want to review the basic approach in the meantime feel free.

@mklim mklim mentioned this pull request Oct 9, 2019
Merged
13 tasks
@mklim mklim requested review from amirh and dnfield October 9, 2019 20:35
@mklim
Copy link
Contributor Author

mklim commented Oct 9, 2019

This is ready for review. The unit test I added is not good, it's just an interaction test. To really test the behavior I would have need to have significantly refactored the PlatformViewsController class and then changed FlutterEngine to use constructor injection. That may be worth it, but I wasn't sure whether it was good to make this PR any riskier.

dnfield
dnfield reviewed Oct 9, 2019
View reviewed changes
shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java
// TODO(mattcarroll): add javadocs
@UiThread
public static native boolean nativeGetIsSoftwareRenderingEnabled();
public native boolean nativeGetIsSoftwareRenderingEnabled();
Copy link
Contributor

@dnfield dnfield Oct 9, 2019

Choose a reason for hiding this comment

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

Was this meant to be part of this change?

Copy link
Contributor

@dnfield dnfield Oct 9, 2019

Choose a reason for hiding this comment

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

I see now.

This seems to indicate that most of these methods shouldn't be static then, right?

dnfield
dnfield approved these changes Oct 9, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

This LGTM, but would defer to @amirh

I suspect that many of the methods on FlutterJNI should be made instance methods similar to this one, except for ones that we know will always be global (like the observatory uri).

@mklim
Copy link
Contributor Author

mklim commented Oct 9, 2019

I suspect that many of the methods on FlutterJNI should be made instance methods similar to this one, except for ones that we know will always be global (like the observatory uri).

I agree. I conservatively just instanced the one thing I needed to for this test case but I think it would make sense to move all of them. These are really depending on some mutable state (the native library being loaded) so I don't think they're really immutable/"static" to begin with.

Michael Klimushyn added 2 commits October 11, 2019 17:12
Fire PlatformViewController FlutterView callbacks
01968e6 
Fixes a bug where `PlatformViewController` was not being notified of
`FlutterView` attachment changes.

Unfortunately the test here is just an interaction test. I refactored
some of the underlying code so that FlutterEngine could be initialized
without through missing native library errors, but even with that
variance there isn't any obvious way to test the real behavior here. I'd
need to significantly refactor FlutterEngine and PlatformViewsController
to write a more effective test here.
Review feedback
5ad6337
amirh
amirh approved these changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

LGTM

@mklim mklim merged commit 531a9cf into flutter:master Oct 14, 2019
@mklim mklim deleted the call_platform_view_attach branch October 14, 2019 21:03
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 14, 2019
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 14, 2019
@engine-flutter-autoroll
531a9cf Fire PlatformViewController FlutterView callbacks (flutter/en…
0a1ec44 
…gine#13015)
mklim pushed a commit to flutter/plugins that referenced this pull request Oct 15, 2019
[flutter_webview] Migrate to the new embedding (#2169)
573c9ff 
This depends on a new bugfix on the engine for text input to work with
the new embedding (flutter/engine#13015).
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Oct 15, 2019
@engine-flutter-autoroll
Roll engine eed171f..66bf00b (44 commits) (#42695)
1d4d63a 
[email protected]:flutter/engine.git/compare/eed171ff3538...66bf00b

git log eed171f..66bf00b --no-merges --oneline
2019-10-14 [email protected] refactoring chrome_installer (flutter/engine#13122)
2019-10-14 [email protected] Fire PlatformViewController FlutterView callbacks (flutter/engine#13015)
2019-10-14 [email protected] iOS Platform View: Fixed overrelease of the observer. (flutter/engine#13093)
2019-10-14 [email protected] [web] Add basic color per vertex drawVertices API support (flutter/engine#13066)
2019-10-14 [email protected] Support keyboard types on mobile browsers (flutter/engine#13044)
2019-10-14 [email protected] Change IO thread shader cache strategy (flutter/engine#13121)
2019-10-14 [email protected] Roll fuchsia/sdk/core/linux-amd64 from _GTls... to xRgq0... (flutter/engine#13115)
2019-10-14 [email protected] Roll src/third_party/skia 3838fe3c82b4..a7e1b45d9c28 (1 commits) (flutter/engine#13117)
2019-10-14 [email protected] Roll fuchsia/sdk/core/mac-amd64 from E3s7G... to Lk7iT... (flutter/engine#13116)
2019-10-14 [email protected] Roll src/third_party/dart 5fad012d02..892fcf2c45 (15 commits)
2019-10-14 [email protected] Integrate more SkParagraph builder patches (flutter/engine#13094)
2019-10-13 [email protected] Roll fuchsia/sdk/core/mac-amd64 from T3Xkz... to E3s7G... (flutter/engine#13114)
2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from lJPDX... to _GTls... (flutter/engine#13113)
2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from W9PBe... to T3Xkz... (flutter/engine#13112)
2019-10-12 [email protected] Roll fuchsia/clang/linux-amd64 from q4DVY... to 2EAvs... (flutter/engine#13111)
2019-10-12 [email protected] Roll fuchsia/clang/mac-amd64 from zpVtV... to xTJWs... (flutter/engine#13110)
2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from LsxeL... to lJPDX... (flutter/engine#13109)
2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from ShVbh... to W9PBe... (flutter/engine#13108)
2019-10-12 [email protected] Roll src/third_party/dart 90ff37e011..5fad012d02 (6 commits)
2019-10-12 [email protected] Roll src/third_party/dart 42dcdf903c..90ff37e011 (7 commits)
2019-10-12 [email protected] Roll src/third_party/skia 8c6c8b0c42e2..3838fe3c82b4 (3 commits) (flutter/engine#13105)
2019-10-12 [email protected] Analyze framework Dart code in presubmit tests (flutter/engine#13037)
2019-10-12 [email protected] [dart_aot_runner] Complete the port of dart_aot_runner (flutter/engine#13103)
2019-10-11 [email protected] Move initialization into FlutterEngine (flutter/engine#12806)
2019-10-11 [email protected] Update felt README (flutter/engine#13097)
2019-10-11 [email protected] Roll src/third_party/dart 9f33e8da04..42dcdf903c (7 commits)
2019-10-11 [email protected] [dart_aot_runner] Generate vmservice aotsnapshots (flutter/engine#13101)
2019-10-11 [email protected] ColorFilter matrix docs (flutter/engine#13100)
2019-10-11 [email protected] cleanup gen_package.py (flutter/engine#13089)
2019-10-11 [email protected] [dart_aot_runner] Use the host_toolchain to build kernels (flutter/engine#13096)
2019-10-11 [email protected] do not wrap font family name (flutter/engine#12801)
2019-10-11 [email protected] Roll src/third_party/skia df640e6d14a5..8c6c8b0c42e2 (12 commits) (flutter/engine#13098)
2019-10-11 [email protected] Remove persistent cache unittest timeout (flutter/engine#13091)
2019-10-11 [email protected] Added FlutterActivity and FlutterFragment hook to cleanUpFlutterEngine() as symmetry for configureFlutterEngine(). (#41943) (flutter/engine#12987)
2019-10-11 [email protected] use rest args for specifying test targets (flutter/engine#13088)
2019-10-11 [email protected] Snapshot the felt tool for faster start-up (flutter/engine#13090)
2019-10-11 [email protected] Roll src/third_party/dart 965b8cb1d8..9f33e8da04 (15 commits)
2019-10-11 [email protected] java imports/style (flutter/engine#13082)
2019-10-11 [email protected] Removed retain cycle from notification center. (flutter/engine#13073)
2019-10-11 [email protected] Roll fuchsia/sdk/core/mac-amd64 from U4IBp... to ShVbh... (flutter/engine#13092)
2019-10-11 [email protected] Roll fuchsia/sdk/core/linux-amd64 from fk6ou... to LsxeL... (flutter/engine#13083)
2019-10-11 [email protected] Roll src/third_party/skia 6c2b2bb02402..df640e6d14a5 (1 commits) (flutter/engine#13080)
2019-10-11 [email protected] Gen package output corrected (flutter/engine#13086)
2019-10-11 [email protected] Print more output when gen_package fails (flutter/engine#13085)

...
@mklim mklim mentioned this pull request Oct 15, 2019
Merged
13 tasks
mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
@mormih
[flutter_webview] Migrate to the new embedding (flutter#2169)
01cfa4f 
This depends on a new bugfix on the engine for text input to work with
the new embedding (flutter/engine#13015).
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine eed171f..66bf00b (44 commits) (flutter#42695)
f7381cc 
[email protected]:flutter/engine.git/compare/eed171ff3538...66bf00b

git log eed171f..66bf00b --no-merges --oneline
2019-10-14 [email protected] refactoring chrome_installer (flutter/engine#13122)
2019-10-14 [email protected] Fire PlatformViewController FlutterView callbacks (flutter/engine#13015)
2019-10-14 [email protected] iOS Platform View: Fixed overrelease of the observer. (flutter/engine#13093)
2019-10-14 [email protected] [web] Add basic color per vertex drawVertices API support (flutter/engine#13066)
2019-10-14 [email protected] Support keyboard types on mobile browsers (flutter/engine#13044)
2019-10-14 [email protected] Change IO thread shader cache strategy (flutter/engine#13121)
2019-10-14 [email protected] Roll fuchsia/sdk/core/linux-amd64 from _GTls... to xRgq0... (flutter/engine#13115)
2019-10-14 [email protected] Roll src/third_party/skia 3838fe3c82b4..a7e1b45d9c28 (1 commits) (flutter/engine#13117)
2019-10-14 [email protected] Roll fuchsia/sdk/core/mac-amd64 from E3s7G... to Lk7iT... (flutter/engine#13116)
2019-10-14 [email protected] Roll src/third_party/dart 5fad012d02..892fcf2c45 (15 commits)
2019-10-14 [email protected] Integrate more SkParagraph builder patches (flutter/engine#13094)
2019-10-13 [email protected] Roll fuchsia/sdk/core/mac-amd64 from T3Xkz... to E3s7G... (flutter/engine#13114)
2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from lJPDX... to _GTls... (flutter/engine#13113)
2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from W9PBe... to T3Xkz... (flutter/engine#13112)
2019-10-12 [email protected] Roll fuchsia/clang/linux-amd64 from q4DVY... to 2EAvs... (flutter/engine#13111)
2019-10-12 [email protected] Roll fuchsia/clang/mac-amd64 from zpVtV... to xTJWs... (flutter/engine#13110)
2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from LsxeL... to lJPDX... (flutter/engine#13109)
2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from ShVbh... to W9PBe... (flutter/engine#13108)
2019-10-12 [email protected] Roll src/third_party/dart 90ff37e011..5fad012d02 (6 commits)
2019-10-12 [email protected] Roll src/third_party/dart 42dcdf903c..90ff37e011 (7 commits)
2019-10-12 [email protected] Roll src/third_party/skia 8c6c8b0c42e2..3838fe3c82b4 (3 commits) (flutter/engine#13105)
2019-10-12 [email protected] Analyze framework Dart code in presubmit tests (flutter/engine#13037)
2019-10-12 [email protected] [dart_aot_runner] Complete the port of dart_aot_runner (flutter/engine#13103)
2019-10-11 [email protected] Move initialization into FlutterEngine (flutter/engine#12806)
2019-10-11 [email protected] Update felt README (flutter/engine#13097)
2019-10-11 [email protected] Roll src/third_party/dart 9f33e8da04..42dcdf903c (7 commits)
2019-10-11 [email protected] [dart_aot_runner] Generate vmservice aotsnapshots (flutter/engine#13101)
2019-10-11 [email protected] ColorFilter matrix docs (flutter/engine#13100)
2019-10-11 [email protected] cleanup gen_package.py (flutter/engine#13089)
2019-10-11 [email protected] [dart_aot_runner] Use the host_toolchain to build kernels (flutter/engine#13096)
2019-10-11 [email protected] do not wrap font family name (flutter/engine#12801)
2019-10-11 [email protected] Roll src/third_party/skia df640e6d14a5..8c6c8b0c42e2 (12 commits) (flutter/engine#13098)
2019-10-11 [email protected] Remove persistent cache unittest timeout (flutter/engine#13091)
2019-10-11 [email protected] Added FlutterActivity and FlutterFragment hook to cleanUpFlutterEngine() as symmetry for configureFlutterEngine(). (flutter#41943) (flutter/engine#12987)
2019-10-11 [email protected] use rest args for specifying test targets (flutter/engine#13088)
2019-10-11 [email protected] Snapshot the felt tool for faster start-up (flutter/engine#13090)
2019-10-11 [email protected] Roll src/third_party/dart 965b8cb1d8..9f33e8da04 (15 commits)
2019-10-11 [email protected] java imports/style (flutter/engine#13082)
2019-10-11 [email protected] Removed retain cycle from notification center. (flutter/engine#13073)
2019-10-11 [email protected] Roll fuchsia/sdk/core/mac-amd64 from U4IBp... to ShVbh... (flutter/engine#13092)
2019-10-11 [email protected] Roll fuchsia/sdk/core/linux-amd64 from fk6ou... to LsxeL... (flutter/engine#13083)
2019-10-11 [email protected] Roll src/third_party/skia 6c2b2bb02402..df640e6d14a5 (1 commits) (flutter/engine#13080)
2019-10-11 [email protected] Gen package output corrected (flutter/engine#13086)
2019-10-11 [email protected] Print more output when gen_package fails (flutter/engine#13085)

...
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
[flutter_webview] Migrate to the new embedding (flutter#2169)
6f78be0 
This depends on a new bugfix on the engine for text input to work with
the new embedding (flutter/engine#13015).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@amirh amirh amirh approved these changes

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mklim @amirh @dnfield @googlebot