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.

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

[web] Expose PlatformLocation and HashUrlStrategy through ui_web #41163

Merged
merged 11 commits into from May 10, 2023

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Apr 13, 2023

  1. Expose PlatformLocation and its subclass BrowserPlatformLocation through the new ui_web.
  2. Expose HashUrlStrategy too. It's useful for users who want to extend and customize it instead of building their own from scratch.
  3. ui_web/url_strategy.dart => ui_web/navigation/url_strategy.dart.

Issue: flutter/flutter#126831

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Apr 13, 2023
@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

Awesome! Looks great!

abstract class UrlStrategy {
/// Abstract const constructor. This constructor enables subclasses to provide
/// const constructors so that they can be used in const expressions.
const UrlStrategy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. All methods are abstract, which tells me this class can be implemented. So we don't need a constructor. The implementation can still be const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I changed PlatformLocation to be an abstract interface (TIL about interfaces in Dart!).

That said, I can't make UrlStrategy an interface yet because it's being extended in flutter_web_plugins. I'll need to change that to implements first then come back and make UrlStrategy an interface. I'll do it in a follow up PR.

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2023
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

lgtm

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented May 10, 2023

auto label is removed for flutter/engine, pr: 41163, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2023
@auto-submit auto-submit bot merged commit a0925f1 into flutter:main May 10, 2023
30 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 11, 2023
…126486)

flutter/engine@ef771f9...a0925f1

2023-05-10 mdebbar@google.com [web] Expose PlatformLocation and HashUrlStrategy through ui_web (flutter/engine#41163)

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 aaclarke@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
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…lutter#126486)

flutter/engine@ef771f9...a0925f1

2023-05-10 mdebbar@google.com [web] Expose PlatformLocation and HashUrlStrategy through ui_web (flutter/engine#41163)

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 aaclarke@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 deleted the ui_web_platform_location branch June 22, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
3 participants