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

[web] Do not wipe the PlatformViewManager when disposing of a view. #49991

Merged
merged 4 commits into from
Jan 26, 2024

Conversation

ditman
Copy link
Member

@ditman ditman commented Jan 24, 2024

When a view gets disposed of, its rasterizer completely clears up the singleton PlatformViewManager. Particularly, it removes all registered platform view factories.

This is wrong because the remaining PlatformViews on the page cannot be re-rendered, and the default Platform View factories (used by pointer_interceptor, for example), disappear.

This PR attempts to preserve the dispose logic of the canvaskit rasterizer, without using the debugClear method of the PlatformViewManager (which is supposedly test-only).

Issues

Tests

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Jan 24, 2024
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM with the small suggestion of using Iterable<int> instead of Set<int>.

The HtmlViewEmbedder has enough information to dispose all the views it
knows of (the keys of _viewClipChains), so there's no need to call the
PlatformViewManager to ask for ALL the views in the app.
@ditman ditman force-pushed the web-remove-platform-views-uncaught branch from 4020f0e to 4217b9c Compare January 26, 2024 03:34
@ditman ditman requested a review from mdebbar January 26, 2024 03:47
@ditman
Copy link
Member Author

ditman commented Jan 26, 2024

PTAL @mdebbar, I implemented the separation that you suggested!

Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this!

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 26, 2024
@auto-submit auto-submit bot merged commit 70039d3 into flutter:main Jan 26, 2024
@ditman ditman deleted the web-remove-platform-views-uncaught branch January 26, 2024 20:25
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 26, 2024
…142347)

flutter/engine@525bd7d...a65a1b5

2024-01-26 [email protected] Roll Skia from c32aa37effcc to 6279c88b9e29 (1 revision) (flutter/engine#50098)
2024-01-26 [email protected] [Impeller] add compute pass API for memory barriers, re-enable for Vulkan. (flutter/engine#49946)
2024-01-26 [email protected] Introduce a prototype of a "header guard enforcement" tool (flutter/engine#48903)
2024-01-26 [email protected] Roll Skia from e24124912cc3 to c32aa37effcc (1 revision) (flutter/engine#50094)
2024-01-26 [email protected] [web] Do not wipe the PlatformViewManager when disposing of a view. (flutter/engine#49991)
2024-01-26 [email protected] Finish landing missing/incorrect header guards across `flutter/engine` (flutter/engine#50069)
2024-01-26 [email protected] Roll Skia from 32f6bff0f193 to e24124912cc3 (2 revisions) (flutter/engine#50093)
2024-01-26 [email protected] Roll Skia from cbdf09d69efc to 32f6bff0f193 (3 revisions) (flutter/engine#50092)
2024-01-26 [email protected] Roll Dart SDK from 5636e338e0b9 to 7337995bc851 (1 revision) (flutter/engine#50087)
2024-01-26 [email protected] Fix Shell::Screenshot for Impeller (flutter/engine#50072)
2024-01-26 [email protected] Roll Skia from ae73baacb793 to cbdf09d69efc (1 revision) (flutter/engine#50085)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[web] unregistered_view_type PlatformException after removing/adding view with platform views
2 participants