-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[webview_flutter] Add support for payment requests on Android #9679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[webview_flutter] Add support for payment requests on Android #9679
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This PR adds support for checking and enabling the Payment Request API on Android. The changes look good overall, with new APIs on AndroidWebViewController
and corresponding native implementations and tests. I've left a few comments to improve documentation, example code clarity, and changelog formatting to better align with the project's standards. Once these are addressed, this should be ready to merge.
@@ -1,3 +1,7 @@ | |||
## 4.9.0 | |||
|
|||
* Adds support to check payment request is supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changelog entry could be more descriptive and follow the repository's style guide more closely (e.g., complete sentence, ending with a period, using code formatting for API names).
* Adds support to check payment request is supported. | |
* Adds support for the Payment Request API with `AndroidWebViewController.isWebViewFeatureSupported` and `AndroidWebViewController.setPaymentRequestEnabled`. |
final bool paymentRequestEnabled = await androidController | ||
.isWebViewFeatureSupported(WebViewFeatureType.paymentRequest); | ||
|
||
// Call this method to enable the Payment Request API. | ||
// if (paymentRequestEnabled) { | ||
// await androidController.setPaymentRequestEnabled(true); | ||
// } | ||
|
||
if (context.mounted) { | ||
ScaffoldMessenger.of(context).showSnackBar(SnackBar( | ||
content: Text('Payment Request API enabled: $paymentRequestEnabled'), | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name paymentRequestEnabled
is a bit misleading as it stores the result of isWebViewFeatureSupported
. A name like isPaymentRequestSupported
would be clearer. Similarly, the snackbar message says 'enabled' but it's showing whether the feature is 'supported'. This could be confusing for developers looking at the example.
final bool isPaymentRequestSupported = await androidController
.isWebViewFeatureSupported(WebViewFeatureType.paymentRequest);
// Call this method to enable the Payment Request API.
// if (isPaymentRequestSupported) {
// await androidController.setPaymentRequestEnabled(true);
// }
if (context.mounted) {
ScaffoldMessenger.of(context).showSnackBar(SnackBar(
content: Text('Payment Request API supported: $isPaymentRequestSupported'),
));
@@ -909,6 +938,12 @@ enum MixedContentMode { | |||
neverAllow, | |||
} | |||
|
|||
/// See https://developer.android.com/reference/androidx/webkit/WebViewFeature#constants_1. | |||
enum WebViewFeatureType { | |||
/// Feature for isFeatureSupported. This feature covers setPaymentRequestEnabled, getPaymentRequestEnabled, setHasEnrolledInstrumentEnabled, and getHasEnrolledInstrumentEnabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is very long and could be wrapped to adhere to the 80-character line limit. Also, it mentions methods like getPaymentRequestEnabled
which are not exposed in this PR, which could be confusing. Consider rephrasing to be more concise and accurate to what is currently exposed, for example, by linking to the setPaymentRequestEnabled
method.
/// This feature covers [setPaymentRequestEnabled].
…led if the feature PAYMENT_REQUEST is present Ensures the function works correctly even when called without prior WebViewFeature.isFeatureSupported check. ref. https://developers.google.com/pay/api/android/guides/recipes/using-android-webview#enable-payment-request-api
f79ef2c
to
8c73517
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! This looks good, just a handful of comments about documentation.
/** | ||
* Host api implementation for {@link WebSettingsCompat}. | ||
* | ||
* <p>Handles static methods for {@link WebSettingsCompat}. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this was copied from the other api implementations which are actually incorrect. You can change this to:
/** | |
* Host api implementation for {@link WebSettingsCompat}. | |
* | |
* <p>Handles static methods for {@link WebSettingsCompat}. | |
*/ | |
/** | |
* Proxy API implementation for {@link WebSettingsCompat}. | |
* | |
* <p>This class may handle instantiating and adding native object instances that are attached to a | |
* Dart instance or handle method calls on the associated native class or an instance of the class. | |
*/ |
/** | ||
* Host api implementation for {@link WebViewFeature}. | ||
* | ||
* <p>Handles creating {@link WebViewFeature}s that intercommunicate with a paired Dart object. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for here:
/** | |
* Host api implementation for {@link WebViewFeature}. | |
* | |
* <p>Handles creating {@link WebViewFeature}s that intercommunicate with a paired Dart object. | |
*/ | |
/** | |
* Proxy API implementation for {@link WebViewFeature}. | |
* | |
* <p>This class may handle instantiating and adding native object instances that are attached to a | |
* Dart instance or handle method calls on the associated native class or an instance of the class. | |
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think we need to update the example app, because there is nothing to print or display to verify it is actually working. This just checks that the method is called, but this is already covered by unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. removed in 3f68c0e
/// | ||
/// Before calling this method, you should check if the feature is supported using | ||
/// [isWebViewFeatureSupported] with [WebViewFeatureConstants.paymentRequest]. | ||
Future<void> setPaymentRequestEnabled(bool enabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs show that the manifest also needs to be updated: https://developer.android.com/reference/androidx/webkit/WebSettingsCompat#setPaymentRequestEnabled(android.webkit.WebSettings,boolean)
I would also include the manifest update in the docs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback! I've also added the content of the AndroidManifest update: 28f8889
@@ -909,6 +938,12 @@ enum MixedContentMode { | |||
neverAllow, | |||
} | |||
|
|||
/// See https://developer.android.com/reference/androidx/webkit/WebViewFeature#constants_1. | |||
enum WebViewFeatureType { | |||
/// Feature for isFeatureSupported. This feature covers [WebSettingsCompat.setPaymentRequestEnabled]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Space after first sentence.
/// Feature for isFeatureSupported. This feature covers [WebSettingsCompat.setPaymentRequestEnabled]. | |
/// Feature for isFeatureSupported. | |
/// | |
/// This feature covers [WebSettingsCompat.setPaymentRequestEnabled]. |
@@ -909,6 +938,12 @@ enum MixedContentMode { | |||
neverAllow, | |||
} | |||
|
|||
/// See https://developer.android.com/reference/androidx/webkit/WebViewFeature#constants_1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would include a description like:
/// See https://developer.android.com/reference/androidx/webkit/WebViewFeature#constants_1. | |
/// WebView support library feature types used to query for support on the device. | |
/// | |
/// See https://developer.android.com/reference/androidx/webkit/WebViewFeature#constants_1. |
@ProxyApi( | ||
kotlinOptions: KotlinProxyApiOptions( | ||
fullClassName: 'androidx.webkit.WebSettingsCompat', | ||
), | ||
) | ||
abstract class WebSettingsCompat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs docs like the other proxyapis. A one-liner and a link is usually fine:
/// Compatibility version of `WebSettings`.
///
/// See https://developer.android.com/reference/kotlin/androidx/webkit/WebSettingsCompat
fullClassName: 'androidx.webkit.WebViewFeature', | ||
), | ||
) | ||
abstract class WebViewFeature { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple more comments on docs.
@stuartmorgan-g This is ready for a secondary review.
/// This method uses [android_webview.WebViewFeature.isFeatureSupported] to check | ||
/// if the specified WebView feature is available on the current device and WebView version. | ||
/// | ||
/// See [WebViewFeatureConstants] for available feature constants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant to use type here since it is apart of the exported api.
/// See [WebViewFeatureConstants] for available feature constants. | |
/// See [WebViewFeatureType] for available feature constants. |
/// to enable or disable the Payment Request API for the WebView. | ||
/// | ||
/// Before calling this method, you should check if the feature is supported using | ||
/// [isWebViewFeatureSupported] with [WebViewFeatureConstants.paymentRequest]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
/// [isWebViewFeatureSupported] with [WebViewFeatureConstants.paymentRequest]. | |
/// [isWebViewFeatureSupported] with [WebViewFeatureType.paymentRequest]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should get a short section in the package's README describing it; platform-specific extensions are non-obvious, so we should highlight them in the README in general.
|
||
@Override | ||
public void setPaymentRequestEnabled(@NonNull WebSettings webSettings, boolean enabled) { | ||
if (WebViewFeature.isFeatureSupported(WebViewFeature.PAYMENT_REQUEST)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this check being done internally on the native side, rather than being a straight pass-through? I would expect this to work the same way the SDK works, which is that clients are responsible for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartmorgan-g This was to address Android lint's RequiresFeature, but as you pointed out, since it's natural to use it after checking WebViewFeature.isFeatureSupported
, it was indeed excessive checking. Therefore, I will address it with SuppressLint
.
Expose android native WebViewFeature#isFeatureSupported, WebSettingsCompat#setPaymentRequestEnabled and WebViewFeature#PAYMENT_REQUEST to allow developers to check if payment request feature is supported in WebView.
This change is for Google Pay integration in Android WebView.
Fixes flutter/flutter#172150
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3