Let CRWWebViewScrollViewProxy correctly handle UIScrollView categories

CRWWebViewScrollViewProxy also now returns the default values of the
properties instead of zero values when the underlying scroll view is
absent. Update unit tests to reflect the change.

Change-Id: I3023999c420b1f1bdf47d1133029ca454046e91a
Bug: 1023250
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2102093
Commit-Queue: Hiroshi Ichikawa <ichikawa@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755276}

ios/web/web_state/ui/crw_web_view_scroll_view_proxy+internal.h

@@ -17,11 +17,16 @@
 @property(nonatomic, readonly)
     CRBProtocolObservers<CRWWebViewScrollViewProxyObserver>* observers;
 
-// The underlying UIScrollView. It can change or become nil.
+// The underlying UIScrollView. It can change.
 //
-// This must be a strong reference to ensure that this proxy is aware of the
-// timing when this becomes nil. The proxy needs to save the properties of the
-// underlying scroll view when it happens.
+// The property supports assigning nil, but it returns a placeholder scroll view
+// instead of nil in that case.
+//
+// This must be a strong reference to:
+//   - avoid situation when the underlying scroll view is deallocated while
+//     associated with the proxy, which would prevent the proxy to preserve its
+//     properties
+//   - retain the placeholder scroll view
 @property(nonatomic, readonly) UIScrollView* underlyingScrollView;
 
 @end

ios/web/web_state/ui/crw_web_view_scroll_view_proxy.mm

@@ -84,6 +84,9 @@
 // This exists for compatibility with UIScrollView (see -asUIScrollView).
 @property(nonatomic, weak) id<UIScrollViewDelegate> delegate;
 
+// YES while key-value observers are registered on the underlying UIScrollView.
+@property(nonatomic) BOOL observingScrollView;
+
 // Returns the key paths that need to be observed for UIScrollView.
 + (NSArray*)scrollViewObserverKeyPaths;
 
@@ -247,6 +250,22 @@
         initWithScrollViewProxy:self];
     _keyValueObserverForwarders = [[NSMutableDictionary alloc] init];
 
+    // Assign a placeholder UIScrollView until the actual underlying scroll view
+    // is set. This must be a real UIScrollView, not nil, so that:
+    //   - The proxy preserves the values of properties assigned before the
+    //     actual scroll view is set. These properties will then be inherited to
+    //     the actual scroll view in -setScrollView:.
+    //   - The proxy returns the actual default value of the property before the
+    //     actual scroll view is set, even when the default value is non-zero
+    //     e.g., scrollsToTop.
+    //   - The proxy uses the actual implementation of methods defined in
+    //     third-party categories of UIScrollView.
+    //
+    // Note that this proxy must support all methods/properties of UIScrollView,
+    // including those defined in third-party categories, because it provides
+    // -asUIScrollView method.
+    _underlyingScrollView = [[UIScrollView alloc] init];
+
     if (base::FeatureList::IsEnabled(
             web::features::kPreserveScrollViewProperties)) {
       _propertiesStore = [self.class propertiesStore];
@@ -271,6 +290,12 @@
   if (self.underlyingScrollView == scrollView)
     return;
 
+  // Use a placeholder UIScrollView instead when nil is given. See the comment
+  // in -init why this is necessary.
+  if (!scrollView) {
+    scrollView = [[UIScrollView alloc] init];
+  }
+
   // Clean up the delegate/observers of the old scroll view, and save its
   // properties for later restoration.
   [self.underlyingScrollView setDelegate:nil];
@@ -278,6 +303,8 @@
   if (base::FeatureList::IsEnabled(
           web::features::kPreserveScrollViewProperties) &&
       self.underlyingScrollView) {
+    // TODO(crbug.com/1023250): Simplify this by directly assigning the
+    // properties of the old scroll view to those in the new scroll view.
     [_propertiesStore savePropertiesFromObject:self.underlyingScrollView];
   }
 
@@ -289,6 +316,8 @@
   if (base::FeatureList::IsEnabled(
           web::features::kPreserveScrollViewProperties) &&
       scrollView) {
+    // TODO(crbug.com/1023250): Simplify this by directly assigning the
+    // properties of the old scroll view to those in the new scroll view.
     [_propertiesStore loadPropertiesToObject:scrollView];
     // Clear the stored values of the properties. This prevents from keeping
     // retaining old property values.
@@ -358,11 +387,16 @@
 - (void)startObservingScrollView:(UIScrollView*)scrollView {
   for (NSString* keyPath in [[self class] scrollViewObserverKeyPaths])
     [scrollView addObserver:self forKeyPath:keyPath options:0 context:nil];
+  self.observingScrollView = YES;
 }
 
 - (void)stopObservingScrollView:(UIScrollView*)scrollView {
+  if (!self.observingScrollView) {
+    return;
+  }
   for (NSString* keyPath in [[self class] scrollViewObserverKeyPaths])
     [scrollView removeObserver:self forKeyPath:keyPath];
+  self.observingScrollView = NO;
 }
 
 - (void)observeValueForKeyPath:(NSString*)keyPath

ios/web/web_state/ui/crw_web_view_scroll_view_proxy_unittest.mm

@@ -27,6 +27,18 @@
 
 @end
 
+@interface UIScrollView (TestingCategory)
+- (int)crw_categoryMethod;
+@end
+
+@implementation UIScrollView (TestingCategory)
+
+- (int)crw_categoryMethod {
+  return 1;
+}
+
+@end
+
 namespace {
 
 class CRWWebViewScrollViewProxyTest : public PlatformTest {
@@ -157,8 +169,8 @@ TEST_F(CRWWebViewScrollViewProxyTest, ScrollViewPresent) {
   EXPECT_TRUE([web_view_scroll_view_proxy_ clipsToBounds]);
 }
 
-// Tests that CRWWebViewScrollViewProxy returns the correct property values when
-// there is no underlying UIScrollView.
+// Tests that CRWWebViewScrollViewProxy returns the default values of
+// UIScrollView's properties when there is no underlying UIScrollView.
 TEST_F(CRWWebViewScrollViewProxyTest, ScrollViewAbsent) {
   [web_view_scroll_view_proxy_ setScrollView:nil];
 
@@ -175,11 +187,11 @@ TEST_F(CRWWebViewScrollViewProxyTest, ScrollViewAbsent) {
   EXPECT_FALSE([web_view_scroll_view_proxy_ isDecelerating]);
   EXPECT_FALSE([web_view_scroll_view_proxy_ isDragging]);
   EXPECT_FALSE([web_view_scroll_view_proxy_ isTracking]);
-  EXPECT_FALSE([web_view_scroll_view_proxy_ scrollsToTop]);
+  EXPECT_TRUE([web_view_scroll_view_proxy_ scrollsToTop]);
   EXPECT_EQ((NSUInteger)0, [web_view_scroll_view_proxy_ subviews].count);
   EXPECT_EQ(UIScrollViewContentInsetAdjustmentAutomatic,
             [web_view_scroll_view_proxy_ contentInsetAdjustmentBehavior]);
-  EXPECT_FALSE([web_view_scroll_view_proxy_ clipsToBounds]);
+  EXPECT_TRUE([web_view_scroll_view_proxy_ clipsToBounds]);
 
   // Make sure setting the properties is fine too.
   // Arbitrary point.
@@ -340,15 +352,15 @@ TEST_F(CRWWebViewScrollViewProxyTest, AsUIScrollViewWithUnderlyingScrollView) {
 }
 
 // Verifies that method calls to -asUIScrollView are no-op if the underlying
-// scroll view is nil and the method is not implemented in
+// scroll view is not set and the method is not implemented in
 // CRWWebViewScrollViewProxy.
 TEST_F(CRWWebViewScrollViewProxyTest,
        AsUIScrollViewWithoutUnderlyingScrollView) {
   [web_view_scroll_view_proxy_ setScrollView:nil];
 
-  // Any methods should return nil when the underlying scroll view is nil.
-  EXPECT_EQ(nil,
-            [[web_view_scroll_view_proxy_ asUIScrollView] viewPrintFormatter]);
+  // Any methods should return nil when the underlying scroll view is not set.
+  EXPECT_EQ(nil, [[web_view_scroll_view_proxy_ asUIScrollView]
+                     restorationIdentifier]);
 
   // It is expected that nothing happens. Just verifies that it doesn't crash.
   CGRect rect = CGRectMake(0, 0, 1, 1);
@@ -575,8 +587,8 @@ TEST_F(CRWWebViewScrollViewProxyTest, RemoveKVObserver) {
 }
 
 // Verifies that properties registered to |propertiesStore| are preserved if:
-//   - the setter is called when the underlying scroll view is nil
-//   - the getter is called after the underlying scroll view is still nil
+//   - the setter is called when the underlying scroll view is not set
+//   - the getter is called after the underlying scroll view is still not set
 TEST_F(CRWWebViewScrollViewProxyTest,
        PreservePropertiesWhileUnderlyingScrollViewIsNil) {
   base::test::ScopedFeatureList scoped_feature_list;
@@ -603,8 +615,8 @@ TEST_F(CRWWebViewScrollViewProxyTest,
 }
 
 // Verifies that properties registered to |propertiesStore| are preserved if:
-//   - the setter is called when the underlying scroll view is nil
-//   - the getter is called after the underlying scroll view is assigned
+//   - the setter is called when the underlying scroll view is not set
+//   - the getter is called after the underlying scroll view is set
 TEST_F(CRWWebViewScrollViewProxyTest,
        PreservePropertiesWhenUnderlyingScrollViewIsNewlyAssigned) {
   base::test::ScopedFeatureList scoped_feature_list;
@@ -636,7 +648,7 @@ TEST_F(CRWWebViewScrollViewProxyTest,
 }
 
 // Verifies that properties registered to |propertiesStore| are preserved if:
-//   - the setter is called when the underlying scroll view is non-nil
+//   - the setter is called when the underlying scroll view is set
 //   - the getter is called after the underlying scroll view is reassigned
 TEST_F(CRWWebViewScrollViewProxyTest,
        PreservePropertiesWhenUnderlyingScrollViewIsReassigned) {
@@ -669,4 +681,39 @@ TEST_F(CRWWebViewScrollViewProxyTest,
   [web_view_scroll_view_proxy_ setScrollView:nil];
 }
 
+// Verifies that the proxy uses the real implementation of a method defined in a
+// category of UIScrollView while the underlying scroll view is not set.
+TEST_F(CRWWebViewScrollViewProxyTest,
+       UIScrollViewCategoryWithoutUnderlyingScrollView) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      web::features::kPreserveScrollViewProperties);
+
+  // Recreate CRWWebViewScrollViewProxy with the updated feature flags.
+  web_view_scroll_view_proxy_ = [[CRWWebViewScrollViewProxy alloc] init];
+
+  [web_view_scroll_view_proxy_ setScrollView:nil];
+
+  EXPECT_EQ(1,
+            [[web_view_scroll_view_proxy_ asUIScrollView] crw_categoryMethod]);
+}
+
+// Verifies that the proxy uses the real implementation of a method defined in a
+// category of UIScrollView while the underlying scroll view is set.
+TEST_F(CRWWebViewScrollViewProxyTest,
+       UIScrollViewCategoryWithUnderlyingScrollView) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      web::features::kPreserveScrollViewProperties);
+
+  // Recreate CRWWebViewScrollViewProxy with the updated feature flags.
+  web_view_scroll_view_proxy_ = [[CRWWebViewScrollViewProxy alloc] init];
+
+  UIScrollView* underlying_scroll_view = [[UIScrollView alloc] init];
+  [web_view_scroll_view_proxy_ setScrollView:underlying_scroll_view];
+
+  EXPECT_EQ(1,
+            [[web_view_scroll_view_proxy_ asUIScrollView] crw_categoryMethod]);
+}
+
 }  // namespace