[Android] Convert `int` in Dart to `long` instead of `int` in java. #39476

0xZOne · 2023-02-08T03:15:22Z

We should use the 64-bit to represent int in Dart.

As an example of an extreme case, if |viewId| is greater than 0x80000000 on the Dart side, the native code will throw the following exception:

See the below codes for more details:
https://github.com/flutter/flutter/blob/19dfde6989b14bd7dedbc50d7edceef4e59879a4/packages/flutter/lib/src/services/message_codecs.dart#L399-L407

Related issue: flutter/flutter#120256

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 Hixie said 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.

flutter-dashboard · 2023-02-08T04:05:56Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

chinmaygarde · 2023-02-09T21:22:47Z

xref the iOS version of this patch #39331

shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java

0xZOne · 2023-02-20T04:13:06Z

shell/platform/android/io/flutter/plugin/platform/PlatformViewFactory.java

   */
  @NonNull
-  public abstract PlatformView create(Context context, int viewId, @Nullable Object args);
+  public abstract PlatformView create(Context context, long viewId, @Nullable Object args);


This is a breaking change. Any advice?

Hard-breaking every plugin that uses platform views is definitely a non-starter.

Is there some real problem that we need this change to solve? From what I see in the PR description and the issue, it's entirely a theoretical concern, rather than something that has actually caused problems. Presumably there are no Flutter apps that need more than several billion platform views over the life of the app, and I'm having a hard time imagining a scenario where some kind of non-sequential random mapping over more than the range of int would be required.

If this is all just theoretical, the easy solution would seem to be to throw if the value is too large, and document+assert on the Dart side that the value must be within the range of int.

(E.g., it's also the case that on the Dart side setSize on a platform view takes a Size, whose width and height are doubles, but I'm willing to bet that passing double.maxFinite for both would not end well. It's common for APIs to not accept every value that the types could allow for.)

Yes, This is not a problem in practice, and given the breaking change, I am also more inclined to the solution you proposed.

BTW, exposing the |viewId| to the user doesn't seem to make much sense.

I didn't realized this was breaking. Stuart's suggestion seems like it is a good idea.

I didn't realized this was breaking. Stuart's suggestion seems like it is a good idea.

Sorry, that's my mistake. The breaking change is not obvious, and I forgot to mark it at the beginning.

I have submitted a new PR. Please review it when you have time. Thank you.

shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java

0xZOne · 2023-02-21T02:29:37Z

@reidbaker Please take another look when you have a moment. Thanks.

chinmaygarde · 2023-02-23T21:04:32Z

Can we land this? @0xZOne is a part of the Flutter org so the presub requirements are satisfied.

stuartmorgan-g

The current version still has a breaking change to PlatformViewFactory, so can't be landed as is. Setting review state to capture that.

0xZOne · 2023-02-27T15:20:02Z

Closing it as I have filed a new PR based on the review results.

flutter-dashboard bot added the platform-android label Feb 8, 2023

0xZOne marked this pull request as draft February 8, 2023 04:00

0xZOne force-pushed the task/type_mismatch branch 10 times, most recently from e0fcb50 to 01bdf81 Compare February 9, 2023 06:56

0xZOne marked this pull request as ready for review February 9, 2023 07:48

0xZOne requested a review from stuartmorgan-g February 9, 2023 07:48

0xZOne mentioned this pull request Feb 15, 2023

Type mismatch may cause a loss of precision when passing int of Dart to native flutter/flutter#120256

Open

4 tasks

camsim99 requested a review from reidbaker February 16, 2023 19:42

This was referenced Feb 16, 2023

Uses int64_t instead of int for the |view_id| parameter. #39618

Merged

[Android][iOS] Write linter for checking platform view viewId type flutter/flutter#120917

Open

reidbaker reviewed Feb 17, 2023

View reviewed changes

shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java Show resolved Hide resolved

reidbaker approved these changes Feb 17, 2023

View reviewed changes

shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java Show resolved Hide resolved

0xZOne force-pushed the task/type_mismatch branch from c324b51 to 78b4ad9 Compare February 20, 2023 03:05

0xZOne commented Feb 20, 2023

View reviewed changes

0xZOne requested a review from reidbaker February 20, 2023 04:15

0xZOne added 2 commits February 20, 2023 12:29

[Android] Convert int in Dart to long instead of int in java.

a081806

Makes | platformViewId | large enough to require a long.

ccfedc8

0xZOne force-pushed the task/type_mismatch branch from 78b4ad9 to ccfedc8 Compare February 20, 2023 04:32

reidbaker approved these changes Feb 21, 2023

View reviewed changes

0xZOne mentioned this pull request Feb 22, 2023

Limit the PlatformView ID within the range of 32-bit integers. flutter/flutter#121203

Merged

8 tasks

stuartmorgan-g suggested changes Feb 27, 2023

View reviewed changes

0xZOne closed this Feb 27, 2023

[Android] Convert int in Dart to long instead of int in java. #39476

[Android] Convert int in Dart to long instead of int in java. #39476

Uh oh!

Conversation

0xZOne commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 8, 2023

Uh oh!

chinmaygarde commented Feb 9, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

Uh oh!

0xZOne commented Feb 21, 2023

Uh oh!

chinmaygarde commented Feb 23, 2023

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

0xZOne commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Android] Convert `int` in Dart to `long` instead of `int` in java. #39476

[Android] Convert `int` in Dart to `long` instead of `int` in java. #39476

0xZOne commented Feb 8, 2023 •

edited

Loading