From a7c026e667bb39bef1c8e4b956c6d0c31e43bb31 Mon Sep 17 00:00:00 2001 From: Paige Sun Date: Tue, 29 Sep 2020 14:15:45 -0700 Subject: [PATCH] Remove unused Fabric image instrumentation Summary: Remove the older implementation of image instrumentation in Fabric by removing, RCTImageInstrumentationProxy, ImageInstrumentation from ImageRequest, and trackURLImageContentDidSetForRequest from RCTImageLoaderWithAttributionProtocol. Changelog: [RN][Fabric][Image] Remove unused Fabric image instrumentation Reviewed By: fkgozali Differential Revision: D23990606 fbshipit-source-id: 004d04025d031af11377a73e5bfb64b1e0449962 --- Libraries/Image/RCTImageLoader.mm | 13 --- .../RCTImageLoaderWithAttributionProtocol.h | 5 -- .../Image/RCTImageURLLoaderWithAttribution.h | 5 -- .../Image/RCTImageComponentView.mm | 7 -- .../components/image/ImageShadowNode.h | 2 +- .../components/slider/SliderShadowNode.h | 8 +- ReactCommon/react/renderer/imagemanager/BUCK | 1 - .../imagemanager/ImageInstrumentation.h | 40 --------- .../renderer/imagemanager/ImageRequest.h | 23 +----- .../renderer/imagemanager/ImageManager.cpp | 2 +- .../renderer/imagemanager/ImageRequest.cpp | 21 +---- .../platform/ios/ImageRequest.cpp | 20 +---- .../ios/RCTImageInstrumentationProxy.h | 40 --------- .../ios/RCTImageInstrumentationProxy.mm | 81 ------------------- .../platform/ios/RCTImageManager.mm | 8 +- .../platform/ios/RCTSyncImageManager.mm | 2 +- 16 files changed, 15 insertions(+), 263 deletions(-) delete mode 100644 ReactCommon/react/renderer/imagemanager/ImageInstrumentation.h delete mode 100644 ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageInstrumentationProxy.h delete mode 100644 ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageInstrumentationProxy.mm diff --git a/Libraries/Image/RCTImageLoader.mm b/Libraries/Image/RCTImageLoader.mm index 0ea5a4a0c98..723b64dbe61 100644 --- a/Libraries/Image/RCTImageLoader.mm +++ b/Libraries/Image/RCTImageLoader.mm @@ -831,19 +831,6 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image, return nil; } -- (void)trackURLImageContentDidSetForRequest:(RCTImageURLLoaderRequest *)loaderRequest -{ - if (!loaderRequest) { - return; - } - - // This delegate method is Fabric-only - id loadHandler = [self imageURLLoaderForURL:loaderRequest.imageURL]; - if ([loadHandler respondsToSelector:@selector(trackURLImageContentDidSetForRequest:)]) { - [(id)loadHandler trackURLImageContentDidSetForRequest:loaderRequest]; - } -} - - (void)trackURLImageVisibilityForRequest:(RCTImageURLLoaderRequest *)loaderRequest imageView:(UIView *)imageView { if (!loaderRequest || !imageView) { diff --git a/Libraries/Image/RCTImageLoaderWithAttributionProtocol.h b/Libraries/Image/RCTImageLoaderWithAttributionProtocol.h index bd26221531b..44f4ee849a4 100644 --- a/Libraries/Image/RCTImageLoaderWithAttributionProtocol.h +++ b/Libraries/Image/RCTImageLoaderWithAttributionProtocol.h @@ -36,11 +36,6 @@ RCT_EXTERN void RCTEnableImageLoadingPerfInstrumentation(BOOL enabled); completionBlock:(RCTImageLoaderCompletionBlockWithMetadata)completionBlock; #endif -/** - * Image instrumentation - notify that the image content (UIImage) has been set on the native view. - */ -- (void)trackURLImageContentDidSetForRequest:(RCTImageURLLoaderRequest *)loaderRequest; - /** * Image instrumentation - start tracking the on-screen visibility of the native image view. */ diff --git a/Libraries/Image/RCTImageURLLoaderWithAttribution.h b/Libraries/Image/RCTImageURLLoaderWithAttribution.h index b05ebfba8ad..4013367b4d3 100644 --- a/Libraries/Image/RCTImageURLLoaderWithAttribution.h +++ b/Libraries/Image/RCTImageURLLoaderWithAttribution.h @@ -59,11 +59,6 @@ struct ImageURLLoaderAttribution { completionHandler:(RCTImageLoaderCompletionBlockWithMetadata)completionHandler; #endif -/** - * Image instrumentation - notify that the image content (UIImage) has been set on the native view. - */ -- (void)trackURLImageContentDidSetForRequest:(RCTImageURLLoaderRequest *)loaderRequest; - /** * Image instrumentation - start tracking the on-screen visibility of the native image view. */ diff --git a/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm b/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm index ce8e55c0229..09dd599650f 100644 --- a/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm @@ -13,9 +13,7 @@ #import #import #import -#import #import -#import #import using namespace facebook::react; @@ -160,11 +158,6 @@ using namespace facebook::react; if (!data.hasValue()) { return; } - auto instrumentation = std::static_pointer_cast( - data.value().getImageRequest().getSharedImageInstrumentation()); - if (instrumentation) { - instrumentation->didSetImage(); - } }; if (imageProps.blurRadius > __FLT_EPSILON__) { diff --git a/ReactCommon/react/renderer/components/image/ImageShadowNode.h b/ReactCommon/react/renderer/components/image/ImageShadowNode.h index 12490840b61..0e60f78deef 100644 --- a/ReactCommon/react/renderer/components/image/ImageShadowNode.h +++ b/ReactCommon/react/renderer/components/image/ImageShadowNode.h @@ -46,7 +46,7 @@ class ImageShadowNode final : public ConcreteViewShadowNode< ShadowNodeFamilyFragment const &familyFragment, ComponentDescriptor const &componentDescriptor) { auto imageSource = ImageSource{ImageSource::Type::Invalid}; - return {imageSource, {imageSource, nullptr, nullptr}, 0}; + return {imageSource, {imageSource, nullptr}, 0}; } #pragma mark - LayoutableShadowNode diff --git a/ReactCommon/react/renderer/components/slider/SliderShadowNode.h b/ReactCommon/react/renderer/components/slider/SliderShadowNode.h index c012ff45a51..82ab4d0b1dc 100644 --- a/ReactCommon/react/renderer/components/slider/SliderShadowNode.h +++ b/ReactCommon/react/renderer/components/slider/SliderShadowNode.h @@ -44,13 +44,13 @@ class SliderShadowNode final : public ConcreteViewShadowNode< ComponentDescriptor const &componentDescriptor) { auto imageSource = ImageSource{ImageSource::Type::Invalid}; return {imageSource, - {imageSource, nullptr, nullptr}, + {imageSource, nullptr}, imageSource, - {imageSource, nullptr, nullptr}, + {imageSource, nullptr}, imageSource, - {imageSource, nullptr, nullptr}, + {imageSource, nullptr}, imageSource, - {imageSource, nullptr, nullptr}}; + {imageSource, nullptr}}; } #pragma mark - LayoutableShadowNode diff --git a/ReactCommon/react/renderer/imagemanager/BUCK b/ReactCommon/react/renderer/imagemanager/BUCK index a9797d2884b..52f99b3fec6 100644 --- a/ReactCommon/react/renderer/imagemanager/BUCK +++ b/ReactCommon/react/renderer/imagemanager/BUCK @@ -70,7 +70,6 @@ rn_xplat_cxx_library( ios_exported_headers = subdir_glob( [ ("", "*.h"), - ("platform/ios", "RCTImageInstrumentationProxy.h"), ("platform/ios", "RCTImagePrimitivesConversions.h"), ], prefix = "react/renderer/imagemanager", diff --git a/ReactCommon/react/renderer/imagemanager/ImageInstrumentation.h b/ReactCommon/react/renderer/imagemanager/ImageInstrumentation.h deleted file mode 100644 index c1671cb6790..00000000000 --- a/ReactCommon/react/renderer/imagemanager/ImageInstrumentation.h +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#pragma once - -#include - -namespace facebook { -namespace react { - -/* - * A base class for performing image loading instrumentation. - * The actual instrumentation is app, platform, and image loader-specific. - */ -class ImageInstrumentation { - public: - virtual ~ImageInstrumentation() noexcept = default; - - /** - * Mark that the image content is set on the native image component on screen. - */ - virtual void didSetImage() const = 0; - - /** - * Mark that the image view starts to be visible on screen. - */ - virtual void didEnterVisibilityRange() const = 0; - - /** - * Mark that the image view is no longer visible on screen. - */ - virtual void didExitVisibilityRange() const = 0; -}; - -} // namespace react -} // namespace facebook diff --git a/ReactCommon/react/renderer/imagemanager/ImageRequest.h b/ReactCommon/react/renderer/imagemanager/ImageRequest.h index a77c1f76c79..e3f30475664 100644 --- a/ReactCommon/react/renderer/imagemanager/ImageRequest.h +++ b/ReactCommon/react/renderer/imagemanager/ImageRequest.h @@ -7,7 +7,6 @@ #pragma once -#include #include #include #include @@ -31,8 +30,7 @@ class ImageRequest final { */ ImageRequest( const ImageSource &imageSource, - std::shared_ptr telemetry, - std::shared_ptr instrumentation); + std::shared_ptr telemetry); /* * The move constructor. @@ -78,20 +76,6 @@ class ImageRequest final { */ const std::shared_ptr &getSharedTelemetry() const; - /* - * Returns stored image instrumentation object as a shared pointer. - * Retain this *or* `ImageRequest` to ensure a correct lifetime of the object. - */ - const std::shared_ptr - &getSharedImageInstrumentation() const; - - /* - * Returns the image instrumentation object specific to this request. - * Use this if a correct lifetime of the object is ensured in some other way - * (e.g. by retaining an `ImageRequest`). - */ - const ImageInstrumentation &getImageInstrumentation() const; - private: /* * Image source associated with the request. @@ -108,11 +92,6 @@ class ImageRequest final { */ std::shared_ptr coordinator_{}; - /* - * Image instrumentation specific to the request. - */ - std::shared_ptr instrumentation_; - /* * Function we can call to cancel image request (see destructor). */ diff --git a/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageManager.cpp b/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageManager.cpp index c1d4d1ac63e..245c3b5e7a7 100644 --- a/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageManager.cpp +++ b/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageManager.cpp @@ -24,7 +24,7 @@ ImageRequest ImageManager::requestImage( const ImageSource &imageSource, SurfaceId surfaceId) const { // Not implemented. - return ImageRequest(imageSource, nullptr, nullptr); + return ImageRequest(imageSource, nullptr); } } // namespace react diff --git a/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageRequest.cpp b/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageRequest.cpp index 9cdbc7fdd0d..9f811bc4d4a 100644 --- a/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageRequest.cpp +++ b/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageRequest.cpp @@ -12,19 +12,15 @@ namespace react { ImageRequest::ImageRequest( const ImageSource &imageSource, - std::shared_ptr telemetry, - std::shared_ptr instrumentation) - : imageSource_(imageSource), - telemetry_(telemetry), - instrumentation_(instrumentation) { + std::shared_ptr telemetry) + : imageSource_(imageSource), telemetry_(telemetry) { // Not implemented. } ImageRequest::ImageRequest(ImageRequest &&other) noexcept : imageSource_(std::move(other.imageSource_)), telemetry_(std::move(other.telemetry_)), - coordinator_(std::move(other.coordinator_)), - instrumentation_(std::move(other.instrumentation_)) { + coordinator_(std::move(other.coordinator_)) { // Not implemented. } @@ -44,16 +40,5 @@ const std::shared_ptr abort(); } -const std::shared_ptr - &ImageRequest::getSharedImageInstrumentation() const { - // Not implemented - abort(); -} - -const ImageInstrumentation &ImageRequest::getImageInstrumentation() const { - // Not implemented - abort(); -} - } // namespace react } // namespace facebook diff --git a/ReactCommon/react/renderer/imagemanager/platform/ios/ImageRequest.cpp b/ReactCommon/react/renderer/imagemanager/platform/ios/ImageRequest.cpp index 216e1ffdfd9..ce7f9b2d045 100644 --- a/ReactCommon/react/renderer/imagemanager/platform/ios/ImageRequest.cpp +++ b/ReactCommon/react/renderer/imagemanager/platform/ios/ImageRequest.cpp @@ -12,24 +12,19 @@ namespace react { ImageRequest::ImageRequest( const ImageSource &imageSource, - std::shared_ptr telemetry, - std::shared_ptr instrumentation) - : imageSource_(imageSource), - telemetry_(telemetry), - instrumentation_(instrumentation) { + std::shared_ptr telemetry) + : imageSource_(imageSource), telemetry_(telemetry) { coordinator_ = std::make_shared(); } ImageRequest::ImageRequest(ImageRequest &&other) noexcept : imageSource_(std::move(other.imageSource_)), telemetry_(std::move(other.telemetry_)), - coordinator_(std::move(other.coordinator_)), - instrumentation_(std::move(other.instrumentation_)) { + coordinator_(std::move(other.coordinator_)) { other.moved_ = true; other.coordinator_ = nullptr; other.cancelRequest_ = nullptr; other.telemetry_ = nullptr; - other.instrumentation_ = nullptr; } ImageRequest::~ImageRequest() { @@ -58,14 +53,5 @@ const std::shared_ptr return coordinator_; } -const std::shared_ptr - &ImageRequest::getSharedImageInstrumentation() const { - return instrumentation_; -} - -const ImageInstrumentation &ImageRequest::getImageInstrumentation() const { - return *instrumentation_; -} - } // namespace react } // namespace facebook diff --git a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageInstrumentationProxy.h b/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageInstrumentationProxy.h deleted file mode 100644 index 95726a5ff0c..00000000000 --- a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageInstrumentationProxy.h +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#pragma once - -#include - -#include -#include - -NS_ASSUME_NONNULL_BEGIN - -namespace facebook { -namespace react { - -class RCTImageInstrumentationProxy final : public ImageInstrumentation { - public: - RCTImageInstrumentationProxy(id imageLoader); - ~RCTImageInstrumentationProxy(); - - void didSetImage() const override; - void didEnterVisibilityRange() const override; - void didExitVisibilityRange() const override; - - void trackNativeImageView(UIView *imageView) const; - void setImageURLLoaderRequest(RCTImageURLLoaderRequest *request); - - private: - __weak id imageLoader_; - RCTImageURLLoaderRequest *imageURLLoaderRequest_; -}; - -} // namespace react -} // namespace facebook - -NS_ASSUME_NONNULL_END diff --git a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageInstrumentationProxy.mm b/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageInstrumentationProxy.mm deleted file mode 100644 index 1ed24f9faac..00000000000 --- a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageInstrumentationProxy.mm +++ /dev/null @@ -1,81 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#import "RCTImageInstrumentationProxy.h" - -namespace facebook { -namespace react { - -RCTImageInstrumentationProxy::RCTImageInstrumentationProxy(id imageLoader) - : imageLoader_(imageLoader) -{ -} - -RCTImageInstrumentationProxy::~RCTImageInstrumentationProxy() -{ - if (!imageURLLoaderRequest_) { - return; - } - [imageLoader_ trackURLImageDidDestroy:imageURLLoaderRequest_]; -} - -void RCTImageInstrumentationProxy::didSetImage() const -{ - if (!RCTImageLoadingPerfInstrumentationEnabled()) { - return; - } - - if (!imageURLLoaderRequest_) { - return; - } - - [imageLoader_ trackURLImageContentDidSetForRequest:imageURLLoaderRequest_]; -} - -void RCTImageInstrumentationProxy::didEnterVisibilityRange() const -{ - if (!RCTImageLoadingPerfInstrumentationEnabled()) { - return; - } - - // TODO (T58941612): Not yet supported. - if (!imageURLLoaderRequest_) { - return; - } -} - -void RCTImageInstrumentationProxy::didExitVisibilityRange() const -{ - if (!RCTImageLoadingPerfInstrumentationEnabled()) { - return; - } - - // TODO (T58941612): Not yet supported. - if (!imageURLLoaderRequest_) { - return; - } -} - -void RCTImageInstrumentationProxy::trackNativeImageView(UIView *imageView) const -{ - if (!RCTImageLoadingPerfInstrumentationEnabled()) { - return; - } - - if (!imageURLLoaderRequest_) { - return; - } - [imageLoader_ trackURLImageVisibilityForRequest:imageURLLoaderRequest_ imageView:imageView]; -} - -void RCTImageInstrumentationProxy::setImageURLLoaderRequest(RCTImageURLLoaderRequest *request) -{ - imageURLLoaderRequest_ = request; -} - -} // namespace react -} // namespace facebook diff --git a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageManager.mm b/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageManager.mm index b719be1158f..08e0c255885 100644 --- a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageManager.mm +++ b/ReactCommon/react/renderer/imagemanager/platform/ios/RCTImageManager.mm @@ -16,7 +16,6 @@ #import #import -#import "RCTImageInstrumentationProxy.h" #import "RCTImagePrimitivesConversions.h" using namespace facebook::react; @@ -41,10 +40,9 @@ using namespace facebook::react; { SystraceSection s("RCTImageManager::requestImage"); - auto imageInstrumentation = std::make_shared(_imageLoader); auto telemetry = std::make_shared(surfaceId); telemetry->willRequestUrl(); - auto imageRequest = ImageRequest(imageSource, telemetry, imageInstrumentation); + auto imageRequest = ImageRequest(imageSource, telemetry); auto weakObserverCoordinator = (std::weak_ptr)imageRequest.getSharedObserverCoordinator(); @@ -108,10 +106,6 @@ using namespace facebook::react; completionBlock:completionBlock]; RCTImageLoaderCancellationBlock cancelationBlock = loaderRequest.cancellationBlock; sharedCancelationFunction.assign([cancelationBlock]() { cancelationBlock(); }); - - if (imageInstrumentation) { - imageInstrumentation->setImageURLLoaderRequest(loaderRequest); - } }); return imageRequest; diff --git a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTSyncImageManager.mm b/ReactCommon/react/renderer/imagemanager/platform/ios/RCTSyncImageManager.mm index df89da51cec..7a36d42ea4a 100644 --- a/ReactCommon/react/renderer/imagemanager/platform/ios/RCTSyncImageManager.mm +++ b/ReactCommon/react/renderer/imagemanager/platform/ios/RCTSyncImageManager.mm @@ -37,7 +37,7 @@ using namespace facebook::react; - (ImageRequest)requestImage:(ImageSource)imageSource surfaceId:(SurfaceId)surfaceId { auto telemetry = std::make_shared(surfaceId); - auto imageRequest = ImageRequest(imageSource, telemetry, nullptr); + auto imageRequest = ImageRequest(imageSource, telemetry); auto weakObserverCoordinator = (std::weak_ptr)imageRequest.getSharedObserverCoordinator();