[webview_flutter] Implementations of `loadFile` and `loadHtmlString` for WKWebView #4486

mvanbeusekom · 2021-11-08T14:38:44Z

Adds implementations of the loadFile and loadHtmlString method channel calls for the webview_flutter_wkwebview package.

The loadFile and loadHtmlString methods have been added to the platform interface by PR #4446 and this PR will add the WKWebView implementation.

The two important commits are:

c1dc83a: contains the implementation, tests and example usage for the loadFile method channel call;
f1343c2: contains the implementation, tests and example usage for the loadHtmlString method channel call.

Related to issues:

NOTE: as per offline discussion with @stuartmorgan the loading of Flutter assets (assets registered within the pubspec.yaml) should be handled via the loadFile method and retrieve the path to the physical asset location the path_provider should be used (or something similar). At the moment the path_provider doesn't support returning the path to the physical location of Flutter assets there is however an issue (flutter/flutter#34026) to track this functionality.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

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

…_wkwebview

Adds native implementation for the `loadFile` method channel call to the webview_flutter_wkwebview package.

Adds native implementation for the `loadHtmlString` method channel call to the webview_flutter_wkwebview package.

…_wkwebview

packages/webview_flutter/webview_flutter_wkwebview/example/lib/main.dart

BeMacized

LGTM

stuartmorgan

Minor nits, but looks good otherwise.

stuartmorgan · 2021-11-11T01:07:57Z

packages/webview_flutter/webview_flutter_wkwebview/example/lib/main.dart

+<body>
+
+<h1>Local demo page</h1>
+<p>This is an example page used to demonstrate how to load a local file or HTML string using the <a href="https://pub.dev/packages/webview_flutter">Flutter webview</a> plugin.</p>


Nit: There's no reason not to break this line to wrap to 80 characters, since HTML doesn't care about whitespace.

stuartmorgan · 2021-11-11T01:10:09Z

packages/webview_flutter/webview_flutter_wkwebview/example/lib/main.dart

+    File indexFile = File('$tmpDir/www/index.html');
+
+    if (await indexFile.exists()) {
+      return indexFile.path;


This early return seems odd; why would we want not changes someone made locally to the example HTML (e.g., to try to reproduce an issue in the context of the example) after having already run once to take effect?

You are right. I added this check with the idea that if the file exists we don't have to write it again (in case the user presses the button twice in the same session). However I didn't think about the fact that the file will be persisted over multiple sessions and developers might want to change the HTML contents.

stuartmorgan · 2021-11-11T01:15:05Z

packages/webview_flutter/webview_flutter_wkwebview/example/lib/web_view.dart

+  Future<void> loadFile(
+    String absoluteFilePath,
+  ) {
+    assert(absoluteFilePath != null || absoluteFilePath.isNotEmpty);


We don't need null assertions in example code that's null-safe; no non-null-safe code can call it since it's not a library.

Removed the redundant assert.

stuartmorgan · 2021-11-11T01:15:12Z

packages/webview_flutter/webview_flutter_wkwebview/example/lib/web_view.dart

+    String html, {
+    String? baseUrl,
+  }) {
+    assert(html != null || html.isNotEmpty);


Removed the redundant assert.

stuartmorgan · 2021-11-11T01:16:42Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FlutterWebView.m

+    return;
+  }
+
+  NSURL* url = [NSURL fileURLWithPath:[call arguments] isDirectory:NO];


You should validate that this is non-nil before passing it below (returning an error if it is nil) in case someone passes a garbage string that doesn't parse as a path.

Added additional check to ensure the url variable is not nil, return an error if the variable is nil.

stuartmorgan · 2021-11-11T01:17:26Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FlutterWebView.m

+
+- (void)onLoadHtmlString:(FlutterMethodCall*)call result:(FlutterResult)result {
+  NSDictionary* arguments = [call arguments];
+  if (!arguments || ![arguments isKindOfClass:NSDictionary.class]) {


The first check is redundant; nil isKindOfClass: will always return false.

Removed the redundant check.

stuartmorgan · 2021-11-11T15:07:20Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FlutterWebView.m

@@ -517,6 +564,29 @@ - (void)updateUserAgent:(NSString*)userAgent {
  }
 }

+ (bool)isValidStringArgument:(id)argument withErrorMessage:(NSString**)errorDetails {


Needs a declaration comment, per style guide.

…_wkwebview

stuartmorgan

LGTM with final nits

stuartmorgan · 2021-11-11T19:54:43Z

packages/webview_flutter/webview_flutter_wkwebview/example/lib/web_view.dart

@@ -315,7 +315,6 @@ class WebViewController {
  Future<void> loadFile(
    String absoluteFilePath,
  ) {
-    assert(absoluteFilePath != null || absoluteFilePath.isNotEmpty);


Don't you still want the non-emtpy assert here and below?

stuartmorgan · 2021-11-11T19:56:02Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FlutterWebView.m

+ * @param argument The argument that should be validated.
+ * @param errorDetails An optional NSString variable which will contain a detailed error message in
+ * case the supplied argument is not valid.
+ * @return `true` if the given `argument` is a valid non-null, non-empty string; otherwise `false`.


Nit: "YES" and "NO", not "true" and "false"

stuartmorgan · 2021-11-11T19:56:18Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FlutterWebView.m

+ * @param errorDetails An optional NSString variable which will contain a detailed error message in
+ * case the supplied argument is not valid.
+ * @return `true` if the given `argument` is a valid non-null, non-empty string; otherwise `false`.
+ */
 + (bool)isValidStringArgument:(id)argument withErrorMessage:(NSString**)errorDetails {


Oops, and I missed that this is bool; it should be BOOL.

…String` for WKWebView (flutter/plugins#4486)

…for WKWebView (flutter#4486)

mvanbeusekom added 4 commits November 5, 2021 09:28

Merge remote-tracking branch 'upstream/master' into webview_load_file…

8e05ac2

…_wkwebview

Implementation of loadFile for WKWebView.

c1dc83a

Adds native implementation for the `loadFile` method channel call to the webview_flutter_wkwebview package.

Implementation of loadHtmlString for WKWebView.

f1343c2

Adds native implementation for the `loadHtmlString` method channel call to the webview_flutter_wkwebview package.

Merge remote-tracking branch 'upstream/master' into webview_load_file…

0351601

…_wkwebview

mvanbeusekom requested review from stuartmorgan and BeMacized November 8, 2021 14:38

google-cla bot added the cla: yes label Nov 8, 2021

github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-ios labels Nov 8, 2021

BeMacized reviewed Nov 8, 2021

View reviewed changes

packages/webview_flutter/webview_flutter_wkwebview/example/lib/main.dart Outdated Show resolved Hide resolved

packages/webview_flutter/webview_flutter_wkwebview/example/lib/main.dart Show resolved Hide resolved

Bump version number

b945839

mvanbeusekom requested a review from bparrishMines November 9, 2021 09:11

BeMacized approved these changes Nov 9, 2021

View reviewed changes

stuartmorgan suggested changes Nov 11, 2021

View reviewed changes

mvanbeusekom added 2 commits November 11, 2021 16:25

Merge remote-tracking branch 'upstream/master' into webview_load_file…

5218c0a

…_wkwebview

Apply feedback on PR

d5cf9f9

mvanbeusekom force-pushed the webview_load_file_wkwebview branch from f78a3bc to d5cf9f9 Compare November 11, 2021 16:09

mvanbeusekom requested a review from stuartmorgan November 11, 2021 16:18

stuartmorgan approved these changes Nov 11, 2021

View reviewed changes

Fix final nits

98eb236

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 Nov 12, 2021

fluttergithubbot merged commit 66ed8f4 into flutter:master Nov 12, 2021

engine-flutter-autoroll mentioned this pull request Nov 12, 2021

Roll Plugins from a103bcd3327e to 66ed8f4105ef (1 revision) flutter/flutter#93531

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2021

66ed8f4 [webview_flutter] Implementations of loadFile and `loadHtml…

5444d30

…String` for WKWebView (flutter/plugins#4486)

engine-flutter-autoroll mentioned this pull request Nov 12, 2021

Roll Plugins from a103bcd3327e to 66ed8f4105ef (1 revision) flutter/flutter#93538

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2021

66ed8f4 [webview_flutter] Implementations of loadFile and `loadHtml…

7362a97

…String` for WKWebView (flutter/plugins#4486)

engine-flutter-autoroll mentioned this pull request Nov 12, 2021

Roll Plugins from a103bcd3327e to 66ed8f4105ef (1 revision) flutter/flutter#93549

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2021

66ed8f4 [webview_flutter] Implementations of loadFile and `loadHtml…

0584ad0

…String` for WKWebView (flutter/plugins#4486)

engine-flutter-autoroll mentioned this pull request Nov 12, 2021

Roll Plugins from a103bcd3327e to ace9e78bd3c3 (2 revisions) flutter/flutter#93560

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2021

66ed8f4 [webview_flutter] Implementations of loadFile and `loadHtml…

28ec6c6

…String` for WKWebView (flutter/plugins#4486)

This was referenced Nov 16, 2021

[webview_flutter] Fix issue where local and asset HTML files cannot be loaded #3693

Closed

[webview_flutter] local html and baseUrl support #2830

Closed

amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021

[webview_flutter] Implementations of loadFile and loadHtmlString …

40a684a

…for WKWebView (flutter#4486)

KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021

[webview_flutter] Implementations of loadFile and loadHtmlString …

2d1e3e9

…for WKWebView (flutter#4486)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webview_flutter] Implementations of `loadFile` and `loadHtmlString` for WKWebView #4486

[webview_flutter] Implementations of `loadFile` and `loadHtmlString` for WKWebView #4486

mvanbeusekom commented Nov 8, 2021 •

edited

Loading

BeMacized left a comment

stuartmorgan left a comment

stuartmorgan Nov 11, 2021

mvanbeusekom Nov 11, 2021

stuartmorgan Nov 11, 2021

mvanbeusekom Nov 11, 2021

stuartmorgan Nov 11, 2021

mvanbeusekom Nov 11, 2021

stuartmorgan Nov 11, 2021

mvanbeusekom Nov 11, 2021

stuartmorgan Nov 11, 2021

mvanbeusekom Nov 11, 2021

stuartmorgan Nov 11, 2021

mvanbeusekom Nov 11, 2021

stuartmorgan Nov 11, 2021

mvanbeusekom Nov 11, 2021

stuartmorgan left a comment

stuartmorgan Nov 11, 2021

stuartmorgan Nov 11, 2021

stuartmorgan Nov 11, 2021

[webview_flutter] Implementations of loadFile and loadHtmlString for WKWebView #4486

[webview_flutter] Implementations of loadFile and loadHtmlString for WKWebView #4486

Conversation

mvanbeusekom commented Nov 8, 2021 • edited Loading

Pre-launch Checklist

BeMacized left a comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[webview_flutter] Implementations of `loadFile` and `loadHtmlString` for WKWebView #4486

[webview_flutter] Implementations of `loadFile` and `loadHtmlString` for WKWebView #4486

mvanbeusekom commented Nov 8, 2021 •

edited

Loading