ios: remove shared_application and support app extension build #44732

cyanglaz · 2023-08-15T18:49:28Z

This PR turns on the flag to support engine to build app extension.
When building for app extension, the usage of sharedApplication API is removed.

fixes flutter/flutter#124286 and flutter/flutter#124289

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.

draft format draf draft format draft fix bundle format fix test

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.h

stuartmorgan-g · 2023-08-22T15:07:20Z

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.h


+NSBundle* FLTGetApplicationBundle();
+
+NSURL* FLTAssetsFromBundle(NSBundle* bundle);


Can this be given a more precise name? I'm assuming this is an assets directory, rather than actual assets?

stuartmorgan-g · 2023-08-22T15:08:28Z

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.mm

-    return bundle;
+NSBundle* FLTGetApplicationBundle() {
+  NSBundle* mainBundle = [NSBundle mainBundle];
+  // App extension bundle is in Runner.app/PlugIns/Extension.appex.


Nit: It's not necessarily "Runner.app", right? Maybe <AppName>.app/PlugiIns/Extension.appex?

stuartmorgan-g · 2023-08-22T15:10:11Z

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.mm

+
+NSBundle* FLTFrameworkBundleWithIdentifier(NSString* flutterBundleID) {
+  NSBundle* appBundle = FLTGetApplicationBundle();
+  NSBundle* bundle = FLTFrameworkBundleInternal(flutterBundleID, appBundle.privateFrameworksURL);


frameworkBundle (since it's confusing to have something just called bundle when there are two different bundles being tracked).

stuartmorgan-g · 2023-08-22T15:11:54Z

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.mm

+NSURL* FLTAssetsFromBundle(NSBundle* bundle) {
+  NSString* assetsPathFromInfoPlist = [bundle objectForInfoDictionaryKey:@"FLTAssetsPath"];
+  NSString* flutterAssetsName =
+      assetsPathFromInfoPlist != nil ? assetsPathFromInfoPlist : @"flutter_assets";


Optional: You can use the Elvis operator extensions here (it's a style-guide-approved language extension): assetsPathFromInfoPlist ?: @"flutter_assets";

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.mm

stuartmorgan-g · 2023-08-22T15:16:19Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.mm

 constexpr char kTextPlainFormat[] = "text/plain";
 const UInt32 kKeyPressClickSoundId = 1306;
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-variable"


Can we ifdef these pragmas to make it clearer why it's needed?

stuartmorgan-g · 2023-08-22T15:18:01Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

        currentInterfaceOrientation = 1 << windowScene.interfaceOrientation;
      } else {
+#if APPLICATION_EXTENSION_API_ONLY
+        FML_LOG(ERROR) << "Applicatioin based status bar orentiation update is not supported in "


Typo: Application

stuartmorgan-g

LGTM with comment nits.

stuartmorgan-g · 2023-08-25T19:34:32Z

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.h

-NSBundle* FLTFrameworkBundleWithIdentifier(NSString* bundleID);
+NSBundle* FLTFrameworkBundleWithIdentifier(NSString* flutterFrameworkBundleID);
+
+// Find the bundle of the application.


stuartmorgan-g · 2023-08-25T19:34:40Z

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.h

+// This returns [NSBundle mainBundle] if the current running process is the application.
+NSBundle* FLTGetApplicationBundle();
+
+// Find the Flutter asset directory from `bundle`.


stuartmorgan-g · 2023-08-25T19:34:56Z

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.h

+
+// Find the bundle of the application.
+//
+// This returns [NSBundle mainBundle] if the current running process is the application.


Nit: "This returns" -> "Returns"

stuartmorgan-g · 2023-08-25T19:35:10Z

shell/platform/darwin/common/framework/Source/FlutterNSBundleUtils.h

+// The raw path can be set by the application via info.plist's `FLTAssetsPath` key.
+// If the key is not set, `flutter_assets` is used as the raw path value.
+//
+// If no valid asset is found under the raw path, return nil.


nit: returns

…ild (flutter/engine#44732)

…133362) flutter/engine@1ec7b89...53595c9 2023-08-25 [email protected] [Impeller] Fix mask blurs and the Gaussian blur coverage hint. (flutter/engine#45079) 2023-08-25 [email protected] ios: remove shared_application and support app extension build (flutter/engine#44732) 2023-08-25 [email protected] Roll Skia from 76672468e8d7 to 40f4e01fca40 (3 revisions) (flutter/engine#45126) 2023-08-25 [email protected] Roll Fuchsia Mac SDK from 4LHUJjNlDT21v_pdT... to Qj4BGsKtF7EJssKIK... (flutter/engine#45127) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from 4LHUJjNlDT21 to Qj4BGsKtF7EJ 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

#44732)" This reverts commit 6ef35bb.

… extension build" (#45250) Reverts #44732 context: b/297654739 Synced with Chris and the error message is related to the PR

…er#44732) This PR turns on the flag to support engine to build app extension. When building for app extension, the usage of sharedApplication API is removed. fixes flutter/flutter#124286 and flutter/flutter#124289 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

… extension build" (flutter#45250) Reverts flutter#44732 context: b/297654739 Synced with Chris and the error message is related to the PR

…#44732" (#45351) Relands #44732 with fix. The original PR returns nil when the assets is not reachable, in some cases, the assets are not loaded yet but will be loaded later, so we should return the asset URL regardless. Also added a fallback to main bundle to match the previous implementation. The original PR was failed in internal tests in b/297654739 Now with the fix, all tests passed: cl/561449914 fixes flutter/flutter#124289 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added platform-ios affects: desktop platform-macos labels Aug 15, 2023

cyanglaz force-pushed the remove_shared_application branch 2 times, most recently from fddbd73 to f168f89 Compare August 16, 2023 18:28

draft

c17f83f

draft format draf draft format draft fix bundle format fix test

cyanglaz marked this pull request as ready for review August 17, 2023 19:44

cyanglaz requested a review from stuartmorgan-g August 17, 2023 19:44

cyanglaz changed the title ~~[Draft] ios: remove shared_application and support app extension build~~ ios: remove shared_application and support app extension build Aug 17, 2023

stuartmorgan-g suggested changes Aug 22, 2023

View reviewed changes

format

4b5c020

cyanglaz force-pushed the remove_shared_application branch from f168f89 to 4b5c020 Compare August 22, 2023 19:13

Chris Yang added 3 commits August 23, 2023 10:06

review and test fixes

b61dba6

format

8492964

fix

f0e8999

cyanglaz requested a review from stuartmorgan-g August 23, 2023 20:25

stuartmorgan-g approved these changes Aug 25, 2023

View reviewed changes

review

11bff56

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 25, 2023

auto-submit bot merged commit 6ef35bb into flutter:main Aug 25, 2023

engine-flutter-autoroll mentioned this pull request Aug 25, 2023

Roll Flutter Engine from 1ec7b89f3a6b to 53595c937df1 (4 revisions) flutter/flutter#133362

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2023

6ef35bb05 ios: remove shared_application and support app extension bu…

4f3f219

…ild (flutter/engine#44732)

cyanglaz deleted the remove_shared_application branch August 25, 2023 22:47

cyanglaz mentioned this pull request Aug 29, 2023

[app_extension_safe] Make iOS embedder extension safe when extension_safe build flag is true flutter/flutter#124289

Closed

XilaiZhang added a commit that referenced this pull request Aug 29, 2023

Revert "ios: remove shared_application and support app extension build (

d303314

#44732)" This reverts commit 6ef35bb.

XilaiZhang mentioned this pull request Aug 29, 2023

[flutter roll] Revert "ios: remove shared_application and support app extension build" #45250

Merged

auto-submit bot pushed a commit that referenced this pull request Aug 29, 2023

[flutter roll] Revert "ios: remove shared_application and support app…

367c709

… extension build" (#45250) Reverts #44732 context: b/297654739 Synced with Chris and the error message is related to the PR

cyanglaz mentioned this pull request Aug 31, 2023

Reland "ios: remove shared_application and support app extension build #44732" #45351

Merged

8 tasks


		NSBundle* FLTGetApplicationBundle();

		NSURL* FLTAssetsFromBundle(NSBundle* bundle);

ios: remove shared_application and support app extension build #44732

ios: remove shared_application and support app extension build #44732

Uh oh!

Conversation

cyanglaz commented Aug 15, 2023

Pre-launch Checklist

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants