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

[webview_flutter] Extract WKWebView implementation into a separate package #4345

Merged

Conversation

mvanbeusekom
Copy link
Contributor

This PR puts Apple's WKWebView control implementation of the platform interface (which was separated in #4302) into its own package, in preparation for moving the main plugin to a federated architecture.

Relevant issue:

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.

mvanbeusekom and others added 8 commits September 14, 2021 13:35
Creates a new `webview_flutter_wkwebview` directory and adds
the following meta-data files:
- `AUTHORS`: copied from the `webview_flutter` package and added my name;
- `CHANGELOG.md`: new file adding description for release 0.0.1;
- `LICENSE`: copied from the `webview_flutter` package;
- `README.md`: new file adding the standard platform implementation
  description;
- `pubspec.yaml`: new file adding package meta-data for the
  `webview_flutter_wkwebview` package.
A one to one copy of the `webview_flutter/ios` folder to
`webview_flutter_wkwebview/` using the following command:
```
cp -R ./webview_flutter/ios ./webview_flutter_wkwebview/
```
For the Cocaopod package to be registered correctly the .podspec file
name needs to match the name of the Flutter package.
Copied the WKWebView specific .dart files over from the
`./webview_flutter` package.
Make sure the `CupertinoWebView` widget extends the `WebViewPlatform`
class from the `webview_flutter_platform_interface` package correctly by
accepting an instance of the `JavascriptChannelRegistry` class.
This commit makes a direct copy of the `webview_flutter/example` app to
the  `webview_flutter_wkwebview` package. After the copy the `example/android`
folder is removed as it doesn't serve a purpose in the WKWebView specific
package. Commands run where:
```
cp -R ./webview_flutter/example ./webview_flutter_wkwebview/
rm -rf ./webview_flutter_wkwebview/example/ios
```
This commit updates the example App so it directly implements the
WKWebView specific implementation of the webview_flutter_platform_interface.
Updated the existing integration tests (copied from webview_flutter
package) so they work correctly with the implementation of the
webview_flutter_wkwebview package.

Co-authored-by: BeMacized <[email protected]>
@google-cla
Copy link

google-cla bot commented Sep 14, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Sep 14, 2021
@github-actions github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-ios labels Sep 14, 2021
@BeMacized
Copy link
Contributor

@googlebot I consent.

This commit resolves failing UI tests and ensures the `Publishable` task
is green.
Update the documentation URL in the
`ios/webview_flutter_wkwebview.podspec` file to point to a valid
location. The `https://pub.dev/packages/webview_flutter_wkwebview`
package doesn't exists until this PR is published. However the `pod lib
lint` step in CI is failing if the URL doesn't exist yet.
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Just a few minor things. Also, we should temporarily exclude this from build_all even if it's not actively breaking anything, just in case.

@@ -0,0 +1,3 @@
## 0.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Let's use the current flutter_webview version as a starting point. It's kind of weird to have it look like this is a completely new thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree and updated the version to 2.0.13 (same as current webview_flutter package).


environment:
sdk: ">=2.12.0 <3.0.0"
flutter: ">=2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid a follow-up PR: this can be 2.14 and 2.5 respectively, since we aren't supporting iOS 8 now. We're in the process of updating plugins accordingly.

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.

@@ -21,6 +21,6 @@
<key>CFBundleVersion</key>
<string>1.0</string>
<key>MinimumOSVersion</key>
<string>8.0</string>
<string>9.0</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

The webview_flutter/webview_flutter changes should be reverted.

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.

@@ -352,3 +363,584 @@ class NavigationControls extends StatelessWidget {
);
}
}

/// A web view widget for showing html content.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put these classes that wouldn't be part of normal webview package use into a separate file, and put a comment at the top explaining why they are needed, since I don't think it'll be obvious to someone new to the strucutre why this is done.

Copy link
Contributor Author

@mvanbeusekom mvanbeusekom Sep 16, 2021

Choose a reason for hiding this comment

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

I have put all classes in one file on purpose since the contents of the example/main.dart file is shown on the "Example" tab on pub.dev. In the past I have received complaints from users explaining that the code shown on this page doesn't demonstrate how to use the plugin because that particular code is hidden in files that are not displayed on the "Example" tab on pub.dev. Since the actual usage of the webview_flutter_android and webview_flutter_wkwebview is demonstrated in the WebView widget (which has been separated from the WebViewExample widget to facilitate integration testing) moving it out into a separate file means that the pub.dev "Example" page would not show how to use/ implement the platform specific package.

Before I start moving the classes out I would like to double check if you are fine with these consequences? Most likely the platform specific packages will not be used directly by developers (and the packages should be marked as unlisted on pub.dev) but it doesn't hurt to double check and be sure ;).

Copy link
Contributor

Choose a reason for hiding this comment

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

In the past I have received complaints from users explaining that the code shown on this page doesn't demonstrate how to use the plugin because that particular code is hidden in files that are not displayed on the "Example" tab on pub.dev.
[...]
Most likely the platform specific packages will not be used directly by developers (and the packages should be marked as unlisted on pub.dev) but it doesn't hurt to double check and be sure ;).

Yes, I don't expect anyone to actually use these packages directly; the example exists to run integration tests with. If someone really want to use the implementation package directly by itself, and are already going to the trouble of manually constructing pub.dev URLs to go to the unlisted page for it, they can take the extra step to click to the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great thank you for confirming, I will continue with the necessary changes.

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.

@mvanbeusekom mvanbeusekom force-pushed the webview/federated_architecture_part_4 branch from f8672ae to 2181ff1 Compare September 20, 2021 14:04
Move the `WebView` and related `WebViewController` classes from the
main.dart into a separate web_view.dart file.
@mvanbeusekom mvanbeusekom force-pushed the webview/federated_architecture_part_4 branch from 2181ff1 to d20e802 Compare September 20, 2021 14:08
Updated the version of the plugin to the version of webview_flutter
package (2.0.13).

Also updated the Dart and Flutter versions to respectively 2.14.0 and
2.5.0.
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@mvanbeusekom mvanbeusekom 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 Sep 20, 2021
@fluttergithubbot
Copy link

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite ios-platform_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed 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 Sep 20, 2021
@mvanbeusekom mvanbeusekom merged commit c05887f into flutter:master Sep 21, 2021
@mvanbeusekom mvanbeusekom deleted the webview/federated_architecture_part_4 branch September 21, 2021 06:40
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 21, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 21, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 21, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 21, 2021
amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021
…ckage (flutter#4345)

* Setup webview_flutter_wkwebview package.

Creates a new `webview_flutter_wkwebview` directory and adds
the following meta-data files:
- `AUTHORS`: copied from the `webview_flutter` package and added my name;
- `CHANGELOG.md`: new file adding description for release 0.0.1;
- `LICENSE`: copied from the `webview_flutter` package;
- `README.md`: new file adding the standard platform implementation
  description;
- `pubspec.yaml`: new file adding package meta-data for the
  `webview_flutter_wkwebview` package.

* Direct copy of "iOS" folder.

A one to one copy of the `webview_flutter/ios` folder to
`webview_flutter_wkwebview/` using the following command:
```
cp -R ./webview_flutter/ios ./webview_flutter_wkwebview/
```

* Rename .podspec file to match package name.

For the Cocaopod package to be registered correctly the .podspec file
name needs to match the name of the Flutter package.

* Direct copy of WKWebView specific .dart files.

Copied the WKWebView specific .dart files over from the
`./webview_flutter` package.

* Modify .dart code to work with new platform_interface.

Make sure the `CupertinoWebView` widget extends the `WebViewPlatform`
class from the `webview_flutter_platform_interface` package correctly by
accepting an instance of the `JavascriptChannelRegistry` class.

* Direct copy of the `webview_flutter/example` app.

This commit makes a direct copy of the `webview_flutter/example` app to
the  `webview_flutter_wkwebview` package. After the copy the `example/android`
folder is removed as it doesn't serve a purpose in the WKWebView specific
package. Commands run where:
```
cp -R ./webview_flutter/example ./webview_flutter_wkwebview/
rm -rf ./webview_flutter_wkwebview/example/ios
```

* Update example to WKWebView specific implementation.

This commit updates the example App so it directly implements the
WKWebView specific implementation of the webview_flutter_platform_interface.

* Update integration tests.

Updated the existing integration tests (copied from webview_flutter
package) so they work correctly with the implementation of the
webview_flutter_wkwebview package.

Co-authored-by: BeMacized <[email protected]>

* Fix iOS UI tests.

This commit resolves failing UI tests and ensures the `Publishable` task
is green.

* Point to existing documentation URL

Update the documentation URL in the
`ios/webview_flutter_wkwebview.podspec` file to point to a valid
location. The `https://pub.dev/packages/webview_flutter_wkwebview`
package doesn't exists until this PR is published. However the `pod lib
lint` step in CI is failing if the URL doesn't exist yet.

* Split helper classes from main example widget.

Move the `WebView` and related `WebViewController` classes from the
main.dart into a separate web_view.dart file.

* Updated version numbers as suggested in review.

Updated the version of the plugin to the version of webview_flutter
package (2.0.13).

Also updated the Dart and Flutter versions to respectively 2.14.0 and
2.5.0.

Co-authored-by: BeMacized <[email protected]>
NickalasB added a commit to NickalasB/plugins that referenced this pull request Sep 27, 2021
* master: (51 commits)
  [webview_flutter] Update version number app_facing package (flutter#4375)
  [webview_flutter] Adjust integration test domains (flutter#4383)
  Remove some trivial custom analysis options files (flutter#4379)
  [google_maps_flutter_platfomr_interface] Add Marker drag events (flutter#2653)
  [flutter_plugin_tools] Improve version check error handling (flutter#4376)
  [flutter_plugin_tools] Allow overriding breaking change check (flutter#4369)
  [url_launcher] Error handling when URL cannot be parsed with Uri.parse (flutter#4365)
  [webview_flutter] Migrate main package to fully federated architecture. (flutter#4366)
  [google_sign_in] Bump minimum Flutter version and iOS deployment target (flutter#4334)
  Add false secret lists, and enforce ordering (flutter#4372)
  [camera_web] Update usage documentation (flutter#4371)
  [video_player] VTT Support (flutter#2878)
  Require authors file (flutter#4367)
  [flutter_plugin_tools] Fix federated safety check (flutter#4368)
  [webview_flutter] Extract WKWebView implementation into a separate package (flutter#4345)
  [webview_flutter] Extract Android implementation into a separate package (flutter#4343)
  [in_app_purchase] Ensure the `introductoryPriceMicros` field is populated correctly. (flutter#4364)
  [flutter_plugin_tools] Add a federated PR safety check (flutter#4329)
  [camera] Add web support (flutter#4240)
  [webview_flutter] Bump minimum Flutter version and iOS deployment target (flutter#4361)
  ...

# Conflicts:
#	packages/webview_flutter/webview_flutter/lib/platform_interface.dart
#	packages/webview_flutter/webview_flutter/lib/src/webview_method_channel.dart
#	packages/webview_flutter/webview_flutter/lib/webview_flutter.dart
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
…ckage (flutter#4345)

* Setup webview_flutter_wkwebview package.

Creates a new `webview_flutter_wkwebview` directory and adds
the following meta-data files:
- `AUTHORS`: copied from the `webview_flutter` package and added my name;
- `CHANGELOG.md`: new file adding description for release 0.0.1;
- `LICENSE`: copied from the `webview_flutter` package;
- `README.md`: new file adding the standard platform implementation
  description;
- `pubspec.yaml`: new file adding package meta-data for the
  `webview_flutter_wkwebview` package.

* Direct copy of "iOS" folder.

A one to one copy of the `webview_flutter/ios` folder to
`webview_flutter_wkwebview/` using the following command:
```
cp -R ./webview_flutter/ios ./webview_flutter_wkwebview/
```

* Rename .podspec file to match package name.

For the Cocaopod package to be registered correctly the .podspec file
name needs to match the name of the Flutter package.

* Direct copy of WKWebView specific .dart files.

Copied the WKWebView specific .dart files over from the
`./webview_flutter` package.

* Modify .dart code to work with new platform_interface.

Make sure the `CupertinoWebView` widget extends the `WebViewPlatform`
class from the `webview_flutter_platform_interface` package correctly by
accepting an instance of the `JavascriptChannelRegistry` class.

* Direct copy of the `webview_flutter/example` app.

This commit makes a direct copy of the `webview_flutter/example` app to
the  `webview_flutter_wkwebview` package. After the copy the `example/android`
folder is removed as it doesn't serve a purpose in the WKWebView specific
package. Commands run where:
```
cp -R ./webview_flutter/example ./webview_flutter_wkwebview/
rm -rf ./webview_flutter_wkwebview/example/ios
```

* Update example to WKWebView specific implementation.

This commit updates the example App so it directly implements the
WKWebView specific implementation of the webview_flutter_platform_interface.

* Update integration tests.

Updated the existing integration tests (copied from webview_flutter
package) so they work correctly with the implementation of the
webview_flutter_wkwebview package.

Co-authored-by: BeMacized <[email protected]>

* Fix iOS UI tests.

This commit resolves failing UI tests and ensures the `Publishable` task
is green.

* Point to existing documentation URL

Update the documentation URL in the
`ios/webview_flutter_wkwebview.podspec` file to point to a valid
location. The `https://pub.dev/packages/webview_flutter_wkwebview`
package doesn't exists until this PR is published. However the `pod lib
lint` step in CI is failing if the URL doesn't exist yet.

* Split helper classes from main example widget.

Move the `WebView` and related `WebViewController` classes from the
main.dart into a separate web_view.dart file.

* Updated version numbers as suggested in review.

Updated the version of the plugin to the version of webview_flutter
package (2.0.13).

Also updated the Dart and Flutter versions to respectively 2.14.0 and
2.5.0.

Co-authored-by: BeMacized <[email protected]>
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-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants