Skip to content

Commit

Permalink
Merge enablePropIteratorSetter flags into a single one in CoreFeatures (
Browse files Browse the repository at this point in the history
#34869)

Summary:
Pull Request resolved: #34869

Changelog: [Internal]

This merges all instances of `enablePropIteratorSetter` into a single one.

Both `AccesibilityProps` and `BaseTextProps` had their own instances if it, which is now redundant.

Reviewed By: cipolleschi

Differential Revision: D40062555

fbshipit-source-id: b6ccf5a9538612dd731a6f9c4eaceeebcb6d95be
  • Loading branch information
rshest authored and facebook-github-bot committed Oct 5, 2022
1 parent bed977b commit aa5d43f
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 130 deletions.
2 changes: 0 additions & 2 deletions React/Fabric/RCTSurfacePresenter.mm
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,6 @@ - (RCTScheduler *)_createScheduler

if (reactNativeConfig && reactNativeConfig->getBool("react_fabric:enable_cpp_props_iterator_setter_ios")) {
CoreFeatures::enablePropIteratorSetter = true;
AccessibilityProps::enablePropIteratorSetter = true;
BaseTextProps::enablePropIteratorSetter = true;
}

auto componentRegistryFactory =
Expand Down
4 changes: 0 additions & 4 deletions ReactAndroid/src/main/jni/react/fabric/Binding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -458,10 +458,6 @@ void Binding::installFabricUIManager(
// Props setter pattern feature
CoreFeatures::enablePropIteratorSetter =
getFeatureFlagValue("enableCppPropsIteratorSetter");
AccessibilityProps::enablePropIteratorSetter =
CoreFeatures::enablePropIteratorSetter;
BaseTextProps::enablePropIteratorSetter =
CoreFeatures::enablePropIteratorSetter;

// RemoveDelete mega-op
ShadowViewMutation::PlatformSupportsRemoveDeleteTreeInstruction =
Expand Down
5 changes: 2 additions & 3 deletions ReactCommon/react/renderer/components/text/BaseTextProps.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,14 @@
#include "BaseTextProps.h"

#include <react/renderer/attributedstring/conversions.h>
#include <react/renderer/core/CoreFeatures.h>
#include <react/renderer/core/propsConversions.h>
#include <react/renderer/debug/DebugStringConvertibleItem.h>
#include <react/renderer/graphics/conversions.h>

namespace facebook {
namespace react {

bool BaseTextProps::enablePropIteratorSetter = false;

static TextAttributes convertRawProp(
PropsParserContext const &context,
RawProps const &rawProps,
Expand Down Expand Up @@ -195,7 +194,7 @@ BaseTextProps::BaseTextProps(
const BaseTextProps &sourceProps,
const RawProps &rawProps)
: textAttributes(
BaseTextProps::enablePropIteratorSetter
CoreFeatures::enablePropIteratorSetter
? sourceProps.textAttributes
: convertRawProp(
context,
Expand Down
2 changes: 0 additions & 2 deletions ReactCommon/react/renderer/components/text/BaseTextProps.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ class BaseTextProps {
const char *propName,
RawValue const &value);

static bool enablePropIteratorSetter;

#pragma mark - Props

TextAttributes textAttributes{};
Expand Down
247 changes: 130 additions & 117 deletions ReactCommon/react/renderer/components/view/AccessibilityProps.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,100 +9,108 @@

#include <react/renderer/components/view/accessibilityPropsConversions.h>
#include <react/renderer/components/view/propsConversions.h>
#include <react/renderer/core/CoreFeatures.h>
#include <react/renderer/core/propsConversions.h>
#include <react/renderer/debug/debugStringConvertibleUtils.h>

namespace facebook {
namespace react {

bool AccessibilityProps::enablePropIteratorSetter = false;

AccessibilityProps::AccessibilityProps(
const PropsParserContext &context,
AccessibilityProps const &sourceProps,
RawProps const &rawProps)
: accessible(
enablePropIteratorSetter ? sourceProps.accessible
: convertRawProp(
context,
rawProps,
"accessible",
sourceProps.accessible,
false)),
CoreFeatures::enablePropIteratorSetter ? sourceProps.accessible
: convertRawProp(
context,
rawProps,
"accessible",
sourceProps.accessible,
false)),
accessibilityState(
enablePropIteratorSetter ? sourceProps.accessibilityState
: convertRawProp(
context,
rawProps,
"accessibilityState",
sourceProps.accessibilityState,
{})),
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityState
: convertRawProp(
context,
rawProps,
"accessibilityState",
sourceProps.accessibilityState,
{})),
accessibilityLabel(
enablePropIteratorSetter ? sourceProps.accessibilityLabel
: convertRawProp(
context,
rawProps,
"accessibilityLabel",
sourceProps.accessibilityLabel,
"")),
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityLabel
: convertRawProp(
context,
rawProps,
"accessibilityLabel",
sourceProps.accessibilityLabel,
"")),
accessibilityLabelledBy(
enablePropIteratorSetter ? sourceProps.accessibilityLabelledBy
: convertRawProp(
context,
rawProps,
"accessibilityLabelledBy",
sourceProps.accessibilityLabelledBy,
{})),
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityLabelledBy
: convertRawProp(
context,
rawProps,
"accessibilityLabelledBy",
sourceProps.accessibilityLabelledBy,
{})),
accessibilityLiveRegion(
enablePropIteratorSetter ? sourceProps.accessibilityLiveRegion
: convertRawProp(
context,
rawProps,
"accessibilityLiveRegion",
sourceProps.accessibilityLiveRegion,
AccessibilityLiveRegion::None)),
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityLiveRegion
: convertRawProp(
context,
rawProps,
"accessibilityLiveRegion",
sourceProps.accessibilityLiveRegion,
AccessibilityLiveRegion::None)),
accessibilityHint(
enablePropIteratorSetter ? sourceProps.accessibilityHint
: convertRawProp(
context,
rawProps,
"accessibilityHint",
sourceProps.accessibilityHint,
"")),
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityHint
: convertRawProp(
context,
rawProps,
"accessibilityHint",
sourceProps.accessibilityHint,
"")),
accessibilityLanguage(
enablePropIteratorSetter ? sourceProps.accessibilityLanguage
: convertRawProp(
context,
rawProps,
"accessibilityLanguage",
sourceProps.accessibilityLanguage,
"")),
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityLanguage
: convertRawProp(
context,
rawProps,
"accessibilityLanguage",
sourceProps.accessibilityLanguage,
"")),
accessibilityValue(
enablePropIteratorSetter ? sourceProps.accessibilityValue
: convertRawProp(
context,
rawProps,
"accessibilityValue",
sourceProps.accessibilityValue,
{})),
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityValue
: convertRawProp(
context,
rawProps,
"accessibilityValue",
sourceProps.accessibilityValue,
{})),
accessibilityActions(
enablePropIteratorSetter ? sourceProps.accessibilityActions
: convertRawProp(
context,
rawProps,
"accessibilityActions",
sourceProps.accessibilityActions,
{})),
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityActions
: convertRawProp(
context,
rawProps,
"accessibilityActions",
sourceProps.accessibilityActions,
{})),
accessibilityViewIsModal(
enablePropIteratorSetter ? sourceProps.accessibilityViewIsModal
: convertRawProp(
context,
rawProps,
"accessibilityViewIsModal",
sourceProps.accessibilityViewIsModal,
false)),
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityViewIsModal
: convertRawProp(
context,
rawProps,
"accessibilityViewIsModal",
sourceProps.accessibilityViewIsModal,
false)),
accessibilityElementsHidden(
enablePropIteratorSetter
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityElementsHidden
: convertRawProp(
context,
Expand All @@ -111,7 +119,7 @@ AccessibilityProps::AccessibilityProps(
sourceProps.accessibilityElementsHidden,
false)),
accessibilityIgnoresInvertColors(
enablePropIteratorSetter
CoreFeatures::enablePropIteratorSetter
? sourceProps.accessibilityIgnoresInvertColors
: convertRawProp(
context,
Expand All @@ -120,61 +128,66 @@ AccessibilityProps::AccessibilityProps(
sourceProps.accessibilityIgnoresInvertColors,
false)),
onAccessibilityTap(
enablePropIteratorSetter ? sourceProps.onAccessibilityTap
: convertRawProp(
context,
rawProps,
"onAccessibilityTap",
sourceProps.onAccessibilityTap,
{})),
CoreFeatures::enablePropIteratorSetter
? sourceProps.onAccessibilityTap
: convertRawProp(
context,
rawProps,
"onAccessibilityTap",
sourceProps.onAccessibilityTap,
{})),
onAccessibilityMagicTap(
enablePropIteratorSetter ? sourceProps.onAccessibilityMagicTap
: convertRawProp(
context,
rawProps,
"onAccessibilityMagicTap",
sourceProps.onAccessibilityMagicTap,
{})),
CoreFeatures::enablePropIteratorSetter
? sourceProps.onAccessibilityMagicTap
: convertRawProp(
context,
rawProps,
"onAccessibilityMagicTap",
sourceProps.onAccessibilityMagicTap,
{})),
onAccessibilityEscape(
enablePropIteratorSetter ? sourceProps.onAccessibilityEscape
: convertRawProp(
context,
rawProps,
"onAccessibilityEscape",
sourceProps.onAccessibilityEscape,
{})),
CoreFeatures::enablePropIteratorSetter
? sourceProps.onAccessibilityEscape
: convertRawProp(
context,
rawProps,
"onAccessibilityEscape",
sourceProps.onAccessibilityEscape,
{})),
onAccessibilityAction(
enablePropIteratorSetter ? sourceProps.onAccessibilityAction
: convertRawProp(
context,
rawProps,
"onAccessibilityAction",
sourceProps.onAccessibilityAction,
{})),
CoreFeatures::enablePropIteratorSetter
? sourceProps.onAccessibilityAction
: convertRawProp(
context,
rawProps,
"onAccessibilityAction",
sourceProps.onAccessibilityAction,
{})),
importantForAccessibility(
enablePropIteratorSetter ? sourceProps.importantForAccessibility
: convertRawProp(
context,
rawProps,
"importantForAccessibility",
sourceProps.importantForAccessibility,
ImportantForAccessibility::Auto)),
CoreFeatures::enablePropIteratorSetter
? sourceProps.importantForAccessibility
: convertRawProp(
context,
rawProps,
"importantForAccessibility",
sourceProps.importantForAccessibility,
ImportantForAccessibility::Auto)),
testId(
enablePropIteratorSetter ? sourceProps.testId
: convertRawProp(
context,
rawProps,
"testID",
sourceProps.testId,
"")) {
CoreFeatures::enablePropIteratorSetter ? sourceProps.testId
: convertRawProp(
context,
rawProps,
"testID",
sourceProps.testId,
"")) {
// It is a (severe!) perf deoptimization to request props out-of-order.
// Thus, since we need to request the same prop twice here
// (accessibilityRole) we "must" do them subsequently here to prevent
// a regression. It is reasonable to ask if the `at` function can be improved;
// it probably can, but this is a fairly rare edge-case that (1) is easy-ish
// to work around here, and (2) would require very careful work to address
// this case and not regress the more common cases.
if (!enablePropIteratorSetter) {
if (!CoreFeatures::enablePropIteratorSetter) {
const auto *rawPropValue =
rawProps.at("accessibilityRole", nullptr, nullptr);
AccessibilityTraits traits;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ class AccessibilityProps {
const char *propName,
RawValue const &value);

static bool enablePropIteratorSetter;

#ifdef ANDROID
void propsDiffMapBuffer(Props const *oldProps, MapBufferBuilder &builder)
const;
Expand Down

0 comments on commit aa5d43f

Please sign in to comment.