Fabric: Enforcing const-correctness around ImageResponseObserverCoordinator

Summary:
`addObserver` and `removeObserver` now accepts const references instead of pointers which indicates the intent (non-nullability and non-owning) clearly. The delegate methods are also marked as `const` to designate the possible concurrent execution (`const` means "thread-safe" here).

All changes are pure syntactical, nothing really changes (besides the fact overall code quality and redability).

Reviewed By: JoshuaGross

Differential Revision: D17535395

fbshipit-source-id: b0c6c872d44fee22e38fd067ccd3320e7231c94a

React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm

@@ -20,14 +20,14 @@
 @implementation RCTImageComponentView {
   UIImageView *_imageView;
   SharedImageLocalData _imageLocalData;
-  const ImageResponseObserverCoordinator *_coordinator;
+  ImageResponseObserverCoordinator const *_coordinator;
   std::unique_ptr<RCTImageResponseObserverProxy> _imageResponseObserverProxy;
 }
 
 - (instancetype)initWithFrame:(CGRect)frame
 {
   if (self = [super initWithFrame:frame]) {
-    static const auto defaultProps = std::make_shared<const ImageProps>();
+    static auto const defaultProps = std::make_shared<ImageProps const>();
     _props = defaultProps;
 
     _imageView = [[UIImageView alloc] initWithFrame:self.bounds];
@@ -52,8 +52,8 @@
 
 - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const &)oldProps
 {
-  const auto &oldImageProps = *std::static_pointer_cast<const ImageProps>(_props);
-  const auto &newImageProps = *std::static_pointer_cast<const ImageProps>(props);
+  auto const &oldImageProps = *std::static_pointer_cast<ImageProps const>(_props);
+  auto const &newImageProps = *std::static_pointer_cast<ImageProps const>(props);
 
   // `resizeMode`
   if (oldImageProps.resizeMode != newImageProps.resizeMode) {
@@ -76,7 +76,7 @@
 
 - (void)updateLocalData:(SharedLocalData)localData oldLocalData:(SharedLocalData)oldLocalData
 {
-  auto imageLocalData = std::static_pointer_cast<const ImageLocalData>(localData);
+  auto imageLocalData = std::static_pointer_cast<ImageLocalData const>(localData);
 
   // This call (setting `coordinator`) must be unconditional (at the same block as setting `LocalData`)
   // because the setter stores a raw pointer to object that `LocalData` owns.
@@ -96,18 +96,18 @@
   if (!havePreviousData || _imageLocalData->getImageSource() != previousData->getImageSource()) {
     // Loading actually starts a little before this, but this is the first time we know
     // the image is loading and can fire an event from this component
-    std::static_pointer_cast<const ImageEventEmitter>(_eventEmitter)->onLoadStart();
+    std::static_pointer_cast<ImageEventEmitter const>(_eventEmitter)->onLoadStart();
   }
 }
 
-- (void)setCoordinator:(const ImageResponseObserverCoordinator *)coordinator
+- (void)setCoordinator:(ImageResponseObserverCoordinator const *)coordinator
 {
   if (_coordinator) {
-    _coordinator->removeObserver(_imageResponseObserverProxy.get());
+    _coordinator->removeObserver(*_imageResponseObserverProxy);
   }
   _coordinator = coordinator;
   if (_coordinator != nullptr) {
-    _coordinator->addObserver(_imageResponseObserverProxy.get());
+    _coordinator->addObserver(*_imageResponseObserverProxy);
   }
 }
 
@@ -127,7 +127,7 @@
 
 #pragma mark - RCTImageResponseDelegate
 
-- (void)didReceiveImage:(UIImage *)image fromObserver:(void *)observer
+- (void)didReceiveImage:(UIImage *)image fromObserver:(void const *)observer
 {
   if (!_eventEmitter) {
     // Notifications are delivered asynchronously and might arrive after the view is already recycled.
@@ -136,9 +136,9 @@
     return;
   }
 
-  std::static_pointer_cast<const ImageEventEmitter>(_eventEmitter)->onLoad();
+  std::static_pointer_cast<ImageEventEmitter const>(_eventEmitter)->onLoad();
 
-  const auto &imageProps = *std::static_pointer_cast<const ImageProps>(_props);
+  const auto &imageProps = *std::static_pointer_cast<ImageProps const>(_props);
 
   if (imageProps.tintColor) {
     image = [image imageWithRenderingMode:UIImageRenderingModeAlwaysTemplate];
@@ -159,19 +159,19 @@
   self->_imageView.layer.minificationFilter = kCAFilterTrilinear;
   self->_imageView.layer.magnificationFilter = kCAFilterTrilinear;
 
-  std::static_pointer_cast<const ImageEventEmitter>(self->_eventEmitter)->onLoadEnd();
+  std::static_pointer_cast<ImageEventEmitter const>(self->_eventEmitter)->onLoadEnd();
 }
 
-- (void)didReceiveProgress:(float)progress fromObserver:(void *)observer
+- (void)didReceiveProgress:(float)progress fromObserver:(void const *)observer
 {
   if (!_eventEmitter) {
     return;
   }
 
-  std::static_pointer_cast<const ImageEventEmitter>(_eventEmitter)->onProgress(progress);
+  std::static_pointer_cast<ImageEventEmitter const>(_eventEmitter)->onProgress(progress);
 }
 
-- (void)didReceiveFailureFromObserver:(void *)observer
+- (void)didReceiveFailureFromObserver:(void const *)observer
 {
   _imageView.image = nil;
 
@@ -179,7 +179,7 @@
     return;
   }
 
-  std::static_pointer_cast<const ImageEventEmitter>(_eventEmitter)->onError();
+  std::static_pointer_cast<ImageEventEmitter const>(_eventEmitter)->onError();
 }
 
 @end

React/Fabric/Mounting/ComponentViews/Slider/RCTSliderComponentView.mm

@@ -177,44 +177,44 @@ using namespace facebook::react;
 - (void)setTrackImageCoordinator:(const ImageResponseObserverCoordinator *)coordinator
 {
   if (_trackImageCoordinator) {
-    _trackImageCoordinator->removeObserver(_trackImageResponseObserverProxy.get());
+    _trackImageCoordinator->removeObserver(*_trackImageResponseObserverProxy);
   }
   _trackImageCoordinator = coordinator;
   if (_trackImageCoordinator) {
-    _trackImageCoordinator->addObserver(_trackImageResponseObserverProxy.get());
+    _trackImageCoordinator->addObserver(*_trackImageResponseObserverProxy);
   }
 }
 
 - (void)setMinimumTrackImageCoordinator:(const ImageResponseObserverCoordinator *)coordinator
 {
   if (_minimumTrackImageCoordinator) {
-    _minimumTrackImageCoordinator->removeObserver(_minimumTrackImageResponseObserverProxy.get());
+    _minimumTrackImageCoordinator->removeObserver(*_minimumTrackImageResponseObserverProxy);
   }
   _minimumTrackImageCoordinator = coordinator;
   if (_minimumTrackImageCoordinator) {
-    _minimumTrackImageCoordinator->addObserver(_minimumTrackImageResponseObserverProxy.get());
+    _minimumTrackImageCoordinator->addObserver(*_minimumTrackImageResponseObserverProxy);
   }
 }
 
 - (void)setMaximumTrackImageCoordinator:(const ImageResponseObserverCoordinator *)coordinator
 {
   if (_maximumTrackImageCoordinator) {
-    _maximumTrackImageCoordinator->removeObserver(_maximumTrackImageResponseObserverProxy.get());
+    _maximumTrackImageCoordinator->removeObserver(*_maximumTrackImageResponseObserverProxy);
   }
   _maximumTrackImageCoordinator = coordinator;
   if (_maximumTrackImageCoordinator) {
-    _maximumTrackImageCoordinator->addObserver(_maximumTrackImageResponseObserverProxy.get());
+    _maximumTrackImageCoordinator->addObserver(*_maximumTrackImageResponseObserverProxy);
   }
 }
 
 - (void)setThumbImageCoordinator:(const ImageResponseObserverCoordinator *)coordinator
 {
   if (_thumbImageCoordinator) {
-    _thumbImageCoordinator->removeObserver(_thumbImageResponseObserverProxy.get());
+    _thumbImageCoordinator->removeObserver(*_thumbImageResponseObserverProxy);
   }
   _thumbImageCoordinator = coordinator;
   if (_thumbImageCoordinator) {
-    _thumbImageCoordinator->addObserver(_thumbImageResponseObserverProxy.get());
+    _thumbImageCoordinator->addObserver(*_thumbImageResponseObserverProxy);
   }
 }
 

React/Fabric/RCTImageResponseDelegate.h

@@ -11,9 +11,9 @@ NS_ASSUME_NONNULL_BEGIN
 
 @protocol RCTImageResponseDelegate <NSObject>
 
-- (void)didReceiveImage:(UIImage *)image fromObserver:(void*)observer;
-- (void)didReceiveProgress:(float)progress fromObserver:(void*)observer;
-- (void)didReceiveFailureFromObserver:(void*)observer;
+- (void)didReceiveImage:(UIImage *)image fromObserver:(void const *)observer;
+- (void)didReceiveProgress:(float)progress fromObserver:(void const *)observer;
+- (void)didReceiveFailureFromObserver:(void const *)observer;
 
 @end
 

React/Fabric/RCTImageResponseObserverProxy.h

@@ -15,16 +15,19 @@ NS_ASSUME_NONNULL_BEGIN
 
 namespace facebook {
 namespace react {
-class RCTImageResponseObserverProxy : public ImageResponseObserver {
+
+class RCTImageResponseObserverProxy final : public ImageResponseObserver {
  public:
   RCTImageResponseObserverProxy(void *delegate);
-  void didReceiveImage(const ImageResponse &imageResponse) override;
-  void didReceiveProgress(float p) override;
-  void didReceiveFailure() override;
+
+  void didReceiveImage(ImageResponse const &imageResponse) const override;
+  void didReceiveProgress(float progress) const override;
+  void didReceiveFailure() const override;
 
  private:
   __weak id<RCTImageResponseDelegate> delegate_;
 };
+
 } // namespace react
 } // namespace facebook
 

React/Fabric/RCTImageResponseObserverProxy.mm

@@ -18,26 +18,26 @@ RCTImageResponseObserverProxy::RCTImageResponseObserverProxy(void *delegate)
 {
 }
 
-void RCTImageResponseObserverProxy::didReceiveImage(const ImageResponse &imageResponse)
+void RCTImageResponseObserverProxy::didReceiveImage(ImageResponse const &imageResponse) const
 {
   UIImage *image = (__bridge UIImage *)imageResponse.getImage().get();
-  void *this_ = this;
+  auto this_ = this;
   dispatch_async(dispatch_get_main_queue(), ^{
     [delegate_ didReceiveImage:image fromObserver:this_];
   });
 }
 
-void RCTImageResponseObserverProxy::didReceiveProgress(float p)
+void RCTImageResponseObserverProxy::didReceiveProgress(float progress) const
 {
-  void *this_ = this;
+  auto this_ = this;
   dispatch_async(dispatch_get_main_queue(), ^{
-    [delegate_ didReceiveProgress:p fromObserver:this_];
+    [delegate_ didReceiveProgress:progress fromObserver:this_];
   });
 }
 
-void RCTImageResponseObserverProxy::didReceiveFailure()
+void RCTImageResponseObserverProxy::didReceiveFailure() const
 {
-  void *this_ = this;
+  auto this_ = this;
   dispatch_async(dispatch_get_main_queue(), ^{
     [delegate_ didReceiveFailureFromObserver:this_];
   });

ReactCommon/fabric/imagemanager/ImageResponseObserver.h

@@ -14,13 +14,15 @@ namespace react {
 
 /*
  * Represents any observer of ImageResponse progression, completion, or failure.
+ * All methods must be thread-safe.
  */
 class ImageResponseObserver {
  public:
-  virtual void didReceiveProgress(float) = 0;
-  virtual void didReceiveImage(const ImageResponse &imageResponse) = 0;
-  virtual void didReceiveFailure() = 0;
   virtual ~ImageResponseObserver() noexcept = default;
+
+  virtual void didReceiveProgress(float progress) const = 0;
+  virtual void didReceiveImage(ImageResponse const &imageResponse) const = 0;
+  virtual void didReceiveFailure() const = 0;
 };
 
 } // namespace react

ReactCommon/fabric/imagemanager/ImageResponseObserverCoordinator.cpp

@@ -14,34 +14,34 @@ namespace facebook {
 namespace react {
 
 void ImageResponseObserverCoordinator::addObserver(
-    ImageResponseObserver *observer) const {
+    ImageResponseObserver const &observer) const {
   mutex_.lock();
   switch (status_) {
     case ImageResponse::Status::Loading: {
-      observers_.push_back(observer);
+      observers_.push_back(&observer);
       mutex_.unlock();
       break;
     }
     case ImageResponse::Status::Completed: {
       auto imageData = imageData_;
       mutex_.unlock();
-      observer->didReceiveImage(ImageResponse{imageData});
+      observer.didReceiveImage(ImageResponse{imageData});
       break;
     }
     case ImageResponse::Status::Failed: {
       mutex_.unlock();
-      observer->didReceiveFailure();
+      observer.didReceiveFailure();
       break;
     }
   }
 }
 
 void ImageResponseObserverCoordinator::removeObserver(
-    ImageResponseObserver *observer) const {
+    ImageResponseObserver const &observer) const {
   std::lock_guard<std::mutex> lock(mutex_);
 
   // We remove only one element to maintain a balance between add/remove calls.
-  auto position = std::find(observers_.begin(), observers_.end(), observer);
+  auto position = std::find(observers_.begin(), observers_.end(), &observer);
   if (position != observers_.end()) {
     observers_.erase(position, observers_.end());
   }
@@ -60,7 +60,7 @@ void ImageResponseObserverCoordinator::nativeImageResponseProgress(
 }
 
 void ImageResponseObserverCoordinator::nativeImageResponseComplete(
-    const ImageResponse &imageResponse) const {
+    ImageResponse const &imageResponse) const {
   mutex_.lock();
   imageData_ = imageResponse.getImage();
   assert(status_ == ImageResponse::Status::Loading);

ReactCommon/fabric/imagemanager/ImageResponseObserverCoordinator.h

@@ -30,23 +30,23 @@ class ImageResponseObserverCoordinator {
    * If the current image request status is not equal to `Loading`, the observer
    * will be called immediately.
    */
-  void addObserver(ImageResponseObserver *observer) const;
+  void addObserver(ImageResponseObserver const &observer) const;
 
   /*
    * Interested parties may stop observing the image response.
    */
-  void removeObserver(ImageResponseObserver *observer) const;
+  void removeObserver(ImageResponseObserver const &observer) const;
 
   /*
    * Platform-specific image loader will call this method with progress updates.
    */
-  void nativeImageResponseProgress(float) const;
+  void nativeImageResponseProgress(float progress) const;
 
   /*
    * Platform-specific image loader will call this method with a completed image
    * response.
    */
-  void nativeImageResponseComplete(const ImageResponse &imageResponse) const;
+  void nativeImageResponseComplete(ImageResponse const &imageResponse) const;
 
   /*
    * Platform-specific image loader will call this method in case of any
@@ -59,7 +59,7 @@ class ImageResponseObserverCoordinator {
    * List of observers.
    * Mutable: protected by mutex_.
    */
-  mutable better::small_vector<ImageResponseObserver *, 1> observers_;
+  mutable better::small_vector<ImageResponseObserver const *, 1> observers_;
 
   /*
    * Current status of image loading.