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

[rfw] Support web (as JS) #4650

Merged
merged 1 commit into from
Aug 23, 2023
Merged

[rfw] Support web (as JS) #4650

merged 1 commit into from
Aug 23, 2023

Conversation

Hixie
Copy link
Contributor

@Hixie Hixie commented Aug 4, 2023

Pre-launch Checklist

Fixes flutter/flutter#129843

  • 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 relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@github-actions github-actions bot added the p: rfw Remote Flutter Widgets label Aug 4, 2023
@Hixie Hixie force-pushed the rfwweb branch 4 times, most recently from f2c41dc to af60c5e Compare August 7, 2023 18:57
@Hixie Hixie requested a review from stuartmorgan August 14, 2023 21:16
Copy link
Contributor

@stuartmorgan stuartmorgan 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 some optional nits.


* Improved web compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Improves

(Per style guide linked from the checklist.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -264,6 +276,10 @@ RemoteWidgetLibrary decodeLibraryBlob(Uint8List bytes) {
// endianess used by this format
const Endian _blobEndian = Endian.little;

// whether we can use 64 bit APIs on this platform
// (on JS, we can only use 32 bit APIs and integers only go up to ~2^53)
const bool _kHas64Bits = 0x1000000000000000 + 1 != 0x1000000000000000; // 2^60
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Effective Dart says not to use k, and Flutter style only says to use it for global constants

(I would strongly vote for changing the Flutter style guide to remove the _k... example and change "It’s not necessary to add the k prefix to non-global constants." to explicitly forbid it, to reduce pointless diff from Effective Dart.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

(go for it!)

// yet rolled to stable. Avoid using this to skip tests of _RFW_ features that
// aren't compatible with stable. Those should wait until the stable release
// channel is updated so that RFW can be compatible with it.
bool get isMainChannel {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the Flutter channel renaming actually happening soon? If not, this seems like confusing naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

master and main are synonyms now for flutter/flutter.

@Hixie
Copy link
Contributor Author

Hixie commented Aug 17, 2023

updated per comment, will autoland on green. Feel free to remove the autosubmit label if you want additional changes (e.g. if you want the main/master thing changed).

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 17, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 17, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 17, 2023

auto label is removed for flutter/packages/4650, due to Pull request flutter/packages/4650 is not in a mergeable state.

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2023
@auto-submit auto-submit bot merged commit 3060b1a into flutter:main Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 23, 2023
flutter/packages@c730a90...3060b1a

2023-08-23 [email protected] [rfw] Support web (as JS) (flutter/packages#4650)
2023-08-22 [email protected] [webview_flutter] Update sample code. (flutter/packages#4727)
2023-08-22 [email protected] [flutter_adaptive_scaffold] Fix top padding for NavigationBar (flutter/packages#4661)
2023-08-22 [email protected] Remove deprecated `ImageProvider` methods (flutter/packages#4725)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [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://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
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 p: rfw Remote Flutter Widgets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[rfw] Many unit test failures in browser mode
2 participants