Skip to content
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

Jump to bottom

[web] Move platformViewRegistry to ui_web #41877

Merged
merged 10 commits into from May 23, 2023
Merged

[web] Move platformViewRegistry to ui_web #41877

merged 10 commits into from May 23, 2023

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented May 9, 2023 •
edited

In this PR, I'm leaving a getter for platformViewRegistry in dart:ui for backwards compatibility. At some point, we need to properly deprecate it (e.g. print a warning asking users to import it from dart:ui_web instead).

Next step is to migrate all call sites in flutter/flutter, flutter/packages and flutter/devtools (anything else?).

Issue: flutter/flutter#126831

Fixes flutter/flutter#41563

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label May 9, 2023
@ditman
Copy link
Member

ditman commented May 9, 2023

anything else?

We need to follow the flutter deprecation policy, because this used to be a public API, and we know for a fact that people have used it on their own apps to define their custom platform views.

I don't think we own anything else that uses this. Maybe G3 has some internal usages too?

ditman
ditman reviewed May 9, 2023
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

This is fantastic! Small question about imports organization (this is a new package, so we can have whatever library structure we want within it.)

lib/web_ui/lib/ui.dart Show resolved Hide resolved
@mdebbar
Copy link
Contributor Author

mdebbar commented May 10, 2023

We need to follow the flutter deprecation policy, because this used to be a public API, and we know for a fact that people have used it on their own apps to define their custom platform views.

As I mentioned in the PR description, I left a getter for platformViewRegistry in dart:ui so there's no breaking change in this PR. But we need to start planning the deprecation. I think printing a warning is a good first step while we continue migrating the projects that we own. What do you think?

@mdebbar mdebbar requested a review from ditman May 15, 2023 14:23
ditman
ditman approved these changes May 19, 2023
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

LGTM!

But we need to start planning the deprecation. I think printing a warning is a good first step while we continue migrating the projects that we own. What do you think?

SGTM!

Please consider tackling the webOnlyAssetManager next, that would allow to migrate all of flutter/packages out of this! :P

This was referenced May 19, 2023
Closed
Closed
@mdebbar mdebbar mentioned this pull request May 23, 2023
Open
@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label May 23, 2023
@auto-submit auto-submit bot merged commit f37e78c into flutter:main May 23, 2023
29 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 23, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 23, 2023
@engine-flutter-autoroll
f37e78ca2 [web] Move platformViewRegistry to ui_web (flutter/engine#4…
e446234 
…1877)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 23, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 23, 2023
@engine-flutter-autoroll
f37e78ca2 [web] Move platformViewRegistry to ui_web (flutter/engine#4…
4a78b4c 
…1877)
zanderso pushed a commit to flutter/flutter that referenced this pull request May 23, 2023
@engine-flutter-autoroll
Roll Flutter Engine from a6e37bde65fd to ccf50f344d5d (7 revisions) (#…
3518d69 
…127416)

flutter/engine@a6e37bd...ccf50f3

2023-05-23 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from
Zbd3haNY2Idcdu1Fj... to -dXJ_pnUVwmjscIYm... (flutter/engine#42250)
2023-05-23 30870216+gaaclarke@users.noreply.github.com [Impeller]
Removed last ivars to raw vk::Device's. (flutter/engine#42215)
2023-05-23 skia-flutter-autoroll@skia.org Roll Skia from 225d51031ad0 to
292879d286e9 (5 revisions) (flutter/engine#42249)
2023-05-23 zanderso@users.noreply.github.com Build arm64
FlutterEmbedder.framework and lipo the result (flutter/engine#42233)
2023-05-23 aam@google.com Roll watcher dependency for web_ui to 1.1.0.
(flutter/engine#42248)
2023-05-23 mdebbar@google.com [web] Move platformViewRegistry to ui_web
(flutter/engine#41877)
2023-05-23 skia-flutter-autoroll@skia.org Roll Skia from 3ded22186598 to
225d51031ad0 (1 revision) (flutter/engine#42246)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Zbd3haNY2Idc to -dXJ_pnUVwmj

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 bdero@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@mdebbar mdebbar mentioned this pull request May 24, 2023
Merged
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
@engine-flutter-autoroll
Roll Flutter Engine from a6e37bde65fd to ccf50f344d5d (7 revisions) (f…
b25b3ee 
…lutter#127416)

flutter/engine@a6e37bd...ccf50f3

2023-05-23 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from
Zbd3haNY2Idcdu1Fj... to -dXJ_pnUVwmjscIYm... (flutter/engine#42250)
2023-05-23 30870216+gaaclarke@users.noreply.github.com [Impeller]
Removed last ivars to raw vk::Device's. (flutter/engine#42215)
2023-05-23 skia-flutter-autoroll@skia.org Roll Skia from 225d51031ad0 to
292879d286e9 (5 revisions) (flutter/engine#42249)
2023-05-23 zanderso@users.noreply.github.com Build arm64
FlutterEmbedder.framework and lipo the result (flutter/engine#42233)
2023-05-23 aam@google.com Roll watcher dependency for web_ui to 1.1.0.
(flutter/engine#42248)
2023-05-23 mdebbar@google.com [web] Move platformViewRegistry to ui_web
(flutter/engine#41877)
2023-05-23 skia-flutter-autoroll@skia.org Roll Skia from 3ded22186598 to
225d51031ad0 (1 revision) (flutter/engine#42246)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Zbd3haNY2Idc to -dXJ_pnUVwmj

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 bdero@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 24, 2023
@mdebbar
[web] ui.platformViewRegistry => ui_web.platformViewRegistry (#127493)
2d142c5 
Now that `platformViewRegistry` is [exposed](flutter/engine#41877) through `dart:ui_web`, we can do some cleanup here.

Part of #126831
@mdebbar mdebbar mentioned this pull request May 25, 2023
Closed
2 tasks
@mdebbar mdebbar deleted the platform_view_registry branch June 22, 2023 21:38
@keepgagaga keepgagaga mentioned this pull request Jul 10, 2023
Open
@LuchoTurtle LuchoTurtle mentioned this pull request Aug 22, 2023
Closed
@alexmercerind alexmercerind mentioned this pull request Nov 10, 2023
Merged
@ztaylor54 ztaylor54 mentioned this pull request Feb 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ditman ditman ditman approved these changes

@harryterkelsen harryterkelsen Awaiting requested review from harryterkelsen

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Milestone
No milestone
2 participants
@mdebbar @ditman