Fix the snapshot_cache unittests.

This fix is necessary to enable by default the iPad tab switcher.
It is a partial reland of https://codereview.chromium.org/1725533004

This CL:
-Always register/deregister for notifications.
-Centralizes the logic to use a LRU cache.
-Centralizes the logic to use an in-memory cache.
-Replaces msarda with jif in OWNERS.
-Triggers memory warning instead of calling |handleLowMemory| from tests.

BUG=None

Review-Url: https://codereview.chromium.org/2394253002
Cr-Commit-Position: refs/heads/master@{#427302}

ios/chrome/browser/BUILD.gn

@@ -403,6 +403,7 @@ source_set("browser") {
     "snapshots/lru_cache.mm",
     "snapshots/snapshot_cache.h",
     "snapshots/snapshot_cache.mm",
+    "snapshots/snapshot_cache_internal.h",
     "snapshots/snapshot_manager.h",
     "snapshots/snapshot_manager.mm",
     "snapshots/snapshot_overlay.h",

ios/chrome/browser/snapshots/OWNERS

+jif@chromium.org
 justincohen@chromium.org
-msarda@chromium.org

ios/chrome/browser/snapshots/snapshot_cache.mm

@@ -18,13 +18,12 @@
 #include "base/task_runner_util.h"
 #include "base/threading/thread_restrictions.h"
 #include "ios/chrome/browser/experimental_flags.h"
+#import "ios/chrome/browser/snapshots/snapshot_cache_internal.h"
 #include "ios/chrome/browser/ui/ui_util.h"
 #import "ios/chrome/browser/ui/uikit_ui_util.h"
 #include "ios/web/public/web_thread.h"
 
 @interface SnapshotCache ()
-+ (base::FilePath)imagePathForSessionID:(NSString*)sessionID;
-+ (base::FilePath)greyImagePathForSessionID:(NSString*)sessionID;
 // Returns the directory where the thumbnails are saved.
 + (base::FilePath)cacheDirectory;
 // Returns the directory where the thumbnails were stored in M28 and earlier.
@@ -144,52 +143,45 @@ void ConvertAndSaveGreyImage(
     DCHECK_CURRENTLY_ON(web::WebThread::UI);
     propertyReleaser_SnapshotCache_.Init(self, [SnapshotCache class]);
 
-    // Always use the LRUCache when the tab switcher is enabled.
-    if (experimental_flags::IsTabSwitcherEnabled() ||
-        experimental_flags::IsLRUSnapshotCacheEnabled()) {
+    if ([self usesLRUCache]) {
       lruCache_.reset(
           [[LRUCache alloc] initWithCacheSize:kLRUCacheMaxCapacity]);
     } else {
       imageDictionary_.reset(
           [[NSMutableDictionary alloc] initWithCapacity:kCacheInitialCapacity]);
     }
-
-    if (!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled()) {
-      [[NSNotificationCenter defaultCenter]
-          addObserver:self
-             selector:@selector(handleLowMemory)
-                 name:UIApplicationDidReceiveMemoryWarningNotification
-               object:nil];
-      [[NSNotificationCenter defaultCenter]
-          addObserver:self
-             selector:@selector(handleEnterBackground)
-                 name:UIApplicationDidEnterBackgroundNotification
-               object:nil];
-      [[NSNotificationCenter defaultCenter]
-          addObserver:self
-             selector:@selector(handleBecomeActive)
-                 name:UIApplicationDidBecomeActiveNotification
-               object:nil];
-    }
+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(handleLowMemory)
+               name:UIApplicationDidReceiveMemoryWarningNotification
+             object:nil];
+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(handleEnterBackground)
+               name:UIApplicationDidEnterBackgroundNotification
+             object:nil];
+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(handleBecomeActive)
+               name:UIApplicationDidBecomeActiveNotification
+             object:nil];
   }
   return self;
 }
 
 - (void)dealloc {
-  if (!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled()) {
-    [[NSNotificationCenter defaultCenter]
-        removeObserver:self
-                  name:UIApplicationDidReceiveMemoryWarningNotification
-                object:nil];
-    [[NSNotificationCenter defaultCenter]
-        removeObserver:self
-                  name:UIApplicationDidEnterBackgroundNotification
-                object:nil];
-    [[NSNotificationCenter defaultCenter]
-        removeObserver:self
-                  name:UIApplicationDidBecomeActiveNotification
-                object:nil];
-  }
+  [[NSNotificationCenter defaultCenter]
+      removeObserver:self
+                name:UIApplicationDidReceiveMemoryWarningNotification
+              object:nil];
+  [[NSNotificationCenter defaultCenter]
+      removeObserver:self
+                name:UIApplicationDidEnterBackgroundNotification
+              object:nil];
+  [[NSNotificationCenter defaultCenter]
+      removeObserver:self
+                name:UIApplicationDidBecomeActiveNotification
+              object:nil];
   [super dealloc];
 }
 
@@ -210,9 +202,7 @@ void ConvertAndSaveGreyImage(
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(sessionID);
 
-  // Cache on iPad is enabled only when the tab switcher is enabled.
-  if ((IsIPadIdiom() && !experimental_flags::IsTabSwitcherEnabled()) &&
-      !callback)
+  if (![self inMemoryCacheIsEnabled] && !callback)
     return;
 
   UIImage* img = nil;
@@ -236,9 +226,7 @@ void ConvertAndSaveGreyImage(
             [SnapshotCache imagePathForSessionID:sessionID]) retain]);
       }),
       base::BindBlock(^(base::scoped_nsobject<UIImage> image) {
-        // Cache on iPad is enabled only when the tab switcher is enabled.
-        if ((!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled()) &&
-            image) {
+        if ([self inMemoryCacheIsEnabled] && image) {
           if (lruCache_)
             [lruCache_ setObject:image forKey:sessionID];
           else
@@ -254,8 +242,7 @@ void ConvertAndSaveGreyImage(
   if (!img || !sessionID)
     return;
 
-  // Cache on iPad is enabled only when the tab switcher is enabled.
-  if (!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled()) {
+  if ([self inMemoryCacheIsEnabled]) {
     if (lruCache_)
       [lruCache_ setObject:img forKey:sessionID];
     else
@@ -386,8 +373,18 @@ void ConvertAndSaveGreyImage(
   }
 }
 
+- (BOOL)inMemoryCacheIsEnabled {
+  // In-memory cache on iPad is enabled only when the tab switcher is enabled.
+  return !IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled();
+}
+
+- (BOOL)usesLRUCache {
+  // Always use the LRUCache when the tab switcher is enabled.
+  return experimental_flags::IsTabSwitcherEnabled() ||
+         experimental_flags::IsLRUSnapshotCacheEnabled();
+}
+
 - (void)handleLowMemory {
-  DCHECK(!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled());
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
   base::scoped_nsobject<NSMutableDictionary> dictionary(
       [[NSMutableDictionary alloc] initWithCapacity:2]);
@@ -411,14 +408,12 @@ void ConvertAndSaveGreyImage(
 }
 
 - (void)handleEnterBackground {
-  DCHECK(!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled());
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
   [imageDictionary_ removeAllObjects];
   [lruCache_ removeAllObjects];
 }
 
 - (void)handleBecomeActive {
-  DCHECK(!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled());
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
   for (NSString* sessionID in pinnedIDs_)
     [self retrieveImageForSessionID:sessionID callback:nil];
@@ -569,11 +564,12 @@ void ConvertAndSaveGreyImage(
 @implementation SnapshotCache (TestingAdditions)
 
 - (BOOL)hasImageInMemory:(NSString*)sessionID {
-  if (experimental_flags::IsLRUSnapshotCacheEnabled())
+  if ([self usesLRUCache])
     return [lruCache_ objectForKey:sessionID] != nil;
   else
     return [imageDictionary_ objectForKey:sessionID] != nil;
 }
+
 - (BOOL)hasGreyImageInMemory:(NSString*)sessionID {
   return [greyImageDictionary_ objectForKey:sessionID] != nil;
 }

ios/chrome/browser/snapshots/snapshot_cache_internal.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_CHROME_BROWSER_SNAPSHOTS_SNAPSHOT_CACHE_INTERNAL_H_
+#define IOS_CHROME_BROWSER_SNAPSHOTS_SNAPSHOT_CACHE_INTERNAL_H_
+
+#import "ios/chrome/browser/snapshots/snapshot_cache_internal.h"
+
+namespace base {
+class FilePath;
+}
+
+@class NSString;
+
+@interface SnapshotCache (Internal)
+// Returns filepath to the color snapshot of |sessionID|.
++ (base::FilePath)imagePathForSessionID:(NSString*)sessionID;
+// Returns filepath to the greyscale snapshot of |sessionID|.
++ (base::FilePath)greyImagePathForSessionID:(NSString*)sessionID;
+// Returns whether the snapshots are cached in a LRU cache.
+- (BOOL)usesLRUCache;
+// Returns whether the in-memory cache (as opposed to the on-disk cache) is
+// enabled.
+- (BOOL)inMemoryCacheIsEnabled;
+@end
+
+#endif  // IOS_CHROME_BROWSER_SNAPSHOTS_SNAPSHOT_CACHE_INTERNAL_H_
\ No newline at end of file

ios/chrome/browser/snapshots/snapshot_cache_unittest.mm

@@ -9,15 +9,13 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/format_macros.h"
-#include "base/ios/ios_util.h"
 #include "base/location.h"
 #include "base/mac/bind_objc_block.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
 #include "base/run_loop.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/time/time.h"
-#include "ios/chrome/browser/experimental_flags.h"
-#include "ios/chrome/browser/ui/ui_util.h"
+#import "ios/chrome/browser/snapshots/snapshot_cache_internal.h"
 #include "ios/web/public/test/test_web_thread_bundle.h"
 #include "ios/web/public/web_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -27,13 +25,6 @@
 static const NSUInteger kSessionCount = 10;
 static const NSUInteger kSnapshotPixelSize = 8;
 
-// Promote some implementation methods to public.
-@interface SnapshotCache (Testing)
-+ (base::FilePath)imagePathForSessionID:(NSString*)sessionID;
-+ (base::FilePath)greyImagePathForSessionID:(NSString*)sessionID;
-- (void)handleLowMemory;
-@end
-
 namespace {
 
 class SnapshotCacheTest : public PlatformTest {
@@ -204,6 +195,13 @@ class SnapshotCacheTest : public PlatformTest {
     return reinterpret_cast<const char*>(CFDataGetBytePtr(data));
   }
 
+  void TriggerMemoryWarning() {
+    // _performMemoryWarning is a private API and must not be compiled into
+    // official builds.
+    [[UIApplication sharedApplication]
+        performSelector:@selector(_performMemoryWarning)];
+  }
+
   web::TestWebThreadBundle thread_bundle_;
   base::scoped_nsobject<SnapshotCache> snapshotCache_;
   base::scoped_nsobject<NSMutableArray> testSessions_;
@@ -214,16 +212,13 @@ class SnapshotCacheTest : public PlatformTest {
 // As the snapshots are kept in memory, the same pointer can be retrieved.
 // This test also checks that images are correctly removed from the disk.
 TEST_F(SnapshotCacheTest, Cache) {
-  // Don't run on tablets because color snapshots are not cached so this test
-  // can't compare the UIImage pointers directly.
-  if (IsIPadIdiom() && !experimental_flags::IsTabSwitcherEnabled()) {
-    return;
-  }
-
   SnapshotCache* cache = GetSnapshotCache();
 
+  if (![cache inMemoryCacheIsEnabled])
+    return;
+
   NSUInteger expectedCacheSize = kSessionCount;
-  if (experimental_flags::IsLRUSnapshotCacheEnabled())
+  if ([cache usesLRUCache])
     expectedCacheSize = MIN(kSessionCount, [cache lruCacheMaxSize]);
 
   // Put all images in the cache.
@@ -347,17 +342,8 @@ TEST_F(SnapshotCacheTest, Purge) {
 }
 
 // Loads the color images into the cache, and pins two of them.  Ensures that
-// only the two pinned IDs remain in memory after a call to -handleLowMemory.
-TEST_F(SnapshotCacheTest, HandleLowMemory) {
-// TODO(crbug.com/455209): This test failed on iPad iOS8 device, but may no
-// longer be failing on iOS 9 and up. Should test by re-enabling and monitor.
-#if !TARGET_IPHONE_SIMULATOR
-  if (IsIPadIdiom()) {
-    LOG(WARNING) << "Test disabled on iPad device.";
-    return;
-  }
-#endif
-
+// only the two pinned IDs remain in memory after a memory warning.
+TEST_F(SnapshotCacheTest, HandleMemoryWarning) {
   LoadAllColorImagesIntoCache(true);
 
   SnapshotCache* cache = GetSnapshotCache();
@@ -369,15 +355,15 @@ TEST_F(SnapshotCacheTest, HandleLowMemory) {
   [set addObject:secondPinnedID];
   cache.pinnedIDs = set;
 
-  if (!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled())
-    [cache handleLowMemory];
-
-  BOOL expectedValue = YES;
-  if (IsIPadIdiom() && !experimental_flags::IsTabSwitcherEnabled())
-    expectedValue = NO;
+  TriggerMemoryWarning();
 
-  EXPECT_EQ(expectedValue, [cache hasImageInMemory:firstPinnedID]);
-  EXPECT_EQ(expectedValue, [cache hasImageInMemory:secondPinnedID]);
+  if ([cache inMemoryCacheIsEnabled]) {
+    EXPECT_EQ(YES, [cache hasImageInMemory:firstPinnedID]);
+    EXPECT_EQ(YES, [cache hasImageInMemory:secondPinnedID]);
+  } else {
+    EXPECT_EQ(NO, [cache hasImageInMemory:firstPinnedID]);
+    EXPECT_EQ(NO, [cache hasImageInMemory:secondPinnedID]);
+  }
 
   NSString* notPinnedID = [testSessions_ objectAtIndex:2];
   EXPECT_FALSE([cache hasImageInMemory:notPinnedID]);
@@ -424,8 +410,7 @@ TEST_F(SnapshotCacheTest, CreateGreyCacheFromDisk) {
   // Remove color images from in-memory cache.
   SnapshotCache* cache = GetSnapshotCache();
 
-  if (!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled())
-    [cache handleLowMemory];
+  TriggerMemoryWarning();
 
   // Request the creation of a grey image cache for all images.
   [cache createGreyCache:testSessions_];
@@ -466,8 +451,7 @@ TEST_F(SnapshotCacheTest, MostRecentGreyBlock) {
   LoadColorImagesIntoCache(kNumImages, true);
   // Make sure the color images are only on disk, to ensure the background
   // thread is slow enough to queue up the requests.
-  if (!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled())
-    [cache handleLowMemory];
+  TriggerMemoryWarning();
 
   // Enable the grey image cache.
   [cache createGreyCache:sessionIDs];
@@ -538,8 +522,7 @@ TEST_F(SnapshotCacheTest, SizeAndScalePreservation) {
   NSString* const kSession = @"foo";
   [cache setImage:image withSessionID:kSession];
   FlushRunLoops();  // ensure the file is written to disk.
-  if (!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled())
-    [cache handleLowMemory];
+  TriggerMemoryWarning();
 
   // Retrive the image and have the callback verify the size and scale.
   __block BOOL callbackComplete = NO;
@@ -575,8 +558,7 @@ TEST_F(SnapshotCacheTest, DeleteRetinaImages) {
   NSString* const kSession = @"foo";
   [cache setImage:image withSessionID:kSession];
   FlushRunLoops();  // ensure the file is written to disk.
-  if (!IsIPadIdiom() || experimental_flags::IsTabSwitcherEnabled())
-    [cache handleLowMemory];
+  TriggerMemoryWarning();
 
   // Verify the file was writted with @2x in the file name.
   base::FilePath retinaFile = [SnapshotCache imagePathForSessionID:kSession];