Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Implement Android WebView api with pigeon (Dart portion) #4435

Merged
merged 11 commits into from Oct 26, 2021

Conversation

bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented Oct 18, 2021

Implements the Android WebView library that is currently supported with pigeon.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla google-cla bot added the cla: yes label Oct 18, 2021
@github-actions github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-android labels Oct 18, 2021
@bparrishMines bparrishMines changed the title add dart files Implement Android WebView api with pigeon (Dart portion) Oct 18, 2021
@bparrishMines
Copy link
Contributor Author

@mvanbeusekom @cyanglaz This PR is mostly documentation and code generation. Could you just take a look at:
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart
packages/webview_flutter/webview_flutter_android/lib/src/android_webview_api_impls.dart
packages/webview_flutter/webview_flutter_android/lib/src/instance_manager.dart
packages/webview_flutter/webview_flutter_android/test/android_webview_test.dart
packages/webview_flutter/webview_flutter_android/test/instance_manager_test.dart

@bparrishMines
Copy link
Contributor Author

@gaaclarke Could you take a look at the pigeon api?:
packages/webview_flutter/webview_flutter_android/pigeons/android_webview.dart and
packages/webview_flutter/webview_flutter_android/generatePigeons.sh

Copy link
Contributor

@mvanbeusekom mvanbeusekom left a comment

Choose a reason for hiding this comment

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

Looks good overall, however I have one remark/ question:

I notice that it is common to put multiple classes into the same file. Personally I always like to split out classes into their own files as much as possible. Keeping multiple classes in the same file makes it more difficult to oversee the whole solution and to find the code I need (maybe this is personal and I should just learn to live with it). Another reason is that it is much harder to enforce certain design principles. In Dart it is perfectly legal to access private class members when both classes are in the same file. This would allow people to call into classes directly where they are not supposed to, creating dependency violations (a good example in my opinion would be the circular dependency between the WebView class and the WebSettings class).

What is the policy within the Flutter team regarding this? I see this happening quite often, although not everywhere (google_maps_flutter_platform_interface is a good example which splits everything into separate files).

@gaaclarke
Copy link
Member

@gaaclarke Could you take a look at the pigeon api?: packages/webview_flutter/webview_flutter_android/pigeons/android_webview.dart and packages/webview_flutter/webview_flutter_android/generatePigeons.sh

Beautiful, lgtm. Hey, I don't understand why you aren't also generating and using the java code? I don't imagine it's much useful to just have the Dart code.

@bparrishMines
Copy link
Contributor Author

@mvanbeusekom According to Effective Dart, it seems to be up to personal preference and whatever is the current style of a plugin. I don't think we have a repo wide policy, but webview_flutter already goes along with this style in https://github.com/flutter/plugins/blob/master/packages/webview_flutter/webview_flutter/lib/src/webview.dart.

I prefer putting related classes in the same file due to

  1. Effective Dart rationale.
  2. All classes in android_webview.dart are a part of the Android WebView Java library.
  3. The lifecycle of each class depends on a WebView instance. WebView is responsible for calling private/@visibleForTesting methods that create and dispose instances. (e.g. WebView.setWebViewClient).

As a side note: I also had the problem of finding the code I needed when coding, but I started using command+N to search by class when using Android Studio. Which made it alot easier for me.

@bparrishMines
Copy link
Contributor Author

@gaaclarke My goal was to do the entire Java side in the next PR because it will probably be another 1000 lines of code to review. It would also be hard to add any running code now because I think putting the plugin in a state that uses method channels and pigeon would put the plugin in a weird state that could cause bugs.

Copy link
Contributor

@mvanbeusekom mvanbeusekom left a comment

Choose a reason for hiding this comment

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

LGTM

@bparrishMines bparrishMines added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Oct 21, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 26, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2021
NickalasB added a commit to NickalasB/plugins that referenced this pull request Oct 27, 2021
* master:
  Implement Android WebView api with pigeon (Java portion) (flutter#4441)
  [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434)
  Implement Android WebView api with pigeon (Dart portion)  (flutter#4435)
  upgraded usage of BinaryMessenger (flutter#4451)
  [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428)
  Use OpenJDK 11 in CI jobs  (flutter#4419)
  [google_sign_in] remove the commented out code in tests (flutter#4442)
NickalasB added a commit to NickalasB/plugins that referenced this pull request Oct 27, 2021
* master:
  Implement Android WebView api with pigeon (Java portion) (flutter#4441)
  [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434)
  Implement Android WebView api with pigeon (Dart portion)  (flutter#4435)
  upgraded usage of BinaryMessenger (flutter#4451)
  [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428)
  Use OpenJDK 11 in CI jobs  (flutter#4419)
  [google_sign_in] remove the commented out code in tests (flutter#4442)
hofmannfelix pushed a commit to hofmannfelix/plugins that referenced this pull request Oct 30, 2021
…ideo_src_on_same_controller

* commit '76e84c0679dbab7bfaaaa553b17bb0dbdb9a3c33': (537 commits)
  [video_player] Initialize player when size and duration become available (flutter#4438)
  [webview_flutter] Implement zoom enabled for ios and android (flutter#4417)
  Partial revert of "upgraded usage of BinaryMessenger (flutter#4451)" (flutter#4453)
  Implement Android WebView api with pigeon (Java portion) (flutter#4441)
  [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434)
  Implement Android WebView api with pigeon (Dart portion)  (flutter#4435)
  upgraded usage of BinaryMessenger (flutter#4451)
  [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428)
  Use OpenJDK 11 in CI jobs  (flutter#4419)
  [google_sign_in] remove the commented out code in tests (flutter#4442)
  [webview] Fix typos in the README (flutter#4249)
  [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180)
  [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429)
  [shared_preferences] Switch to new analysis options (flutter#4384)
  [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413)
  [camera] Run iOS methods on UI thread by default (flutter#4140)
  [ci] Always run all `format` steps (flutter#4427)
  [flutter_plugin_tools] Fix license-check on Windows (flutter#4425)
  [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400)
  [image_picker][android] suppress unchecked warning (flutter#4408)
  ...

# Conflicts:
#	packages/video_player/video_player/pubspec.yaml
#	packages/video_player/video_player_web/lib/video_player_web.dart
#	packages/video_player/video_player_web/pubspec.yaml
amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes p: webview_flutter Edits files for a webview_flutter plugin platform-android waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
3 participants