[webview_flutter_wkwebview] Implementation of Objective-C WKWebView Host Apis #5341

bparrishMines · 2022-04-22T21:49:01Z

Implementation of a few of the Host API methods of WKWebView. I wanted to get early feedback on the method names and the organization of the code.

NOTE 2400 lines of the code is generated by pigeon, so it's not as large as it seems!

No version change:
Part of flutter/flutter#93732 and doesn't make any changes to the current implementation.

No CHANGELOG change: Incremental unused code doesn't need to be noted in the CHANGELOG.

Pre-launch Checklist

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

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFDataConverters.h

...s/webview_flutter/webview_flutter_wkwebview/example/ios/RunnerTests/FWFWebViewHostApiTests.m

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

...s/webview_flutter/webview_flutter_wkwebview/example/ios/RunnerTests/FWFWebViewHostApiTests.m

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

stuartmorgan · 2022-04-26T14:12:14Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFDataConverters.h

+ *
+ * @return An NSURLRequest or nil if data could not be converted.
+ */
+id FWFConvertURLRequestData(FWFNSUrlRequestData* data);


I generally suggest following Apple's Obj-C-ish C function naming convention of naming the returned type for things like this (e.g., NSRectFromCGRect rather than ConvertCGRect), so FWFNSURLRequestFromRequestData.

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

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFWebViewHostApi.h

packages/webview_flutter/webview_flutter_wkwebview/pigeons/web_kit.dart

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

stuartmorgan

I did a full sweep of the generated names and left a bunch of suggestions. Sorry to be bikeshedding on this, it's just tricky since we're still figuring out how to navigate the intersection of the instance manager and Pigeon-generated API ordering constraints.

Jenn and/or Chris should definitely weigh in on my suggestions before you update everything, to avoid you having to rename things over and over.

stuartmorgan · 2022-04-27T19:24:43Z

packages/webview_flutter/webview_flutter_wkwebview/pigeons/web_kit.dart

@@ -376,6 +389,7 @@ abstract class WKWebViewHostApi {

  void loadFlutterAsset(int instanceId, String key);

+  @ObjCSelector('webViewWithInstanceIdCanGoBack:')


Sorry, I missed this before but was reminded by a recent discussion in a code cleanup issue: the ObjC name for all of these should actually be ...WithInstanceIdentifier.... Id is incorrect for ObjC, and while ID is okay, Apple uses Identifier in their APIs and we should be consistent with that.

But I was also thinking as I looked at these that there's not much potential for confusion about what identifier we're referring to in the context of this class IMO, so we could probably just use Identifier instead of InstanceIdentifier everywhere. (The param can still be named instanceId which will make things even more obvious at the declaration point.)

I was considering using identifier as well, but I noticed this made the param names inconsistent. I'm fine with either, though. I'll wait for feedback from @jmagman / @cyanglaz as well.

Identifier sgtm

Identifier +1

stuartmorgan · 2022-04-27T19:25:52Z

packages/webview_flutter/webview_flutter_wkwebview/pigeons/web_kit.dart

@@ -364,10 +375,12 @@ abstract class WKWebViewHostApi {

  void setNavigationDelegate(int instanceId, int? navigationDelegateInstanceId);

+  @ObjCSelector('urlForWebViewWithInstanceId:')


As much as it pains me, the correct naming here is URLFor... even though it's the start of the method. See https://developer.apple.com/documentation/webkit/wkwebview/1415005-url?language=objc for instance.

maybe getUrlForWebViewWithInstanceId:

As Maurice mentioned in another comment where I forgot, ObjC getters shouldn't actually start with get.

stuartmorgan · 2022-04-27T19:28:15Z

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

+@implementation FWFWebView
+- (void)setFrame:(CGRect)frame {
+  [super setFrame:frame];
+  // Prevents the contentInsets to be adjusted by iOS and gives control to Flutter.


Nit: change "to be" to "from being".

stuartmorgan · 2022-04-27T19:28:38Z

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

+  if (!urlRequest) {
+    *error = [FlutterError errorWithCode:@"CreateNSURLRequestFailure"
+                                 message:@"Failed instantiating an NSURLRequest."
+                                 details:[NSString stringWithFormat:@"Url was: '%@'", request.url]];


Nit: "URL was:"

stuartmorgan · 2022-04-27T19:33:01Z

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

+  return self;
+}
+
+- (FWFWebView *)getWebViewInstance:(NSNumber *)instanceId {


getWebViewWithIdentifier:? (see comment in other file about why "identifier")

stuartmorgan · 2022-04-27T19:46:03Z

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

+  @throw [NSException exceptionWithName:@"UnsupportedException" reason:nil userInfo:nil];
+}
+
+- (void)reloadInstanceId:(nonnull NSNumber *)instanceId


reloadWebViewWithIdentifier:

stuartmorgan · 2022-04-27T19:48:04Z

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

+  @throw [NSException exceptionWithName:@"UnsupportedException" reason:nil userInfo:nil];
+}
+
+- (void)setAllowsBackForwardNavigationGesturesInstanceId:(nonnull NSNumber *)instanceId


The setters are all awkward ordering-wise. setBackForwardForWebViewWithIdentifier:isAllowed:error:?

I'd prefer to leave NavigationGestures there, I need a lot context to know what a BackForward mean.
The name becomes very long.

Is navigationGesture not enough? Are there other navigation gestures besides backforward navigation gesture?If not maybe:
setNavigationGesturesForWebViewWithIdentifier:isAllowed:error:

I'd prefer to leave NavigationGestures there, I need a lot context to know what a BackForward mean.

Yes, I'm not sure why I dropped that. My name is clearly wrong since it doesn't disable actually going back or forward, just the gestures.

Having a long name on a method that's only ever going to be called by generated code (and tests) doesn't seem like aa big deal. That's definitely not the longest signature I've ever seen :)

stuartmorgan · 2022-04-27T19:48:20Z

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

+  @throw [NSException exceptionWithName:@"UnsupportedException" reason:nil userInfo:nil];
+}
+
+- (void)setNavigationDelegateInstanceId:(nonnull NSNumber *)instanceId


setNavigationDelegateForWebViewWithIdentifier:delegateIdentifier:error:?

stuartmorgan · 2022-04-27T19:49:39Z

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

+  @throw [NSException exceptionWithName:@"UnsupportedException" reason:nil userInfo:nil];
+}
+
+- (void)setUIDelegateInstanceId:(nonnull NSNumber *)instanceId


Similarly, setUIDelegate{FWVWI}:delegateIdentifier:error:?

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFDataConverters.h

jmagman

Jenn and/or Chris should definitely weigh in on my suggestions before you update everything, to avoid you having to rename things over and over.

@stuartmorgan's name suggestions LGTM

jmagman · 2022-04-27T22:02:45Z

packages/webview_flutter/webview_flutter_wkwebview/pigeons/web_kit.dart

@@ -376,6 +389,7 @@ abstract class WKWebViewHostApi {

  void loadFlutterAsset(int instanceId, String key);

+  @ObjCSelector('webViewWithInstanceIdCanGoBack:')


Identifier sgtm

stuartmorgan

I left a bunch more naming bikeshedding on classes I apparently didn't look at last time. I don't want to beat this PR into the ground though, and I think the patterns are mostly established now and can be applied (with mixed success depending on the name) to all the methods I flagged without me needing to re-review. If there are one or two that we want to adjust later we can do it as we go now that we have a reasonable baseline.

stuartmorgan · 2022-04-29T19:28:58Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFGeneratedWebKitApis.h

+                                           error:(FlutterError *_Nullable *_Nonnull)error;
+- (void)createDefaultDataStoreInstanceId:(NSNumber *)instanceId
+                                   error:(FlutterError *_Nullable *_Nonnull)error;
+- (void)removeDataOfTypesInstanceId:(NSNumber *)instanceId


Apparently I didn't look at every class, sorry 😬

removeDataFromDataStoreWithIdentifier:ofTypes:secondsModifiedSinceEpoch:completion:

stuartmorgan · 2022-04-29T19:29:23Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFGeneratedWebKitApis.h

+
+@protocol FWFUIViewHostApi
+/// @return `nil` only when `error != nil`.
+- (nullable NSArray<NSNumber *> *)getContentOffsetInstanceId:(NSNumber *)instanceId


contentOffsetForViewWithIdentifier:

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFGeneratedWebKitApis.h

stuartmorgan · 2022-04-29T19:43:04Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFGeneratedWebKitApis.h

+/// The codec used by FWFWKPreferencesHostApi.
+NSObject<FlutterMessageCodec> *FWFWKPreferencesHostApiGetCodec(void);
+
+@protocol FWFWKPreferencesHostApi


Same issues for these.

stuartmorgan · 2022-04-29T19:43:28Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFGeneratedWebKitApis.h

+NSObject<FlutterMessageCodec> *FWFWKNavigationDelegateHostApiGetCodec(void);
+
+@protocol FWFWKNavigationDelegateHostApi
+- (void)createInstanceId:(NSNumber *)instanceId error:(FlutterError *_Nullable *_Nonnull)error;


createWithIdentifier:error:

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFGeneratedWebKitApis.h

…KWebView Host Apis (flutter/plugins#5341)

…ost Apis (flutter#5341)

implementation of host apis

fa84355

bparrishMines requested a review from cyanglaz as a code owner April 22, 2022 21:49

github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-ios labels Apr 22, 2022

bparrishMines added 4 commits April 22, 2022 14:55

move comment and fix test

7c13474

add test to verify error

e48695f

add other methods

085e363

change method name and add documentation

8755174

bparrishMines changed the title ~~implementation of host apis~~ [webview_flutter_wkwebview] Implementation of Objective-C WKWebView Host Apis Apr 23, 2022

an

fa5dbeb

bparrishMines requested a review from stuartmorgan April 25, 2022 17:13

jmagman reviewed Apr 25, 2022

View reviewed changes

cyanglaz reviewed Apr 25, 2022

View reviewed changes

This comment was marked as off-topic.

Sign in to view

stuartmorgan suggested changes Apr 26, 2022

View reviewed changes

bparrishMines added 5 commits April 26, 2022 11:11

PR feedback

16d19e9

naming conventions

dd0bb72

use nonatomic

c4f5abb

use nonatomic

0ae3db5

format

ddb8971

bparrishMines requested review from stuartmorgan, cyanglaz and jmagman April 27, 2022 14:42

stuartmorgan reviewed Apr 27, 2022

View reviewed changes

jmagman reviewed Apr 27, 2022

View reviewed changes

bparrishMines added 5 commits April 27, 2022 23:49

implement host api methods

5b1245c

more tests

88e10d0

misspelling

6d7fdfd

assets test

196c07b

rest of tests

25ce261

bparrishMines added 2 commits April 28, 2022 12:38

Merge branch 'master' of github.com:flutter/plugins into host_api

99f043e

test converter

5280385

bparrishMines requested a review from stuartmorgan April 28, 2022 20:24

URL

11ac0a3

stuartmorgan mentioned this pull request Apr 29, 2022

[pigeon] Allow re-ordering parameters in ObjC signature specs flutter/flutter#102837

Open

stuartmorgan approved these changes Apr 29, 2022

View reviewed changes

bparrishMines added 3 commits April 30, 2022 13:58

Merge branch 'master' of github.com:flutter/plugins into host_api

d3fc5e9

fix test

eba9e4b

improve objc naming

4bdbb1d

bparrishMines merged commit 5c9ffc7 into flutter:main Apr 30, 2022

bparrishMines deleted the host_api branch April 30, 2022 22:17

engine-flutter-autoroll mentioned this pull request May 2, 2022

Roll Plugins from 160c714e75f3 to f7d26e94df0b (19 revisions) flutter/flutter#102921

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 2, 2022

5c9ffc71e [webview_flutter_wkwebview] Implementation of Objective-C W…

92d3f14

…KWebView Host Apis (flutter/plugins#5341)

engine-flutter-autoroll mentioned this pull request May 2, 2022

Roll Plugins from 160c714e75f3 to bbcb88d3bae4 (20 revisions) flutter/flutter#102927

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 2, 2022

5c9ffc71e [webview_flutter_wkwebview] Implementation of Objective-C W…

00fb839

…KWebView Host Apis (flutter/plugins#5341)

bparrishMines mentioned this pull request May 3, 2022

[webview_flutter_wkwebview] The rest of the Objective-C HostApi methods #5604

Merged

11 tasks

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[webview_flutter_wkwebview] Implementation of Objective-C WKWebView H…

36820ec

…ost Apis (flutter#5341)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webview_flutter_wkwebview] Implementation of Objective-C WKWebView Host Apis #5341

[webview_flutter_wkwebview] Implementation of Objective-C WKWebView Host Apis #5341

bparrishMines commented Apr 22, 2022 •

edited

Loading

This comment was marked as off-topic.

stuartmorgan Apr 26, 2022

stuartmorgan left a comment

stuartmorgan Apr 27, 2022

bparrishMines Apr 27, 2022

jmagman Apr 27, 2022

cyanglaz Apr 27, 2022

stuartmorgan Apr 27, 2022

cyanglaz Apr 29, 2022

stuartmorgan Apr 29, 2022 •

edited

Loading

stuartmorgan Apr 27, 2022

stuartmorgan Apr 27, 2022

stuartmorgan Apr 27, 2022

stuartmorgan Apr 27, 2022

stuartmorgan Apr 27, 2022

cyanglaz Apr 29, 2022

stuartmorgan Apr 29, 2022

stuartmorgan Apr 27, 2022

stuartmorgan Apr 27, 2022

jmagman left a comment

jmagman Apr 27, 2022

stuartmorgan left a comment

stuartmorgan Apr 29, 2022

stuartmorgan Apr 29, 2022

stuartmorgan Apr 29, 2022

stuartmorgan Apr 29, 2022

		@@ -376,6 +389,7 @@ abstract class WKWebViewHostApi {

		void loadFlutterAsset(int instanceId, String key);

		@ObjCSelector('webViewWithInstanceIdCanGoBack:')

		@@ -364,10 +375,12 @@ abstract class WKWebViewHostApi {

		void setNavigationDelegate(int instanceId, int? navigationDelegateInstanceId);

		@ObjCSelector('urlForWebViewWithInstanceId:')

[webview_flutter_wkwebview] Implementation of Objective-C WKWebView Host Apis #5341

[webview_flutter_wkwebview] Implementation of Objective-C WKWebView Host Apis #5341

Conversation

bparrishMines commented Apr 22, 2022 • edited Loading

Pre-launch Checklist

This comment was marked as off-topic.

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

stuartmorgan Apr 29, 2022 • edited Loading

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

jmagman left a 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

Choose a reason for hiding this comment

bparrishMines commented Apr 22, 2022 •

edited

Loading

stuartmorgan Apr 29, 2022 •

edited

Loading