Rewrite implementation of ClipboardRecentContent in Objective C.

components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm

Index: components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm
diff --git a/components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm b/components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm
index 670a92354c071e8c1d0ace28573b338e4a0cebb6..917d0555678165fbff6de1596e8f79cfa9e969ac 100644
--- a/components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm
+++ b/components/open_from_clipboard/clipboard_recent_content_ios_unittest.mm
@@ -9,6 +9,7 @@
 
 #include <memory>
 
+#import "components/open_from_clipboard/clipboard_recent_content_ios_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
@@ -45,20 +46,48 @@ const char kAppSpecificScheme[] = "test";
 NSTimeInterval kSevenHours = 60 * 60 * 7;
 }  // namespace
 
+@interface ClipboardRecentContentIOSImplWithFakeUptime
+    : ClipboardRecentContentIOSImpl
+@property(nonatomic) NSTimeInterval fakeUptime;
+
+- (instancetype)initWithDelegate:
+                    (id<ClipboardRecentContentMetricsDelegate>)delegate
+               authorizedSchemes:(NSArray*)authorizedSchemes
+                    userDefaults:(NSUserDefaults*)groupUserDefaults
+                          uptime:(NSTimeInterval)uptime;
+
+@end
+
+@implementation ClipboardRecentContentIOSImplWithFakeUptime
+
+@synthesize fakeUptime = _fakeUptime;
+
+- (instancetype)initWithDelegate:
+                    (id<ClipboardRecentContentMetricsDelegate>)delegate
+               authorizedSchemes:(NSArray*)authorizedSchemes
+                    userDefaults:(NSUserDefaults*)groupUserDefaults
+                          uptime:(NSTimeInterval)uptime {
+  self = [super initWithDelegate:delegate
+               authorizedSchemes:authorizedSchemes
+                    userDefaults:groupUserDefaults];
+  if (self) {
+    _fakeUptime = uptime;
+  }
+  return self;
+}
+
+- (NSTimeInterval)uptime {

[sdefresne, 2017/04/03 09:27:17]

I would change this to a public property instead (see comment line 121).

+  return self.fakeUptime;
+}
+
+@end
+
 class ClipboardRecentContentIOSWithFakeUptime
     : public ClipboardRecentContentIOS {
  public:
   ClipboardRecentContentIOSWithFakeUptime(const std::string& application_scheme,
                                           NSUserDefaults* group_user_defaults)
       : ClipboardRecentContentIOS(application_scheme, group_user_defaults) {}
-  // Sets the uptime.
-  void SetUptime(base::TimeDelta uptime) { uptime_ = uptime; }
-
- protected:
-  base::TimeDelta Uptime() const override { return uptime_; }
-
- private:
-  base::TimeDelta uptime_;
 };
 
 class ClipboardRecentContentIOSTest : public ::testing::Test {
@@ -78,16 +107,18 @@ class ClipboardRecentContentIOSTest : public ::testing::Test {
                                    base::TimeDelta time_delta) {
     clipboard_content_.reset(new ClipboardRecentContentIOSWithFakeUptime(
         application_scheme, [NSUserDefaults standardUserDefaults]));
-    clipboard_content_->SetUptime(time_delta);
-  }
 
-  void SetStoredPasteboardChangeDate(NSDate* changeDate) {
-    clipboard_content_->last_pasteboard_change_date_.reset([changeDate copy]);
-    clipboard_content_->SaveToUserDefaults();
+    clipboard_content_->implementation_.reset(

[sdefresne, 2017/04/03 09:27:17]

This is exactly how you should not write test. Instead provide a constructor
that takes a ClipboardRecentContentIOSImpl* and use that constructor when
creating your ClipboardRecentContentIOS.

[sdefresne, 2017/04/03 09:37:32]

Sorry, this comment tone is too negative. I should have remove the first
sentence. Please read this as:

I think injecting the ClipboardRecentContentIOSImpl via the constructor would
make the test easier to write as it would not require accessing private member
fields of ClipboardRecentContentIOS.

[lody, 2017/04/04 13:42:19]

Done.

+        [[ClipboardRecentContentIOSImplWithFakeUptime alloc]
+             initWithDelegate:nil
+            authorizedSchemes:clipboard_content_->GetAuthorizedSchemeList(

[sdefresne, 2017/04/03 09:27:17]

Just inject a know list/set of schemes here, no need to access this private
method.

[lody, 2017/04/04 13:42:19]

Done.

+                                  application_scheme)
+                 userDefaults:[NSUserDefaults standardUserDefaults]
+                       uptime:time_delta.InSecondsF()]);
   }
 
-  void SetStoredPasteboardChangeCount(NSInteger newChangeCount) {
-    clipboard_content_->last_pasteboard_change_count_ = newChangeCount;
+  void SetStoredPasteboardChangeDate(NSDate* change_date) {
+    clipboard_content_->SetLastPasteboardChangeDate([change_date copy]);

[sdefresne, 2017/04/03 09:27:17]

Can't you simulate this by instead manipulating the fake uptime? \

[lody, 2017/04/04 13:42:19]

I don't think so? There's two things, one is the uptime (if the uptime is
shorter than last change date md5 is used to check how many changes have occured
rather than change count) and the changedate (if the changedate is more than 3
hours then nothing is returned). I can't see how to use only uptime to simulate
this.

     clipboard_content_->SaveToUserDefaults();
   }
 
@@ -164,7 +195,7 @@ TEST_F(ClipboardRecentContentIOSTest, SupressedPasteboard) {
   // Check that the pasteboard content is still suppressed.
   EXPECT_FALSE(clipboard_content_->GetRecentURLFromClipboard(&gurl));
 
-  // Check that the even if the device is restarted, pasteboard content is
+  // Check that even if the device is restarted, pasteboard content is
   // still suppressed.
   SimulateDeviceRestart();
   EXPECT_FALSE(clipboard_content_->GetRecentURLFromClipboard(&gurl));