Fixes https://github.com/flutter/flutter/issues/151274
This is only an issue on iOS (so far) because of the platform view rendering strategy that involves submitting multiple impeller frames per compositor frame.
FYI @bdero
When `FlutterUndoManagerPlugin` deallocated, it is supposed to deregister itself from the `NSUndoManager` stack.
However, during dealloc `_undoManagerDelegate.undoManager` is nil (`_undoManagerDelegate` = the engine, and `undoManager` was its view controller's `undoManager`, which was already gone).
Since `_undoManagerDelegate.undoManager` is nil, it doesn't actually call `-[NSUndoManager removeAllActionsWithTarget]`. In the add-to-app scenario, after the view controller pops back to the native view, and "undo" is invoked, the undo action for the `FlutterUndoManagerPlugin` is handled, but it already dealloced so crash.
Fixes https://github.com/flutter/flutter/issues/150408
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
fix cursor hit testing in platform view in #52159
We found that the mechanism of `[NSView hitTest: view] `will handle the
flip coordinate automatically. In the previous PR, because the superview
of FlutterView was not flipped, we should not convert the point using
`[self convertPoint:event.locationInWindow fromView:nil]` .The
FlutterView hit test should run under the window coordinate system;
otherwise, FlutterMutatorView will hit a wrong point.
Before the fix:
Actual mouse move: `event.locationInWindow (x = 47.45703125, y =
416.86328125)`
FlutterView HitTest: `(NSPoint) point = (x = 47.45703125, y =
203.13671875)`
FlutterMutatorView HitTest: `(NSPoint) point = (x = 47.45703125, y =
416.86328125)`
After the fix:
Actual mouse move: `event.locationInWindow (x = 47.45703125, y =
416.86328125)`
FlutterView HitTest: `(NSPoint) point = (x = 47.45703125, y =
416.86328125)`
FlutterMutatorView HitTest: `(NSPoint) point = (x = 47.45703125, y =
203.13671875)`
## Pre-launch Checklist
- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel
on [Discord].
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
In https://github.com/flutter/buildroot/pull/860, disabling `NSAssert` in release build was added, and https://github.com/flutter/engine/pull/53005 tries to roll the change into the engine. However, the change is blocked due to several compilation errors in the engine.
This is due to the fact that Google's `DCHECK`(in flutter engine, `FML_DCHECK`) does not make unused variable warning even in release build, while traditional assertion macro like `assert` or `NSAssert` does. For example, the following code will be expanded like this:
code:
```c++
FML_DCHECK(status == kCVReturnSuccess && pxbuffer != nullptr) <<
"Failed to create pixel buffer";
```
expanded code:
```c++
true || (status == kCVReturnSuccess && pxbuffer != nullptr)
? (void)0 ::
: fml::LogMessageVoidify() &
::fml::LogMessage(::fml::kLogFatal, 0, 0, nullptr).stream()
```
However, equivalent code with `NSAssert` will make unused variable warning, since the expanded code doesn't have any reference to the variable:
code:
```c++
NSAssert(status == kCVReturnSuccess && pxbuffer != NULL, @"Failed to create pixel buffer.");
```
expanded code:
```c++
do {
} while (0)
```
To unblock the build, this CL replaces several `NSAssert` with `FML_DCHECK` in the engine codebase. This CL isn't aimed to replace all `NSAssert` with `FML_DCHECK` since discussions in https://github.com/flutter/buildroot/pull/860 are not finalized whether to replace all `NSAssert` with `FML_DCHECK` or not.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
1. Migrate TextInputSemanticsObject to ARC
2. Rename the Objective-C-only `accessibility_text_entry.*` files to `TextInputSemanticsObject.*`
3. Move `FlutterInactiveTextInput` interface from the header file into the implementation, the only place it's used.
Blocked on `SemanticsObject` ARC migration https://github.com/flutter/engine/pull/52824
Part of https://github.com/flutter/flutter/issues/137801.
Split the too-large `Semantics` [MRC to ARC migration](https://github.com/flutter/flutter/issues/137801) into two PRs: this one, which refactors, and the next which will actually do the migration.
1. Use properties instead of their backing ivars (except in the usual init, dealloc, getters/setters)
2. For Foundation collections prefer `copy` over `strong`.
3. Dot notation for properties
4. Change `privateSetParent:` to instead use a `readwrite` property in `SemanticsObject ()`.
5. Switch the `semanticsObject` property from `weak` to `retain` to get the synthesized property (keeping it as `weak` is a compilation error in MRC) but I'll swap it back to a `weak` in the ARC migration PR coming next.
6. `SemanticsObjectTest` fails on my machine and passes on CI. Switched the cleaner `CGRectEqualToRect` (and related) checks to instead assert x, y, width, height so we can see the value when it fails:
```
((CGSizeEqualToSize( scrollView.contentSize, CGSizeMake((w + scrollExtentMax) * effectivelyScale, h * effectivelyScale))) is true) failed
```
becomes:
```
((scrollView.contentSize.height) equal to (h * effectivelyScale)) failed: ("33.3333333333") is not equal to ("33.333336")
```
Use `XCTAssertEqualWithAccuracy` now that I can see it's a floating point precision issue.
The size of the LTO build of the engine with the dylib compressed is as follows:
```sh
$ du -k libFlutter*
5236 libFlutter.dylib.tar.gz
4324 libFlutterSlimpeller.dylib.tar.gz
```
Sizes are in KiB. This represents a binary size reduction of 17.41% of the compressed artifacts. The compression ratios will likely differ based on the compression scheme.
Uncompressed, the sizes are:
```sh
$ du -k libFlutter*
16920 libFlutter.dylib
14044 libFlutterSlimpeller.dylib
```
This represents a binary size reduction of 16.99% which is in the same ballpark.
The really mucky bit was backing out the raster cache and persistent cache. I want to clean that up in a later patch so that those TUs are part of a separate submodule.
Opting out of Impeller will lead to a fatal log at startup saying the opt-out is disallowed.
Fixes https://github.com/flutter/flutter/issues/126606
- Migrate `FlutterDartProject` and `FlutterPluginAppLifeCycleDelegate` from MRC to ARC.
- Swap a few dictionary `objectForKey:` to subscripting.
- Header cleanup.
- Cleanup build DEPS.
Part of https://github.com/flutter/flutter/issues/137801.
I found this while migrating `FlutterPlatformViews_Internal.mm` to ARC https://github.com/flutter/engine/pull/52535. I'll land this first.
```objc
if (_backdropFilterView != visualEffectView) {
_backdropFilterView = [visualEffectView retain];
}
```
should instead be something like:
```objc
if (_backdropFilterView != visualEffectView) {
id oldBackdropFilterView = _backdropFilterView;
_backdropFilterView = [visualEffectView retain];
[oldBackdropFilterView release];
}
```
But that's already what the built-in MRC `nonatomic, retain` property setter does, so use that instead.
Added a test that passes on this PR and fails on main.
The observed behavior is that if a keypress both reach framework and flutter textinputplugin selector and the keypress handler in framework causes the the textinputplugin to resign first responder, the selector will still go through even if the textinputplugin has already resigned.
The pr makes it so textinputplugin will ignore these selector call
fixes https://github.com/flutter/flutter/issues/143270
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This removes goma most places except for the arguments to the `gn`
script, which are referenced by recipes. This does not remove goma
capabilities from the GN build entirely. That requires changes in the
buildroot which have to be staged after this change.
The latest version of Clang is reporting warnings from the -Wcast-function-type-mismatch check when a function taking a __strong pointer parameter is converted to a void* parameter.
This stub PR removes and extra `setViewController`, which is unnecessary because the view controller's initializer already attaches itself to the engine.
This PR also removes an extra import since `FlutterCompositor` is not used in the engine's definition. As a result, `FlutterEngineTest.mm` needs to import it explicitly.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
CVPixelBuffers from the application can have arbitrary pixel formats. However, current implementation doesn't support all pixel formats. To address this, add a comment to the FlutterTexture class to clarify which pixel formats are supported. Also, reject unsupported pixel formats so that the application can handle them properly.
Fixes [#147242](https://github.com/flutter/flutter/issues/147242).
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Trying to migrate the iOS embedder MRC to ARC, I'm attempting to break some dependency cycles so it's easier to migrate the "leaf" dependencies to ARC, working my way up.
The cycle is:
**`FlutterViewController`** -> `FlutterView` -> `FlutterPlatformView` -> (Before this PR)**`FlutterViewController`**
`FlutterViewController` depends on many other large MRC classes, like FlutterEngine., so I'd like to pull that one out so `FlutterView`, `FlutterPlatformView`, and `FlutterOverlayView` can be migrated to ARC in a smaller PR.
`FlutterPlatformView` only depends on `FlutterViewController` in this one place, casting a `UIViewController` and calling `-forceTouchesCancelled:`.
0e6143504c/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm (L1201)
Instead, move `-forceTouchesCancelled:` to the existing `FlutterViewResponder` protocol with the other touch events, which `FlutterViewController` already implements. The cast can then be removed, breaking the cycle.
Clean up all the imports.
Part of https://github.com/flutter/flutter/issues/137801, though this doesn't actually migrate anything to ARC.
Smart pointers support ARC as of https://github.com/flutter/engine/pull/47612, and the unit tests were migrated in https://github.com/flutter/engine/pull/48162.
Migrate `FlutterUndoManagerPlugin` from MRC to ARC.
1. Refactor so the plugin and its tests don't need to understand the details of `FlutterViewController` or `FlutterEngine` (its delegate). This decouples the plugin, and means it doesn't depend on any MRC classes.
2. Change the delegate so conforming only requires the objects the undo plugin actually needs.
Part of https://github.com/flutter/flutter/issues/137801.
This PR adds `FlutterViewController` with new API `viewIdentifier` and
renames all occasions that mention "viewId" to "viewIdentifier", to
align with the requirement for the iOS shell.
A new typedef `FlutterViewIdentifier` is also added. ~~The problem is,
we don't have a file to contain this typedef. Currently I put them in a
new file called `common.h`. I'm open to other suggestions.~~ It has been
moved to `FlutterViewController.h`.
* Another alternative is to not use the type def, but use `int64_t`
directly. I'm ok with this choice too, since honestly it's just a 64
int.
Also, macOS's definition for `kFlutterImplicitViewId` has been removed
in favor of the one in `common/constants.h`.
## 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 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].
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
Fixes https://github.com/flutter/flutter/issues/138936
Currently the engine hold a single backbuffer per flutter view layer,
which gets swapped with front buffer. This however is too optimistic, as
the front buffer in some cases might not get immediately picked up by
the compositor. When that happens, during next frame the cache may
return a backbuffer that is in fact still being used by the compositor.
Rendering to such surface will result in artifacts seen in the video.
This seems more likely to happen with busy platform thread. It is also
more likely to happen with the presence of "heavy" platform views such
as `WKWebView`.
IOSurface is able to report when it is being used by the compositor
(`IOSurfaceIsInUse`). This PR ensures that the backing store cache never
returns surface that is being used by compositor. This may result in
transiently keeping more surfaces than before, as the compositor
sometimes seems to holds on to the surface longer than it is displayed,
but that's preferable outcome to visual glitches.
This PR adds `age` field to `FlutterSurface`. When returning buffers to
the surface cache (during compositor commit), the age of each surface
currently present in cache increases by one, while the newly returned
surfaces have their age set to 0.
When returning surfaces from cache, a surface with lowest age that is
not in use by compositor is returned.
Surfaces with age that reaches 30 are evicted from the pool. Reaching
this age means one of two things:
- When surface is still in use at age 30 it means the compositor is
holding on to it much longer than we expect. In this case just forget
about the surface, we can't really do anything about it.
- When surface is not in use at age 30, it means it hasn't been removed
from cache for 29 subsequent frames. That means the cache is holding more
surfaces than needed.
Removing all surfaces from cache after idle period remains unchanged.
Before:
https://github.com/flutter/engine/assets/96958/ba2bfc43-525e-4a88-b37c-61842994d3bc
After:
https://github.com/flutter/engine/assets/96958/8b5d393e-3031-46b1-b3f0-cb7f63f8d960
*If you had to change anything in the [flutter/tests] repo, include a
link to the migration guide as per the [breaking change policy].*
## Pre-launch Checklist
- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel
on [Discord].
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
Part of https://github.com/flutter/flutter/issues/145729 .
I don't believe (or wasn't able to find ) if there is a way to specify dart-vm flags from an iOS application. Once its installed, the tool provided process args won't work. By allowing this to be specified in the Plist, developers can ensure the value persists for subsequent launches.
Smart pointers support ARC as of https://github.com/flutter/engine/pull/47612, and the unit tests were migrated in https://github.com/flutter/engine/pull/48162.
Migrate`FlutterCallbackCache` and `FlutterKeyboardManager` from MRC to ARC. These files do not themselves import any MRC files, making them leaf nodes in the dependency graph of MRC files.
Doing a few at a time to make the dependency graph manageable, and to easily revert if this causes retain cycles or other memory management issues.
Part of https://github.com/flutter/flutter/issues/137801.
