Consolidate method signature for `PlatformDispatcher.onError` #34428

ueman · 2022-07-01T18:08:54Z

I'm currently writing an integration which reports PlatformDispatcher.onError errors directly to Sentry, see getsentry/sentry-dart#915.
During that process, I've noticed that the method signatures differ between web and Dart:IO.
I'm pretty sure the method signature should be the same on both platforms.

On dart:ui it's

engine/lib/ui/platform_dispatcher.dart

Line 64 in 28a5d3c

typedef ErrorCallback = bool Function(Object exception, StackTrace stackTrace);

and on dart:web_ui it's

engine/lib/web_ui/lib/platform_dispatcher.dart

Line 17 in 28a5d3c

    
           typedef ErrorCallback = bool Function(Object exception, StackTrace? stackTrace);

This PR consolidates the callback to typedef ErrorCallback = bool Function(Object exception, StackTrace stackTrace);.

I guess this should have a test, but I'm not sure what the right approach for that is. @dnfield do you have an idea, since you've written the original implementation?

List which issues are fixed by this PR. You must list at least one issue.
This helps with getsentry/sentry-dart#915. The issue this PR fixes is not really complicated, so I hope that's enough. Otherwise, I'm happy to create an issue.

This also fixes flutter/flutter#107035

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
No, this is not a breaking change.

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 · 2022-07-01T18:08:57Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

dnfield · 2022-07-01T18:23:19Z

This was a mistake. @jonahwilliams or @yjbanov could the api conformance check catch this?

jonahwilliams · 2022-07-01T18:25:00Z

I'm not sure, I haven't touched it since pre-null safety. I would just update the web ui version. The dart:ui signature is what is used for analysis.

ueman · 2022-07-06T19:21:20Z

lib/web_ui/lib/platform_dispatcher.dart

 typedef TimingsCallback = void Function(List<FrameTiming> timings);
 typedef PointerDataPacketCallback = void Function(PointerDataPacket packet);
-typedef KeyDataCallback = bool Function(KeyData packet);
+typedef KeyDataCallback = bool Function(KeyData data);


This was caught by the new typedef conformance check

ueman · 2022-07-06T19:23:23Z

I've added a conformance check for typedef and also for nullability. Since this is my first time working with the analyzer, I'm pretty sure that this can be improved.

ueman · 2022-07-06T19:24:58Z

web_sdk/test/api_conform_test.dart

      }
    }
  }
+  print('Checking ${uiTypeDefs.length} typedefs.');


I tried to make the typedef check as similar to the method check as possible. So we check for all the same problems.

auto-submit · 2022-07-06T19:48:04Z

Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

auto-submit · 2022-07-06T19:48:05Z

Validations Fail.

jonahwilliams

LGTM

auto-submit · 2022-07-06T22:24:04Z

The status or check suite Windows Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux clang-tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Fuchsia FEMU has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Benchmarks (no-upload) has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Android Debug Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Web Framework tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Framework Smoke Tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

ueman · 2022-07-07T19:40:52Z

@dnfield friendly ping to add the autosubmit label once again. I've merged latest main into this PR and CI now seems to be happy again.

…flutter/engine#34428)

Consolidate method signature

bd4d597

flutter-dashboard bot added the platform-web Code specifically for the web engine label Jul 1, 2022

flutter-dashboard bot added the needs tests label Jul 1, 2022

This was referenced Jul 3, 2022

Feature: PlatformDispatcher.onError integration getsentry/sentry-dart#915

Merged

API conformance check has problems with nullable types flutter/flutter#107035

Closed

ueman and others added 2 commits July 6, 2022 19:02

Merge branch 'flutter:main' into patch-2

0697904

add type def conformance check

8dc9104

ueman commented Jul 6, 2022

View reviewed changes

dnfield approved these changes Jul 6, 2022

View reviewed changes

dnfield added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Jul 6, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 6, 2022

dnfield requested review from jonahwilliams and yjbanov July 6, 2022 20:05

jonahwilliams approved these changes Jul 6, 2022

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 6, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 6, 2022

ueman added 2 commits July 7, 2022 10:53

Merge branch 'flutter:main' into patch-2

d1e2575

Merge branch 'flutter:main' into patch-2

6cbe1f2

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 7, 2022

auto-submit bot merged commit 1276049 into flutter:main Jul 7, 2022

ueman deleted the patch-2 branch July 7, 2022 19:52

engine-flutter-autoroll mentioned this pull request Jul 7, 2022

Roll Flutter Engine from b974511b0c12 to b5fdb54943c0 (4 revisions) flutter/flutter#107261

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 7, 2022

1276049 Consolidate method signature for PlatformDispatcher.onError (…

4c30629

…flutter/engine#34428)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Consolidate method signature for `PlatformDispatcher.onError` #34428

Consolidate method signature for `PlatformDispatcher.onError` #34428

Uh oh!

ueman commented Jul 1, 2022 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Jul 1, 2022

Uh oh!

dnfield commented Jul 1, 2022

Uh oh!

jonahwilliams commented Jul 1, 2022

Uh oh!

ueman Jul 6, 2022

Uh oh!

ueman commented Jul 6, 2022

Uh oh!

ueman Jul 6, 2022

Uh oh!

auto-submit bot commented Jul 6, 2022

Uh oh!

auto-submit bot commented Jul 6, 2022

Uh oh!

jonahwilliams left a comment

Uh oh!

auto-submit bot commented Jul 6, 2022

Uh oh!

ueman commented Jul 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Consolidate method signature for PlatformDispatcher.onError #34428

Consolidate method signature for PlatformDispatcher.onError #34428

Uh oh!

Conversation

ueman commented Jul 1, 2022 • 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 Jul 1, 2022

Uh oh!

dnfield commented Jul 1, 2022

Uh oh!

jonahwilliams commented Jul 1, 2022

Uh oh!

ueman Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

ueman commented Jul 6, 2022

Uh oh!

ueman Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Jul 6, 2022

Uh oh!

auto-submit bot commented Jul 6, 2022

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Jul 6, 2022

Uh oh!

ueman commented Jul 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Consolidate method signature for `PlatformDispatcher.onError` #34428

Consolidate method signature for `PlatformDispatcher.onError` #34428

ueman commented Jul 1, 2022 •

edited

Loading