Add LargeIconCache and LargeIconServiceFactory for iOS.

ios/chrome/browser/favicon/large_icon_cache.h

+// Copyright 2015 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_FAVICON_LARGE_ICON_CACHE_H_
+#define IOS_CHROME_BROWSER_FAVICON_LARGE_ICON_CACHE_H_
+
+#include "base/containers/mru_cache.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "components/keyed_service/core/keyed_service.h"
+
+namespace ios {
+class ChromeBrowserState;
+}
+
+namespace favicon_base {
+struct LargeIconResult;
+};
+
+class GURL;
+
+// Provides a cache of most recently used LargeIconResult.

[sdefresne, 2015/10/29 16:45:25]

Looking at the implementation this does not look like a cache. For a cache, I
would expect the following to be true:

GURL some_url;
favicon_base::LargeIconResult some_result;
LargeIconCache* large_icon_cache = ...;
large_icon_cache->SetCachedResult(some_url, some_result);
DCHECK_EQ(large_icon_cache->GetCachedResult(some_url),
          large_icon_cache->GetCachedResult(some_url));

However this will be false because one of this two calls will return a null
value.

Did you want to have CacheEntry::result be a scoped_refptr<> instead of a
scoped_ptr<>?

[justincohen, 2015/10/29 20:32:16]

Get now calls -CloneLargeIconResult, so it's more of a cache.

+//
+// Example usage:
+//   LargeIconCache* large_icon_cache =
+//       LargeIconCacheFactory::GetForBrowserState(browser_state);

[sdefresne, 2015/10/29 16:45:24]

nit: s/LargeIconCacheFactory/IOSChromeLargeIconServiceFactory/

[justincohen, 2015/10/29 20:32:17]

Done.

+//   large_icon_cache->GetCachedResult(...);
+//
+class LargeIconCache : public KeyedService {

[sdefresne, 2015/10/29 16:45:25]

Since the object only temporary hold the result (as it return scoped_ptr<> to
the data and lose its own reference) I think this needs to be renamed. Maybe
LargeIconBag (I'm terrible at naming)?

[justincohen, 2015/10/29 20:32:17]

Ignored, since it's not temp now.

+ public:
+  explicit LargeIconCache(ios::ChromeBrowserState* browser_state);

[sdefresne, 2015/10/29 16:45:25]

Change to the following as browser_state is never used:

LargeIconCache();

[justincohen, 2015/10/29 20:32:17]

Done.

+  ~LargeIconCache() override;
+
+  // |LargeIconService| does everything on callbacks, and iOS needs to load the
+  // icons immediately on page load. This caches the LargeIconResult so we can
+  // immediately load.
+  void SetCachedResult(GURL url, const favicon_base::LargeIconResult&);

[sdefresne, 2015/10/29 16:45:25]

s/GURL/const GURL&/

[justincohen, 2015/10/29 20:32:17]

Done.

+
+  // Get a cached LargeIconResult.
+  scoped_ptr<favicon_base::LargeIconResult> GetCachedResult(GURL url);

[sdefresne, 2015/10/29 16:45:25]

s/GURL/const GURL&/

[justincohen, 2015/10/29 20:32:17]

Done.

+
+ private:
+  // Cache of LargeIconResult.
+  struct CacheEntry {

[sdefresne, 2015/10/29 16:45:24]

I would forward-declare this and only provide the declaration and implementation
in the large_icon_cache.mm as this is an implementation detail and the
LargeIconCache class declaration only uses pointers to this type (i.e. it can be
forward-declared). The only drawback is that it cannot be scoped into the
LargeIconCache scope.

[justincohen, 2015/10/29 20:32:17]

I'd prefer to not do this.  Is it OK to skip?

+    CacheEntry();
+    ~CacheEntry();
+
+    scoped_ptr<favicon_base::LargeIconResult> result;
+  };
+
+  ios::ChromeBrowserState* browser_state_;

[sdefresne, 2015/10/29 16:45:24]

This is not used, remove.

[justincohen, 2015/10/29 20:32:17]

Done.

+  base::OwningMRUCache<GURL, CacheEntry*> cache_;
+
+  DISALLOW_COPY_AND_ASSIGN(LargeIconCache);
+};
+
+#endif  // IOS_CHROME_BROWSER_FAVICON_LARGE_ICON_CACHE_H_