Skip to content

[webview_flutter] Enable warnings-as-errors on Android #3356

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 4, 2023
Merged

[webview_flutter] Enable warnings-as-errors on Android #3356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4526a8
Update Pigeon
stuartmorgan-g Mar 2, 2023
9fdff80
Address warnings
stuartmorgan-g Mar 2, 2023
61aa39e
Remove TODO
stuartmorgan-g Mar 2, 2023
57e862e
Name update
stuartmorgan-g Mar 2, 2023
dfb307c
Regenerate mocks
stuartmorgan-g Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/webview_flutter/webview_flutter_android/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## NEXT
## 3.3.2

* Resolves compilations warnings.
* Updates compileSdkVersion to 33.

## 3.3.1
Expand Down
22 changes: 17 additions & 5 deletions ...oid/android/src/main/java/io/flutter/plugins/webviewflutter/CookieManagerHostApiImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,28 @@ public void clearCookies(GeneratedAndroidWebView.Result<Boolean> result) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
cookieManager.removeAllCookies(result::success);
} else {
final boolean hasCookies = cookieManager.hasCookies();
if (hasCookies) {
cookieManager.removeAllCookie();
}
result.success(hasCookies);
result.success(removeCookiesPreL(cookieManager));
}
}

@Override
public void setCookie(String url, String value) {
CookieManager.getInstance().setCookie(url, value);
}

/**
* Removes all cookies from the given cookie manager, using the deprecated (pre-Lollipop)
* implementation.
*
* @param cookieManager The cookie manager to clear all cookies from.
* @return Whether any cookies were removed.
*/
@SuppressWarnings("deprecation")
private boolean removeCookiesPreL(CookieManager cookieManager) {
final boolean hasCookies = cookieManager.hasCookies();
if (hasCookies) {
cookieManager.removeAllCookie();
}
return hasCookies;
}
}
201 changes: 97 additions & 104 deletions ...roid/android/src/main/java/io/flutter/plugins/webviewflutter/GeneratedAndroidWebView.java
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions ...d/android/src/main/java/io/flutter/plugins/webviewflutter/WebChromeClientHostApiImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ public boolean shouldOverrideUrlLoading(
return true;
}

// Legacy codepath for < N.
@SuppressWarnings("deprecation")
@Override
public boolean shouldOverrideUrlLoading(WebView windowWebView, String url) {
if (!webViewClient.shouldOverrideUrlLoading(view, url)) {
Expand Down
8 changes: 8 additions & 0 deletions ...oid/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewClientHostApiImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ public void onReceivedError(WebView view, WebResourceRequest request, WebResourc
flutterApi.onReceivedRequestError(this, view, request, error, reply -> {});
}

// Legacy codepath for < 23; newer versions use the variant above.
@SuppressWarnings("deprecation")
@Override
public void onReceivedError(
WebView view, int errorCode, String description, String failingUrl) {
Expand All @@ -72,6 +74,8 @@ public boolean shouldOverrideUrlLoading(WebView view, WebResourceRequest request
return returnValueForShouldOverrideUrlLoading;
}

// Legacy codepath for < 24; newer versions use the variant above.
@SuppressWarnings("deprecation")
@Override
public boolean shouldOverrideUrlLoading(WebView view, String url) {
flutterApi.urlLoading(this, view, url, reply -> {});
Expand Down Expand Up @@ -125,6 +129,8 @@ public void onReceivedError(
flutterApi.onReceivedRequestError(this, view, request, error, reply -> {});
}

// Legacy codepath for versions that don't support the variant above.
@SuppressWarnings("deprecation")
@Override
public void onReceivedError(
WebView view, int errorCode, String description, String failingUrl) {
Expand All @@ -140,6 +146,8 @@ public boolean shouldOverrideUrlLoading(
return returnValueForShouldOverrideUrlLoading;
}

// Legacy codepath for < Lollipop; newer versions use the variant above.
@SuppressWarnings("deprecation")
@Override
public boolean shouldOverrideUrlLoading(WebView view, String url) {
flutterApi.urlLoading(this, view, url, reply -> {});
Expand Down
7 changes: 6 additions & 1 deletion ...android/src/test/java/io/flutter/plugins/webviewflutter/CookieManagerHostApiImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ public void setup() {
when(cookieManager.hasCookies()).thenReturn(true);
doAnswer(
answer -> {
((ValueCallback<Boolean>) answer.getArgument(0)).onReceiveValue(true);
@SuppressWarnings("unchecked")
ValueCallback<Boolean> callback = (ValueCallback<Boolean>) answer.getArgument(0);
(callback).onReceiveValue(true);
return null;
})
.when(cookieManager)
Expand All @@ -59,6 +61,7 @@ public void setCookieShouldCallSetCookie() {
public void clearCookiesShouldCallRemoveAllCookiesOnAndroidLAbove() {
// Setup
TestUtils.setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.LOLLIPOP);
@SuppressWarnings("unchecked")
GeneratedAndroidWebView.Result<Boolean> result = mock(GeneratedAndroidWebView.Result.class);
CookieManagerHostApiImpl impl = new CookieManagerHostApiImpl();
// Run
Expand All @@ -69,9 +72,11 @@ public void clearCookiesShouldCallRemoveAllCookiesOnAndroidLAbove() {
}

@Test
@SuppressWarnings("deprecation")
public void clearCookiesShouldCallRemoveAllCookieBelowAndroidL() {
// Setup
TestUtils.setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.KITKAT_WATCH);
@SuppressWarnings("unchecked")
GeneratedAndroidWebView.Result<Boolean> result = mock(GeneratedAndroidWebView.Result.class);
CookieManagerHostApiImpl impl = new CookieManagerHostApiImpl();
// Run
Expand Down
6 changes: 5 additions & 1 deletion ...ndroid/android/src/test/java/io/flutter/plugins/webviewflutter/JavaScriptChannelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static org.mockito.Mockito.verify;

import android.os.Handler;
import android.os.Looper;
import io.flutter.plugins.webviewflutter.JavaScriptChannelHostApiImpl.JavaScriptChannelCreator;
import org.junit.After;
import org.junit.Before;
Expand Down Expand Up @@ -47,7 +48,10 @@ public JavaScriptChannel createJavaScriptChannel(

hostApiImpl =
new JavaScriptChannelHostApiImpl(
instanceManager, javaScriptChannelCreator, mockFlutterApi, new Handler());
instanceManager,
javaScriptChannelCreator,
mockFlutterApi,
new Handler(Looper.myLooper()));
hostApiImpl.create(0L, "aChannelName");
}

Expand Down
12 changes: 6 additions & 6 deletions ...oid/src/test/java/io/flutter/plugins/webviewflutter/RegistrarFlutterAssetManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
import static org.mockito.Mockito.when;

import android.content.res.AssetManager;
import io.flutter.plugin.common.PluginRegistry.Registrar;
import io.flutter.plugins.webviewflutter.FlutterAssetManager.RegistrarFlutterAssetManager;
import java.io.IOException;
import org.junit.Before;
import org.junit.Test;
Expand All @@ -21,17 +19,19 @@
@SuppressWarnings("deprecation")
public class RegistrarFlutterAssetManagerTest {
@Mock AssetManager mockAssetManager;
@Mock Registrar mockRegistrar;
@Mock io.flutter.plugin.common.PluginRegistry.Registrar mockRegistrar;

RegistrarFlutterAssetManager testRegistrarFlutterAssetManager;
io.flutter.plugins.webviewflutter.FlutterAssetManager.RegistrarFlutterAssetManager
testRegistrarFlutterAssetManager;

@Before
public void setUp() {
mockAssetManager = mock(AssetManager.class);
mockRegistrar = mock(Registrar.class);
mockRegistrar = mock(io.flutter.plugin.common.PluginRegistry.Registrar.class);

testRegistrarFlutterAssetManager =
new RegistrarFlutterAssetManager(mockAssetManager, mockRegistrar);
new io.flutter.plugins.webviewflutter.FlutterAssetManager.RegistrarFlutterAssetManager(
mockAssetManager, mockRegistrar);
}

@Test
Expand Down
4 changes: 1 addition & 3 deletions packages/webview_flutter/webview_flutter_android/example/android/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ task clean(type: Delete) {
gradle.projectsEvaluated {
project(":webview_flutter_android") {
tasks.withType(JavaCompile) {
// TODO(stuartmorgan): Enable this. See
// https://github.com/flutter/flutter/issues/91868
//options.compilerArgs << "-Xlint:all" << "-Werror"
options.compilerArgs << "-Xlint:all" << "-Werror"
}
}
}
7 changes: 2 additions & 5 deletions packages/webview_flutter/webview_flutter_android/lib/src/android_webview.g.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Autogenerated from Pigeon (v4.2.14), do not edit directly.
// Autogenerated from Pigeon (v9.0.4), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import

import 'dart:async';
import 'dart:typed_data' show Float64List, Int32List, Int64List, Uint8List;

Expand Down Expand Up @@ -297,7 +298,6 @@ class _WebViewHostApiCodec extends StandardMessageCodec {
switch (type) {
case 128:
return WebViewPoint.decode(readValue(buffer)!);

default:
return super.readValueOfType(type, buffer);
}
Expand Down Expand Up @@ -1394,10 +1394,8 @@ class _WebViewClientFlutterApiCodec extends StandardMessageCodec {
switch (type) {
case 128:
return WebResourceErrorData.decode(readValue(buffer)!);

case 129:
return WebResourceRequestData.decode(readValue(buffer)!);

default:
return super.readValueOfType(type, buffer);
}
Expand Down Expand Up @@ -1935,7 +1933,6 @@ class _FileChooserParamsFlutterApiCodec extends StandardMessageCodec {
switch (type) {
case 128:
return FileChooserModeEnumData.decode(readValue(buffer)!);

default:
return super.readValueOfType(type, buffer);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/webview_flutter/webview_flutter_android/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: webview_flutter_android
description: A Flutter plugin that provides a WebView widget on Android.
repository: https://github.com/flutter/packages/tree/main/packages/webview_flutter/webview_flutter_android
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+webview%22
version: 3.3.1
version: 3.3.2

environment:
sdk: ">=2.17.0 <3.0.0"
Expand All @@ -29,4 +29,4 @@ dev_dependencies:
flutter_test:
sdk: flutter
mockito: ^5.3.2
pigeon: ^4.2.14
pigeon: ^9.0.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are updating the pigeon version, could you try and regenerate the mocks as well. I tried raising the pigeon version too, but it seems the new analyzer was causing mockito to generate bad mocks. I want to see if you run into the same problem or if it was specific to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not just you; the Future<Offset> and Future<Size> methods are wrong. I'm trying to make a reduced test case to file an issue, but a trivial class with a Future<Offset> works, so it seems like there's something more complicated going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No luck so far. Maybe we should just file it with webview_flutter as-is, and hope someone who knows the internals better can figure out what's going on more easily that I can figure out what the minimal repro is.

How do you want to handle this in the meantime? Hold this PR, or manually fix it in the mocks with a TODO and issue link?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible just to revert the version bump to pigeon? Then this PR should be good.

Copy link
Contributor

@bparrishMines bparrishMines Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

+1 to submitting an issue with webview_flutter as-is. I think we can just give them the android_webview_test.dart with the android_webview.dart and say that the mockito outputs are incorrect. I can also handle making the issue if you would like.

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 filed dart-lang/mockito#612, it turns out it's a dependency issue that doesn't reproduce if you clean or upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for filing it!

10 changes: 8 additions & 2 deletions ...s/webview_flutter/webview_flutter_android/test/android_webview_controller_test.mocks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,12 @@ class MockAndroidNavigationDelegate extends _i1.Mock
class MockAndroidWebViewController extends _i1.Mock
implements _i8.AndroidWebViewController {
@override
int get webViewIdentifier => (super.noSuchMethod(
Invocation.getter(#webViewIdentifier),
returnValue: 0,
returnValueForMissingStub: 0,
) as int);
@override
_i3.PlatformWebViewControllerCreationParams get params => (super.noSuchMethod(
Invocation.getter(#params),
returnValue: _FakePlatformWebViewControllerCreationParams_4(
Expand Down Expand Up @@ -650,11 +656,11 @@ class MockAndroidWebViewController extends _i1.Mock
@override
_i9.Future<void> setOnShowFileSelector(
_i9.Future<List<String>> Function(_i8.FileSelectorParams)?
onShowFileSelectorCallback) =>
onShowFileSelector) =>
(super.noSuchMethod(
Invocation.method(
#setOnShowFileSelector,
[onShowFileSelectorCallback],
[onShowFileSelector],
),
returnValue: _i9.Future<void>.value(),
returnValueForMissingStub: _i9.Future<void>.value(),
Expand Down
3 changes: 1 addition & 2 deletions packages/webview_flutter/webview_flutter_android/test/test_android_webview.g.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Autogenerated from Pigeon (v4.2.14), do not edit directly.
// Autogenerated from Pigeon (v9.0.4), do not edit directly.
// See also: https://pub.dev/packages/pigeon
// ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, unnecessary_import
// ignore_for_file: avoid_relative_lib_imports
Expand Down Expand Up @@ -64,7 +64,6 @@ class _TestWebViewHostApiCodec extends StandardMessageCodec {
switch (type) {
case 128:
return WebViewPoint.decode(readValue(buffer)!);

default:
return super.readValueOfType(type, buffer);
}
Expand Down