Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[webview_flutter] Calling destroy on webview when flutter part is getting disposed #1886

Merged
amirh merged 8 commits into from
Aug 15, 2019
Merged

[webview_flutter] Calling destroy on webview when flutter part is getting disposed #1886

amirh merged 8 commits into from
Aug 15, 2019

Conversation

@charafau
Copy link
Contributor

@charafau charafau commented Jul 21, 2019

Description

Webview wasn't properly disposed on android

Related Issues

fixes: flutter/flutter#33299

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@charafau charafau requested a review from amirh as a code owner July 21, 2019 01:43
Liangdi
Liangdi reviewed Jul 21, 2019
View reviewed changes
packages/webview_flutter/CHANGELOG.md Outdated
## 0.3.10+3
## 0.3.10+4

* Don't log an unknown setting key error for 'debuggingEnabled' on iOS.
Copy link

@Liangdi Liangdi Jul 21, 2019

Choose a reason for hiding this comment

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

the wrong logs

Copy link
Contributor Author

@charafau charafau Jul 21, 2019

Choose a reason for hiding this comment

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

thanks for noticing, I missed that after rebasing with master.

amirh
amirh suggested changes Jul 22, 2019
View reviewed changes
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

Thanks!

Overall looks good, we need to add an e2e test that verifies this fix in https://github.com/flutter/plugins/blob/master/packages/webview_flutter/example/test_driver/webview.dart before we can land this.

packages/webview_flutter/CHANGELOG.md

## 0.3.10+3

* Don't log an unknown setting key error for 'debuggingEnabled' on iOS.
Copy link
Contributor

@amirh amirh Jul 22, 2019

Choose a reason for hiding this comment

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

please revert

Copy link
Contributor Author

@charafau charafau Jul 23, 2019

Choose a reason for hiding this comment

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

Sorry about confusion with changelog. I added * to that.

@charafau
Copy link
Contributor Author

charafau commented Aug 11, 2019

@amirh I just noticed that you wanted to put some e2e test here. Not sure how I could test that. Any tips/ideas?

@charafau
Copy link
Contributor Author

charafau commented Aug 15, 2019

as far as I know there's no way of checking if sound still plays in webview.

@amirh
Copy link
Contributor

amirh commented Aug 15, 2019

@amirh I just noticed that you wanted to put some e2e test here. Not sure how I could test that. Any tips/ideas?

You're right, I don't see an obvious way to test this, especially as we've lost the reference to the Android WebView after the platform view has been disposed.

Sorry about that.

amirh
amirh approved these changes Aug 15, 2019
View reviewed changes
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

LGTM

Just need to merge/rebase before landing.

@charafau
Copy link
Contributor Author

charafau commented Aug 15, 2019

Got it, thanks! let me rebase and let's merge.

@amirh amirh merged commit be0cfa3 into flutter:master Aug 15, 2019
@Brooooooklyn
Copy link

Brooooooklyn commented Sep 19, 2019

Why not the same logic in ios? I notice there are many of com.apple.WebKit.WebContent processes in my Mac Activity monitor while I was developing.

sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
@charafau
[webview_flutter] Calling destroy on webview when flutter part is get…
36acb57 
…ting disposed (flutter#1886)

fixes: flutter/flutter#33299
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
@rodruiz
[firebase_core, firebase_analytics, firebase_crashlytics] fix overrid…
e1523ab 
…es a deprecated API (flutter#1886)

* ignore .flutter-plugins-dependencies

* Core, Analytics, Crashlytics fix uses or overrides a deprecated API

* update pubspec.yaml and CHANGELOG.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@Liangdi Liangdi Liangdi left review comments

@amirh amirh amirh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<webview_flutter> Page destruction, audio is still playing

5 participants

@charafau @amirh @Brooooooklyn @Liangdi @googlebot