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

iOS: Eliminate fml::scoped_nsobject pointer use #56295

Merged
merged 3 commits into from
Nov 4, 2024
Merged

iOS: Eliminate fml::scoped_nsobject pointer use #56295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1667da1
iOS: Eliminate fml::scoped_nsobject pointer use
cbracken Oct 31, 2024
ba95517
review feedback
cbracken Nov 1, 2024
d1cacfd
Revert shell_test_platform_view_metal
cbracken Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion shell/gpu/gpu_surface_metal_impeller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

#include "flutter/flow/surface.h"
#include "flutter/fml/macros.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"
#include "flutter/impeller/display_list/aiks_context.h"
#include "flutter/impeller/renderer/backend/metal/context_mtl.h"
#include "flutter/shell/gpu/gpu_surface_metal_delegate.h"
Expand Down
1 change: 0 additions & 1 deletion shell/gpu/gpu_surface_metal_skia.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#include "flutter/common/graphics/persistent_cache.h"
#include "flutter/fml/make_copyable.h"
#include "flutter/fml/platform/darwin/cf_utils.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"
#include "flutter/fml/trace_event.h"
#include "flutter/shell/gpu/gpu_surface_metal_delegate.h"
#include "third_party/skia/include/core/SkCanvas.h"
Expand Down
1 change: 0 additions & 1 deletion shell/platform/darwin/common/buffer_conversions.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#import "flutter/shell/platform/darwin/common/buffer_conversions.h"

#include "flutter/fml/macros.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"

static_assert(__has_feature(objc_arc), "ARC must be enabled.");

Expand Down
1 change: 0 additions & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterDartVMServicePublisher.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ - (instancetype)initWithEnableVMServicePublication:(BOOL)enableVMServicePublicat

#include "flutter/fml/logging.h"
#include "flutter/fml/message_loop.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"
#include "flutter/runtime/dart_service_isolate.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"

Expand Down
1 change: 0 additions & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

#include "flutter/flow/surface.h"
#include "flutter/fml/memory/weak_ptr.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"
#include "flutter/fml/trace_event.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterChannels.h"
#import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterPlugin.h"
Expand Down
16 changes: 8 additions & 8 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,7 @@ @implementation ForwardingGestureRecognizer {
// This gesture recognizer retains the `FlutterViewController` until the
// end of a gesture sequence, that is all the touches in touchesBegan are concluded
// with |touchesCancelled| or |touchesEnded|.
fml::scoped_nsobject<UIViewController<FlutterViewResponder>> _flutterViewController;
UIViewController<FlutterViewResponder>* _flutterViewController;
}

- (instancetype)initWithTarget:(id)target
Expand All @@ -741,26 +741,26 @@ - (void)touchesBegan:(NSSet*)touches withEvent:(UIEvent*)event {
// At the start of each gesture sequence, we reset the `_flutterViewController`,
// so that all the touch events in the same sequence are forwarded to the same
// `_flutterViewController`.
_flutterViewController.reset(_platformViewsController->GetFlutterViewController());
_flutterViewController = _platformViewsController->GetFlutterViewController();
}
[_flutterViewController.get() touchesBegan:touches withEvent:event];
[_flutterViewController touchesBegan:touches withEvent:event];
_currentTouchPointersCount += touches.count;
}

- (void)touchesMoved:(NSSet*)touches withEvent:(UIEvent*)event {
[_flutterViewController.get() touchesMoved:touches withEvent:event];
[_flutterViewController touchesMoved:touches withEvent:event];
}

- (void)touchesEnded:(NSSet*)touches withEvent:(UIEvent*)event {
[_flutterViewController.get() touchesEnded:touches withEvent:event];
[_flutterViewController touchesEnded:touches withEvent:event];
_currentTouchPointersCount -= touches.count;
// Touches in one touch sequence are sent to the touchesEnded method separately if different
// fingers stop touching the screen at different time. So one touchesEnded method triggering does
// not necessarially mean the touch sequence has ended. We Only set the state to
// UIGestureRecognizerStateFailed when all the touches in the current touch sequence is ended.
if (_currentTouchPointersCount == 0) {
self.state = UIGestureRecognizerStateFailed;
_flutterViewController.reset(nil);
_flutterViewController = nil;
[self forceResetStateIfNeeded];
}
}
Expand All @@ -771,11 +771,11 @@ - (void)touchesCancelled:(NSSet*)touches withEvent:(UIEvent*)event {
// Flutter needs all the cancelled touches to be "cancelled" change types in order to correctly
// handle gesture sequence.
// We always override the change type to "cancelled".
[_flutterViewController.get() forceTouchesCancelled:touches];
[_flutterViewController forceTouchesCancelled:touches];
_currentTouchPointersCount -= touches.count;
if (_currentTouchPointersCount == 0) {
self.state = UIGestureRecognizerStateFailed;
_flutterViewController.reset(nil);
_flutterViewController = nil;
[self forceResetStateIfNeeded];
}
}
Expand Down
7 changes: 2 additions & 5 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

#include "flutter/fml/macros.h"
#include "flutter/fml/memory/weak_ptr.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"
#include "flutter/lib/ui/semantics/custom_accessibility_action.h"
#include "flutter/lib/ui/semantics/semantics_node.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterChannels.h"
Expand Down Expand Up @@ -98,10 +97,8 @@ class AccessibilityBridge final : public AccessibilityBridgeIos {
// (i.e. the status bar or keyboard)
int32_t last_focused_semantics_object_id_;

// TODO(cbracken): https://github.com/flutter/flutter/issues/137801
// Eliminate use of fml::scoped_* wrappers here.
fml::scoped_nsobject<NSMutableDictionary<NSNumber*, SemanticsObject*>> objects_;
fml::scoped_nsprotocol<FlutterBasicMessageChannel*> accessibility_channel_;
NSMutableDictionary<NSNumber*, SemanticsObject*>* objects_;
FlutterBasicMessageChannel* accessibility_channel_;
Comment on lines +100 to +101
Copy link
Member

Choose a reason for hiding this comment

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

I guess even if they are fully Objective-C isa objects we don't use Objective-C naming (objects and accessibilityChannel)?

Copy link
Member Author

@cbracken cbracken Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In C++ contexts (C++ class fields) the style guide says we have to use C++ naming, which really hits the code with an ugly stick :(

Ref: https://google.github.io/styleguide/objcguide.html#style-matches-the-language

Copy link
Contributor

Choose a reason for hiding this comment

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

If the naming of ivars and variables depended on the thing being pointed to rather than the context, things would be much, much uglier.

I guarantee nobody would be happier if individual lines of Obj-C++ code had mixes of different variable naming styles depending on the underlying types of the things the variables are referring to.

Copy link
Member Author

@cbracken cbracken Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, but it's still unfortunate. I'd much prefer we reduced the number of C++ classes with Obj-C++ implementations and used straight Obj-C[++] classes where possible. I realise that's not always possible -- e.g. cases where embedders have some translation unit containing subclasses of a common C++ superclass etc.

int32_t previous_route_id_ = 0;
std::unordered_map<int32_t, flutter::CustomAccessibilityAction> actions_;
std::vector<int32_t> previous_routes_;
Expand Down
31 changes: 15 additions & 16 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,17 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,
ios_delegate_(ios_delegate ? std::move(ios_delegate)
: std::make_unique<DefaultIosDelegate>()),
weak_factory_(this) {
accessibility_channel_.reset([[FlutterBasicMessageChannel alloc]
accessibility_channel_ = [[FlutterBasicMessageChannel alloc]
initWithName:@"flutter/accessibility"
binaryMessenger:platform_view->GetOwnerViewController().engine.binaryMessenger
codec:[FlutterStandardMessageCodec sharedInstance]]);
[accessibility_channel_.get() setMessageHandler:^(id message, FlutterReply reply) {
codec:[FlutterStandardMessageCodec sharedInstance]];
[accessibility_channel_ setMessageHandler:^(id message, FlutterReply reply) {
HandleEvent((NSDictionary*)message);
}];
}

AccessibilityBridge::~AccessibilityBridge() {
[accessibility_channel_.get() setMessageHandler:nil];
[accessibility_channel_ setMessageHandler:nil];
clearState();
view_controller_.viewIfLoaded.accessibilityElements = nil;
}
Expand All @@ -74,7 +74,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,

void AccessibilityBridge::AccessibilityObjectDidBecomeFocused(int32_t id) {
last_focused_semantics_object_id_ = id;
[accessibility_channel_.get() sendMessage:@{@"type" : @"didGainFocus", @"nodeId" : @(id)}];
[accessibility_channel_ sendMessage:@{@"type" : @"didGainFocus", @"nodeId" : @(id)}];
}

void AccessibilityBridge::AccessibilityObjectDidLoseFocus(int32_t id) {
Expand Down Expand Up @@ -152,7 +152,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,
}
}

SemanticsObject* root = objects_.get()[@(kRootNodeId)];
SemanticsObject* root = objects_[@(kRootNodeId)];

bool routeChanged = false;
SemanticsObject* lastAdded = nil;
Expand Down Expand Up @@ -196,13 +196,13 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,
view_controller_.viewIfLoaded.accessibilityElements = nil;
}

NSMutableArray<NSNumber*>* doomed_uids = [NSMutableArray arrayWithArray:[objects_ allKeys]];
NSMutableArray<NSNumber*>* doomed_uids = [NSMutableArray arrayWithArray:objects_.allKeys];
if (root) {
VisitObjectsRecursivelyAndRemove(root, doomed_uids);
}
[objects_ removeObjectsForKeys:doomed_uids];

for (SemanticsObject* object in [objects_ allValues]) {
for (SemanticsObject* object in objects_.allValues) {
[object accessibilityBridgeDidFinishUpdate];
}

Expand All @@ -217,8 +217,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,

if (layoutChanged) {
SemanticsObject* next = FindNextFocusableIfNecessary();
SemanticsObject* lastFocused =
[objects_.get() objectForKey:@(last_focused_semantics_object_id_)];
SemanticsObject* lastFocused = [objects_ objectForKey:@(last_focused_semantics_object_id_)];
// Only specify the focus item if the new focus is different, avoiding double focuses on the
// same item. See: https://github.com/flutter/flutter/issues/104176. If there is a route
// change, we always refocus.
Expand Down Expand Up @@ -290,10 +289,10 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode,

SemanticsObject* AccessibilityBridge::GetOrCreateObject(int32_t uid,
flutter::SemanticsNodeUpdates& updates) {
SemanticsObject* object = objects_.get()[@(uid)];
SemanticsObject* object = objects_[@(uid)];
if (!object) {
object = CreateObject(updates[uid], GetWeakPtr());
objects_.get()[@(uid)] = object;
objects_[@(uid)] = object;
} else {
// Existing node case
auto nodeEntry = updates.find(object.node.id);
Expand All @@ -309,7 +308,7 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode,
// SemanticsObject implementation. Instead, we replace it with a new
// instance.
SemanticsObject* newSemanticsObject = CreateObject(node, GetWeakPtr());
ReplaceSemanticsObject(object, newSemanticsObject, objects_.get());
ReplaceSemanticsObject(object, newSemanticsObject, objects_);
object = newSemanticsObject;
}
}
Expand All @@ -332,11 +331,11 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode,
}

// Tries to refocus the previous focused semantics object to avoid random jumps.
return FindFirstFocusable([objects_.get() objectForKey:@(last_focused_semantics_object_id_)]);
return FindFirstFocusable(objects_[@(last_focused_semantics_object_id_)]);
}

SemanticsObject* AccessibilityBridge::FindFirstFocusable(SemanticsObject* parent) {
SemanticsObject* currentObject = parent ?: objects_.get()[@(kRootNodeId)];
SemanticsObject* currentObject = parent ?: objects_[@(kRootNodeId)];
if (!currentObject) {
return nil;
}
Expand All @@ -360,7 +359,7 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode,
ios_delegate_->PostAccessibilityNotification(UIAccessibilityAnnouncementNotification, message);
}
if ([type isEqualToString:@"focus"]) {
SemanticsObject* node = objects_.get()[annotatedEvent[@"nodeId"]];
SemanticsObject* node = objects_[annotatedEvent[@"nodeId"]];
ios_delegate_->PostAccessibilityNotification(UIAccessibilityLayoutChangedNotification, node);
}
}
Expand Down
9 changes: 4 additions & 5 deletions shell/platform/darwin/ios/framework/Source/overlay_layer_pool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#import <UIKit/UIKit.h>

#include "flow/surface.h"
#include "fml/platform/darwin/scoped_nsobject.h"

#import "flutter/shell/platform/darwin/ios/ios_context.h"

Expand All @@ -21,15 +20,15 @@ class IOSSurface;

/// @brief State holder for a Flutter overlay layer.
struct OverlayLayer {
OverlayLayer(const fml::scoped_nsobject<UIView>& overlay_view,
const fml::scoped_nsobject<UIView>& overlay_view_wrapper,
OverlayLayer(UIView* overlay_view,
UIView* overlay_view_wrapper,
std::unique_ptr<IOSSurface> ios_surface,
std::unique_ptr<Surface> surface);

~OverlayLayer() = default;

fml::scoped_nsobject<UIView> overlay_view;
fml::scoped_nsobject<UIView> overlay_view_wrapper;
UIView* overlay_view;
UIView* overlay_view_wrapper;
std::unique_ptr<IOSSurface> ios_surface;
std::unique_ptr<Surface> surface;

Expand Down
34 changes: 16 additions & 18 deletions shell/platform/darwin/ios/framework/Source/overlay_layer_pool.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@

namespace flutter {

OverlayLayer::OverlayLayer(const fml::scoped_nsobject<UIView>& overlay_view,
const fml::scoped_nsobject<UIView>& overlay_view_wrapper,
OverlayLayer::OverlayLayer(UIView* overlay_view,
UIView* overlay_view_wrapper,
std::unique_ptr<IOSSurface> ios_surface,
std::unique_ptr<Surface> surface)
: overlay_view(overlay_view),
Expand All @@ -22,7 +22,6 @@
SkRect rect,
int64_t view_id,
int64_t overlay_id) {
UIView* overlay_view_wrapper = this->overlay_view_wrapper.get();
auto screenScale = [UIScreen mainScreen].scale;
// Set the size of the overlay view wrapper.
// This wrapper view masks the overlay view.
Expand All @@ -32,7 +31,6 @@
overlay_view_wrapper.accessibilityIdentifier =
[NSString stringWithFormat:@"platform_view[%lld].overlay[%lld]", view_id, overlay_id];

UIView* overlay_view = this->overlay_view.get();
// Set the size of the overlay view.
// This size is equal to the device screen size.
overlay_view.frame = [flutter_view convertRect:flutter_view.bounds toView:overlay_view_wrapper];
Expand All @@ -59,32 +57,32 @@
MTLPixelFormat pixel_format) {
FML_DCHECK([[NSThread currentThread] isMainThread]);
std::shared_ptr<OverlayLayer> layer;
fml::scoped_nsobject<UIView> overlay_view;
fml::scoped_nsobject<UIView> overlay_view_wrapper;
UIView* overlay_view;
UIView* overlay_view_wrapper;

bool impeller_enabled = !!ios_context->GetImpellerContext();
if (!gr_context && !impeller_enabled) {
overlay_view.reset([[FlutterOverlayView alloc] init]);
overlay_view_wrapper.reset([[FlutterOverlayView alloc] init]);
overlay_view = [[FlutterOverlayView alloc] init];
overlay_view_wrapper = [[FlutterOverlayView alloc] init];

auto ca_layer = fml::scoped_nsobject<CALayer>{[overlay_view.get() layer]};
CALayer* ca_layer = overlay_view.layer;
std::unique_ptr<IOSSurface> ios_surface = IOSSurface::Create(ios_context, ca_layer);
std::unique_ptr<Surface> surface = ios_surface->CreateGPUSurface();

layer = std::make_shared<OverlayLayer>(std::move(overlay_view), std::move(overlay_view_wrapper),
layer = std::make_shared<OverlayLayer>(overlay_view, overlay_view_wrapper,
std::move(ios_surface), std::move(surface));
} else {
CGFloat screenScale = [UIScreen mainScreen].scale;
overlay_view.reset([[FlutterOverlayView alloc] initWithContentsScale:screenScale
pixelFormat:pixel_format]);
overlay_view_wrapper.reset([[FlutterOverlayView alloc] initWithContentsScale:screenScale
pixelFormat:pixel_format]);
overlay_view = [[FlutterOverlayView alloc] initWithContentsScale:screenScale
pixelFormat:pixel_format];
overlay_view_wrapper = [[FlutterOverlayView alloc] initWithContentsScale:screenScale
pixelFormat:pixel_format];

auto ca_layer = fml::scoped_nsobject<CALayer>{[overlay_view.get() layer]};
CALayer* ca_layer = overlay_view.layer;
std::unique_ptr<IOSSurface> ios_surface = IOSSurface::Create(ios_context, ca_layer);
std::unique_ptr<Surface> surface = ios_surface->CreateGPUSurface(gr_context);

layer = std::make_shared<OverlayLayer>(std::move(overlay_view), std::move(overlay_view_wrapper),
layer = std::make_shared<OverlayLayer>(overlay_view, overlay_view_wrapper,
std::move(ios_surface), std::move(surface));
layer->gr_context = gr_context;
}
Expand All @@ -102,8 +100,8 @@
// | | wrapper | | == mask => | overlay_view |
// | +--------------+ | +--------------+
// +------------------------+
layer->overlay_view_wrapper.get().clipsToBounds = YES;
[layer->overlay_view_wrapper.get() addSubview:layer->overlay_view];
layer->overlay_view_wrapper.clipsToBounds = YES;
[layer->overlay_view_wrapper addSubview:layer->overlay_view];

layers_.push_back(layer);
}
Expand Down
17 changes: 7 additions & 10 deletions shell/platform/darwin/ios/framework/Source/platform_views_controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

#include "flutter/flow/surface.h"
#include "flutter/fml/memory/weak_ptr.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"
#include "flutter/fml/task_runner.h"
#include "flutter/fml/trace_event.h"
#include "impeller/base/thread_safety.h"
Expand Down Expand Up @@ -242,12 +241,10 @@ class PlatformViewsController {
// The Slices are deleted by the PlatformViewsController.reset().
std::unordered_map<int64_t, std::unique_ptr<EmbedderViewSlice>> slices_;

fml::scoped_nsobject<FlutterMethodChannel> channel_;
fml::scoped_nsobject<UIView> flutter_view_;
fml::scoped_nsobject<UIViewController<FlutterViewResponder>> flutter_view_controller_;
fml::scoped_nsobject<FlutterClippingMaskViewPool> mask_view_pool_;
std::unordered_map<std::string, fml::scoped_nsobject<NSObject<FlutterPlatformViewFactory>>>
factories_;
UIView* flutter_view_;
UIViewController<FlutterViewResponder>* flutter_view_controller_;
FlutterClippingMaskViewPool* mask_view_pool_;
std::unordered_map<std::string, NSObject<FlutterPlatformViewFactory>*> factories_;

// The FlutterPlatformViewGestureRecognizersBlockingPolicy for each type of platform view.
std::unordered_map<std::string, FlutterPlatformViewGestureRecognizersBlockingPolicy>
Expand All @@ -264,9 +261,9 @@ class PlatformViewsController {
///
/// This data must only be accessed on the platform thread.
struct PlatformViewData {
fml::scoped_nsobject<NSObject<FlutterPlatformView>> view;
fml::scoped_nsobject<FlutterTouchInterceptingView> touch_interceptor;
fml::scoped_nsobject<UIView> root_view;
NSObject<FlutterPlatformView>* view;
FlutterTouchInterceptingView* touch_interceptor;
UIView* root_view;
};

/// This data must only be accessed on the platform thread.
Expand Down
Loading