[webview_flutter] Proof of concept web implementation by BeMacized · Pull Request #4565 · flutter/plugins

BeMacized · 2021-12-02T08:21:35Z

This PR adds a very limited proof of concept for a web implementation of the webview_flutter plugin in a separate package named webview_flutter_web.

Due to various limits on the web platform, this version currently only supports a few features.

Because of its limited functionality, it currently is not compatible with the example of the app-facing plugin, only its own included example.

The provided shims for dart:ui are still triggering analysis warnings that I'm not sure how to resolve.
(Something to do with flutter/flutter#55000). I cannot seem to disable all of the warnings that occur for the shim files, and I have not been able to figure out if providing a custom analysis_options.yaml that excludes the shim directory is a possibility here.

Likewise a check fails due to there being no integration tests. As far as I know these are not a thing yet for the web platform, but I'm not sure how to disable the check.

Relevant issue:

[webview_flutter] Add support for web flutter#45295

Pre-launch Checklist

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

stuartmorgan-g

A few notes from a quick skim; I'll leave the full review to @ditman

packages/webview_flutter/webview_flutter_web/AUTHORS

packages/webview_flutter/webview_flutter_web/CHANGELOG.md

packages/webview_flutter/webview_flutter_web/example/pubspec.yaml

packages/webview_flutter/webview_flutter_web/lib/shims/dart_ui_fake.dart

packages/webview_flutter/webview_flutter_web/pubspec.yaml

packages/webview_flutter/webview_flutter_web/lib/webview_flutter_web.dart

packages/webview_flutter/webview_flutter_web/README.md

ditman

This LGTM, I've only left a bunch of minor nits, and some confused questions about the New Way of federating things :)

The analysis is also complaining a little bit:

  error - lib/webview_flutter_web.dart:64:7 - Missing concrete implementation of 'WebViewPlatformController.loadFlutterAsset'. Try implementing the missing method, or make the class abstract. - non_abstract_class_inherits_abstract_member
   info - example/lib/main.dart:35:18 - Specify type annotations. - always_specify_types
   info - example/lib/web_view.dart:330:37 - Prefer const with constant constructors. - prefer_const_constructors
   info - example/lib/web_view.dart:382:16 - Prefer const with constant constructors. - prefer_const_constructors
   info - lib/shims/dart_ui.dart:8:1 - Use Flutter TODO format: // TODO(username): message, https://URL-to-issue. - flutter_style_todos

packages/webview_flutter/webview_flutter_web/lib/webview_flutter_web.dart

ditman · 2021-12-06T19:52:03Z

packages/webview_flutter/webview_flutter_web/lib/webview_flutter_web.dart

+}
+
+/// Factory class for creating [HttpRequest] instances.
+class HttpRequestFactory {


Why this factory, instead of asking users to pass in an HttpRequest object?

(This is used to be mocked in tests, to ensure that the WebViewRequest is correctly parsed; I have a comment about this approach below)

The only reason this factory exists, is so that HttpRequest.request can be mocked in the tests.
Ideally we wouldn't have this class at all, but I'm not sure how else I'd mock it.

Having the user supply the HttpRequest object is not really an option, because as far as I can see, the http request is fired immediately when HttpRequest.request(...) is called.

If you know of a better way to do this, I'd be very interested as it feels like a bit much just for a mock.

ditman · 2021-12-06T20:09:21Z

packages/webview_flutter/webview_flutter_web/lib/webview_flutter_web.dart

+  /// when the file cannot be found.
+  ///
+  /// See also: [authorization headers](http://en.wikipedia.org/wiki/Basic_access_authentication).
+  Future<HttpRequest> request(String url,


I think this would be more useful if it was:

Future<HttpRequest> fromWebViewRequest(WebViewRequest webViewRequest) { ... return HttpRequest.request(webViewRequest.url, ... );

then have the HttpRequestFactory class (or maybe the bare fromWebViewRequest function) unit-tested with as many weird WebViewRequest examples as needed, rather than having to make this an overrideable property just to see that the loadRequest method is extracting the correct values from the WebViewRequest object, and passing the expected values to this method.

What do you think?

(This is a completely private API, so this can be done later, if wanted)

As mentioned above, this factory only exists for directly mocking HttpRequest.request. If this is something we need to unit test please let me know, but I assumed it would not be the case as it's nothing more than a direct pass-through.

Whether the correct request is generated from the WebViewRequest in loadRequest should already be unit tested.

ditman · 2021-12-06T20:14:57Z

packages/webview_flutter/webview_flutter_web/lib/webview_flutter_web.dart

+  Future<bool> clearCookies() async => false;
+
+  /// Gets called when the plugin is registered.
+  static void registerWith(Registrar registrar) {}


Shouldn't this method be registering the instance of the plugin to the WebWebViewPlatform class? There's no global WebViewPlugin.instance anymore? Or is this the new way of doing federation?

PS: let's say we want to add web support to the webview (core) example here. Can it be done?

I have to confess that I'm not entirely sure. Afaik the federation is like Stuart mentioned below not very standard on this plugin. I assumed this method would not be needed, but the generated_plugin_registrant.dart in the example does require it to be here. @mvanbeusekom maybe you have some extra info?

Yep, this is normally what we use to set the "instance" of the platform-specific plugin in other plugins (see url_launcher_web for example). However this plugin is not federated exactly the same as others, so that's why this method is empty (see comment).

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

stuartmorgan-g · 2021-12-06T20:47:11Z

and some confused questions about the New Way of federating things :)

webview_flutter doesn't have a new way of federating things, it just doesn't have the modern way. It was (IIUC) a sort of prototype of the platform interface, and we federated it without my realizing that—and thus without us cleaning it up first. See flutter/flutter#94051 (the second bullet point I think is where some of your confusion is coming from)

…er_web.dart Co-authored-by: David Iglesias <[email protected]>

…ebview_flutter/web_impl

…eb_impl

flutter-dashboard · 2021-12-06T23:10:47Z

This pull request was opened against a branch other than master. Since Flutter pull requests should not normally be opened against branches other than master, I have changed the base to master. If this was intended, you may modify the base back to master. See the Release Process for information about how other branches get updated.

Reviewers: Use caution before merging pull requests to branches other than master, unless this is an intentional hotfix/cherrypick.

ditman · 2021-12-07T00:51:26Z

See flutter/flutter#94051 (the second bullet point I think is where some of your confusion is coming from)

Ah, yes @stuartmorgan, that issue helped to understand what's going on in this PR, thanks for the link!

ditman · 2021-12-07T00:58:49Z

Running "flutter pub get" in webview_flutter_web...              2,228ms
org-dartlang-app:///webview_flutter_web_test.mocks.dart:735:43: Error: The parameter 'value' of the method 'MockIFrameElement.setAttribute' has type 'String?', which does not match the corresponding type, 'Object', in the overridden method, 'Element.setAttribute'.
 - 'Object' is from 'dart:core'.
Change to a supertype of 'Object', or, for a covariant parameter, a subtype.
  void setAttribute(String? name, String? value) =>
                                          ^
org-dartlang-sdk:///dart-sdk/lib/html/dart2js/html_dart2js.dart: Context: This is the overridden method ('setAttribute').
org-dartlang-app:///webview_flutter_web_test.mocks.dart:739:67: Error: The parameter 'value' of the method 'MockIFrameElement.setAttributeNS' has type 'String?', which does not match the corresponding type, 'Object', in the overridden method, 'Element.setAttributeNS'.
 - 'Object' is from 'dart:core'.
Change to a supertype of 'Object', or, for a covariant parameter, a subtype.
  void setAttributeNS(String? namespaceURI, String? name, String? value) =>
                                                                  ^
org-dartlang-sdk:///dart-sdk/lib/html/dart2js/html_dart2js.dart: Context: This is the overridden method ('setAttributeNS').
Failed to compile

Is this a problem with the generated mock?

BeMacized · 2021-12-07T11:48:24Z

Is this a problem with the generated mock?

@ditman Seems like it. As this is from a generated file, I'm not really sure how to go about fixing it. I've regenerated the mocks just to be sure, but it doesn't seem to have an effect.

stuartmorgan-g

A few minor things

packages/webview_flutter/webview_flutter_web/AUTHORS

packages/webview_flutter/webview_flutter_web/README.md

packages/webview_flutter/webview_flutter_web/lib/webview_flutter_web.dart

stuartmorgan-g · 2021-12-07T15:34:12Z

Is this a problem with the generated mock?

@ditman Seems like it. As this is from a generated file, I'm not really sure how to go about fixing it. I've regenerated the mocks just to be sure, but it doesn't seem to have an effect.

Are there non-web shims for the class being mocked? If so, perhaps those shims have the wrong types.

If minor edits to the generated file work, then let's just do that (with comments), and then follow up with an issue to try to find a better solution. If minor edits aren't enough, can you make a manual fake that does what you need? It looks like you're not using that many methods/properties.

ditman · 2021-12-07T19:58:56Z

In this case, the class being problematic seems to be dart:html IFrameElement; maybe you can use a new IFrameElement() and pass it around, instead of a mock?

(This is normally easier said than done, but in general you should only mock classes owned by your package)

stuartmorgan-g · 2021-12-07T20:00:37Z

I just noticed that it's only failing on master, so maybe the types changed. What Flutter channel did you generate the mocks on?

ditman · 2021-12-07T20:05:18Z

Looks like this change: dart-lang/sdk@017850f (it's likely that mocks generated on master will be backwards compatible with stable, at least for that change)

stuartmorgan-g · 2021-12-07T20:07:11Z

I had the same thought; I'm locally testing changing to Object? to see if stable is happy.

stuartmorgan-g · 2021-12-07T20:11:03Z

It's also missing integration tests; I'm trying to figure out if we can useful test anything without being able to inspect the iframe. It's probably useful to at least have something that builds the whole plugin and checks that calling loadUrl doesn't explode even if we can't see that it worked...

stuartmorgan-g · 2021-12-07T20:12:06Z

I had the same thought; I'm locally testing changing to Object? to see if stable is happy.

Yep. I'll push that shortly once I figure out what to do with integration tests.

stuartmorgan-g · 2021-12-07T20:17:03Z

Wait, why does the example only have post when post doesn't work on web? @BeMacized do you have an updated example already, or should I fix it now?

ditman · 2021-12-07T20:18:21Z

@stuartmorgan it seems that POST does work on web, as long as CORS/same domain passes. The only thing POST seems to do is to read the response body from the request, and render it as text on the iframe via a data: URL (if I understood the code correctly) ((It's a very odd thing to demonstrate, though))

stuartmorgan-g · 2021-12-07T20:22:56Z

Oh, I missed that; I thought the only thing we were supporting was setting the src. Maybe I'll just add a simple loadUrl option then, rather than changing it.

BeMacized · 2021-12-07T20:31:18Z

it seems that POST does work on web, as long as CORS/same domain passes.

This is correct. The current example should be up to date.

It's also missing integration tests

I left these out, as when I tried writing these, I got an error message telling me that integration tests were not yet supported on web.

stuartmorgan-g · 2021-12-07T20:35:10Z

Oh, I missed that; I thought the only thing we were supporting was setting the src. Maybe I'll just add a simple loadUrl option then, rather than changing it.

I see, this is just me being unfamiliar with the webview_flutter examples. The main page does the standard simple load, and then there's a menu for other things. This is the subset of that standard example that works here. It's a bit odd in the context of this package that doesn't have many options, but that's fine.

I'll look at a simple integration test though.

I left these out, as when I tried writing these, I got an error message telling me that integrations tests were not yet supported on web.

Where you using flutter test rather than flutter drive maybe?

BeMacized · 2021-12-07T20:39:13Z

I'll look at a simple integration test though.

Where you using flutter test rather than flutter drive maybe?

This could very well have been the case yes. I can take another look at writing some integration tests tomorrow morning if needed.

stuartmorgan-g · 2021-12-07T20:47:51Z

@BeMacized I have very minimal integration tests ready, and a fix for the unit test. Can you open this branch to pushes from Flutter contributors so I can push them?

stuartmorgan-g · 2021-12-08T00:39:44Z

Closing in favor of #4594, which fixed/added tests.

BeMacized added 3 commits November 3, 2021 08:52

First WIP draft of web implementation

518a78a

Add tests and implement loadUrl

582744c

Merge branch 'master' into webview_flutter/web_impl

49c6d2a

google-cla bot added the cla: yes label Dec 2, 2021

github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-web labels Dec 2, 2021

Fix fixable analysis errors.

5685e9a

mvanbeusekom requested a review from stuartmorgan-g December 2, 2021 19:36

BeMacized added 2 commits December 3, 2021 14:36

Add missing files and update platform interface

23645af

Add missing license header

b212bea

stuartmorgan-g requested a review from ditman December 3, 2021 20:33

stuartmorgan-g suggested changes Dec 3, 2021

View reviewed changes

BeMacized added 2 commits December 6, 2021 17:07

Processed feedback. Implemented loadRequest & loadHtmlString.

196d540

Update changelog

bcfcae6

ditman approved these changes Dec 6, 2021

View reviewed changes

BeMacized and others added 5 commits December 6, 2021 23:33

Update packages/webview_flutter/webview_flutter_web/lib/webview_flutt…

e882edb

…er_web.dart Co-authored-by: David Iglesias <[email protected]>

Implement PR feedback

e60e26a

Merge remote-tracking branch 'origin/webview_flutter/web_impl' into w…

e845413

…ebview_flutter/web_impl

Merge remote-tracking branch 'upstream/master' into webview_flutter/w…

fd36e2c

…eb_impl

Update platform interface dependency

5600caa

flutter-dashboard bot changed the base branch from master to main December 6, 2021 23:10

Update mocks

87154fa

stuartmorgan-g suggested changes Dec 7, 2021

View reviewed changes

packages/webview_flutter/webview_flutter_web/AUTHORS Outdated Show resolved Hide resolved

packages/webview_flutter/webview_flutter_web/README.md Outdated Show resolved Hide resolved

packages/webview_flutter/webview_flutter_web/lib/webview_flutter_web.dart Outdated Show resolved Hide resolved

stuartmorgan-g changed the base branch from main to master December 7, 2021 19:57

Fix authors file

c4d577a

BeMacized added 2 commits December 7, 2021 21:34

Fix readme

e323029

Remove redundant dart doc

8a0ec19

stuartmorgan-g mentioned this pull request Dec 7, 2021

[webview_flutter] Proof of concept web implementation #4594

Merged

11 tasks

stuartmorgan-g closed this Dec 8, 2021

Conversation

BeMacized commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ditman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ditman Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

ditman Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

BeMacized Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

ditman Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

BeMacized Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

ditman Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

BeMacized Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g commented Dec 6, 2021

Uh oh!

flutter-dashboard bot commented Dec 6, 2021

Uh oh!

ditman commented Dec 7, 2021

Uh oh!

ditman commented Dec 7, 2021

Uh oh!

BeMacized commented Dec 7, 2021

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g commented Dec 7, 2021

Uh oh!

ditman commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Dec 7, 2021

Uh oh!

ditman commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Dec 7, 2021

Uh oh!

stuartmorgan-g commented Dec 7, 2021

Uh oh!

BeMacized commented Dec 2, 2021 •

edited

Loading

ditman left a comment •

edited

Loading

BeMacized Dec 6, 2021 •

edited

Loading

ditman Dec 7, 2021 •

edited

Loading

ditman commented Dec 7, 2021 •

edited

Loading

ditman commented Dec 7, 2021 •

edited

Loading

ditman commented Dec 7, 2021 •

edited

Loading

BeMacized commented Dec 7, 2021 •

edited

Loading