Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Jul 5, 2023

Description

This adds checks for the app delegate to make sure that it supports the Flutter-specific selectors before calling them, so that a non FlutterAppDelegate can be used for the NSApplicationDelegate on NSApp.

Related Issues

Tests

  • Added a test to make sure things don't crash if the app delegate isn't a FlutterAppDelegate.

@gspencergoog gspencergoog force-pushed the check_delegate_selectors branch from 5c44501 to af32633 Compare July 6, 2023 19:33
@gspencergoog gspencergoog requested a review from cbracken July 6, 2023 20:45
@gspencergoog
Copy link
Contributor Author

@cbracken How would I set the delegate to a non-FlutterAppDelegate in a test?

@cbracken
Copy link
Member

You should be able to just create a class that extends NSObject <NSApplicationDelegate> and set the delegate property of the current application at runtime. IIRC you'll want to hold on to the previous one then set it back at the end of the test to avoid test ordering bugs.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

Overall lgtm - just the test to do.

@gspencergoog gspencergoog force-pushed the check_delegate_selectors branch from 85a5c53 to 5fba395 Compare July 14, 2023 19:55
@gspencergoog gspencergoog force-pushed the check_delegate_selectors branch from 5fba395 to 25609e0 Compare July 14, 2023 21:00
@gspencergoog gspencergoog marked this pull request as ready for review July 14, 2023 21:01
Copy link
Contributor Author

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

Okay, this is ready for a "real" review. I added a test.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

Looks great! lgtm.

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 18, 2023
@auto-submit auto-submit bot merged commit d1712b2 into flutter:main Jul 18, 2023
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Jul 18, 2023
…130820)

flutter/engine@45851af...71bbece

2023-07-18 [email protected] [web] sync => isSync , scuba => golden (flutter/engine#43699)
2023-07-18 [email protected] Roll Skia from 219ca2581ab2 to 4e518e65fea0 (2 revisions) (flutter/engine#43777)
2023-07-18 [email protected] Roll ANGLE from 113f847be69f to 52fe3116ead9 (146 revisions) (flutter/engine#43776)
2023-07-18 [email protected] Roll Skia from b7103fe086c1 to 219ca2581ab2 (1 revision) (flutter/engine#43774)
2023-07-18 [email protected] [ci.yaml] Skip windows arm on releases (flutter/engine#43771)
2023-07-18 [email protected] Check FlutterAppDelegate selector support before calling (flutter/engine#43425)
2023-07-18 [email protected] Roll Fuchsia Mac SDK from _CIP-1iUTmGCbCDZ5... to SCshjyIlymHWD9W4D... (flutter/engine#43773)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from _CIP-1iUTmGC to SCshjyIlymHW

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 [email protected],[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
harryterkelsen pushed a commit to harryterkelsen/engine that referenced this pull request Jul 20, 2023
## Description

This adds checks for the app delegate to make sure that it supports the Flutter-specific selectors before calling them, so that a non `FlutterAppDelegate` can be used for the `NSApplicationDelegate` on `NSApp`.

## Related Issues
 - flutter/flutter#124829
 - flutter/flutter#127476

## Tests
 - Added a test to make sure things don't crash if the app delegate isn't a `FlutterAppDelegate`.
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
…lutter#130820)

flutter/engine@45851af...71bbece

2023-07-18 [email protected] [web] sync => isSync , scuba => golden (flutter/engine#43699)
2023-07-18 [email protected] Roll Skia from 219ca2581ab2 to 4e518e65fea0 (2 revisions) (flutter/engine#43777)
2023-07-18 [email protected] Roll ANGLE from 113f847be69f to 52fe3116ead9 (146 revisions) (flutter/engine#43776)
2023-07-18 [email protected] Roll Skia from b7103fe086c1 to 219ca2581ab2 (1 revision) (flutter/engine#43774)
2023-07-18 [email protected] [ci.yaml] Skip windows arm on releases (flutter/engine#43771)
2023-07-18 [email protected] Check FlutterAppDelegate selector support before calling (flutter/engine#43425)
2023-07-18 [email protected] Roll Fuchsia Mac SDK from _CIP-1iUTmGCbCDZ5... to SCshjyIlymHWD9W4D... (flutter/engine#43773)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from _CIP-1iUTmGC to SCshjyIlymHW

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 [email protected],[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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-macos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants