Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1413903008:
Add LargeIconCache and LargeIconServiceFactory for iOS. (Closed)
|
Created:
5 years, 1 month ago by justincohen Modified:
5 years, 1 month ago CC:
browser-components-watch_chromium.org, chromium-reviews, sdefresne+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd LargeIconCache and LargeIconServiceFactory for iOS.
Also adds a secondary cache that holds on to a reference to the favicon
RefCountedMemory because the LargeIconService returns icons too slowly
iOS new tab page animations.
BUG=543738
Committed: https://crrev.com/af62fafc5f695c806ae5696d66f78643e082dc37
Cr-Commit-Position: refs/heads/master@{#357203}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address missing files #Patch Set 3 : Using wrong thread pool #
Total comments: 34
Patch Set 4 : Address Sylvain comments. #
Total comments: 16
Patch Set 5 : Better skia utils locations #Patch Set 6 : broken sort #
Total comments: 14
Patch Set 7 : Address skia comments #Patch Set 8 : Address Sylvain comments #Patch Set 9 : Add a test #
Total comments: 1
Patch Set 10 : Fix nits for unit test #
Total comments: 6
Messages
Total messages: 23 (6 generated)
justincohen@chromium.org changed reviewers: + sdefresne@chromium.org
PTAL!
https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:23: // Provides a cache of most recently used LargeIconResult. 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<>? https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:27: // LargeIconCacheFactory::GetForBrowserState(browser_state); nit: s/LargeIconCacheFactory/IOSChromeLargeIconServiceFactory/ https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:30: class LargeIconCache : public KeyedService { 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)? https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:32: explicit LargeIconCache(ios::ChromeBrowserState* browser_state); Change to the following as browser_state is never used: LargeIconCache(); https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:38: void SetCachedResult(GURL url, const favicon_base::LargeIconResult&); s/GURL/const GURL&/ https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:41: scoped_ptr<favicon_base::LargeIconResult> GetCachedResult(GURL url); s/GURL/const GURL&/ https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:45: struct CacheEntry { 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. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:52: ios::ChromeBrowserState* browser_state_; This is not used, remove. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/large_icon_cache.mm (right): https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.mm:27: favicon_base::LargeIconResult* cache_value; scoped_ptr<favicon_base::LargeIconResult> cache_value; https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.mm:49: return iter->second->result.Pass(); You're sinking the reference here (since result is a scoped_ptr<>), so I think you should do the following: if (iter != cache_.end()) { scoped_ptr<favicon_base::LargeIconResult> result( iter->second->result.Pass()); cache_.Erase(iter); return result; } https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.mm:54: return cache_value.Pass(); return scoped_ptr<favicon_base::LargeIconResult>();
Thanks, PTAL! https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:23: // Provides a cache of most recently used LargeIconResult. On 2015/10/29 16:45:25, sdefresne wrote: > 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<>? Get now calls -CloneLargeIconResult, so it's more of a cache. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:27: // LargeIconCacheFactory::GetForBrowserState(browser_state); On 2015/10/29 16:45:24, sdefresne wrote: > nit: s/LargeIconCacheFactory/IOSChromeLargeIconServiceFactory/ Done. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:30: class LargeIconCache : public KeyedService { On 2015/10/29 16:45:25, sdefresne wrote: > 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)? Ignored, since it's not temp now. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:32: explicit LargeIconCache(ios::ChromeBrowserState* browser_state); On 2015/10/29 16:45:25, sdefresne wrote: > Change to the following as browser_state is never used: > > LargeIconCache(); Done. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:38: void SetCachedResult(GURL url, const favicon_base::LargeIconResult&); On 2015/10/29 16:45:25, sdefresne wrote: > s/GURL/const GURL&/ Done. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:41: scoped_ptr<favicon_base::LargeIconResult> GetCachedResult(GURL url); On 2015/10/29 16:45:25, sdefresne wrote: > s/GURL/const GURL&/ Done. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:45: struct CacheEntry { On 2015/10/29 16:45:24, sdefresne wrote: > 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. I'd prefer to not do this. Is it OK to skip? https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.h:52: ios::ChromeBrowserState* browser_state_; On 2015/10/29 16:45:24, sdefresne wrote: > This is not used, remove. Done. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/large_icon_cache.mm (right): https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.mm:27: favicon_base::LargeIconResult* cache_value; On 2015/10/29 16:45:25, sdefresne wrote: > scoped_ptr<favicon_base::LargeIconResult> cache_value; Removed completely https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.mm:49: return iter->second->result.Pass(); On 2015/10/29 16:45:25, sdefresne wrote: > You're sinking the reference here (since result is a scoped_ptr<>), so I think > you should do the following: > > if (iter != cache_.end()) { > scoped_ptr<favicon_base::LargeIconResult> result( > iter->second->result.Pass()); > cache_.Erase(iter); > return result; > } Changed to not Pass what we hold on to. https://codereview.chromium.org/1413903008/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/large_icon_cache.mm:54: return cache_value.Pass(); On 2015/10/29 16:45:25, sdefresne wrote: > return scoped_ptr<favicon_base::LargeIconResult>(); Done.
Could your reword your description to not use GCL acronym (I don't know what it means, and thus the description is really opaque to me)? https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:23: class KeyedService; nit: the convention is to put forward-declaration in the global namespace before the forward-declaration in namespace, so put this line before "namespace base". https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:25: namespace favicon { nit: please sort namespaces (i.e. base > favicon > ios). https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.mm (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.mm:26: IOSChromeLargeIconServiceFactory::GetInstance() { Please put a call to this method in ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:13: namespace ios { nit: ditto, sort namespaces. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:21: class GURL; nit: ditto, please put before namespaces. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:28: // large_icon_cache->GetCachedResult(...); nit: scoped_ptr<favicon_base::LargeIconResult> icon = large_icon_cache->GetCachedResult(...); https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:32: explicit LargeIconCache(); style: do not use "explicit" for constructor without parameters (you should have received a warning at "git cl upload"). https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:38: void SetCachedResult(const GURL url, const favicon_base::LargeIconResult&); const GURL -> const GURL& https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:40: // Get a cached LargeIconResult. s/Get/Returns/ https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:44: // Clone a LargeIconResult. s/Clone/Clones/ https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:49: struct CacheEntry { Please forward-declare: class LargeIconCacheEntry; class LargeIconCache { private: base::OwningMRUCache<GURL, LargeIconCacheEntry*> cache_; }; https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache.mm (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.mm:17: } // anonymous namespace style: } // namespace https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.mm:24: GURL url, GURL -> const GURL& https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache_factory.h (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.h:21: class KeyedService; nit: ditto, move before base namespace. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache_factory.mm (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.mm:22: return base::Singleton<LargeIconCacheFactory>::get(); Please put a call to this method in ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.mm:34: return make_scoped_ptr(new LargeIconCache()); nit: return make_scoped_ptr(new LargeIconCache); https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/ui/u... File ios/chrome/browser/ui/uikit_ui_util.h (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/ui/u... ios/chrome/browser/ui/uikit_ui_util.h:13: #include "ui/gfx/color_analysis.h" Why not #include "third_party/skia/include/core/SkColor.h"?
Description was changed from ========== Add LargeIconCache and LargeIconServiceFactory for iOS. Note: Because the LargeIconService returns icons too slowly for the GLC new tab animation, a secondary cache holds on to a reference to the favicon RefCountedMemory. BUG=543738 ========== to ========== Add LargeIconCache and LargeIconServiceFactory for iOS. Also adds a secondary cache that holds on to a reference to the favicon RefCountedMemory because the LargeIconService returns icons too slowly iOS new tab page animations. BUG=543738 ==========
Done, thanks for the comments. PTAL! https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:23: class KeyedService; On 2015/10/30 09:27:18, sdefresne wrote: > nit: the convention is to put forward-declaration in the global namespace before > the forward-declaration in namespace, so put this line before "namespace base". Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:25: namespace favicon { On 2015/10/30 09:27:18, sdefresne wrote: > nit: please sort namespaces (i.e. base > favicon > ios). Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.mm (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.mm:26: IOSChromeLargeIconServiceFactory::GetInstance() { On 2015/10/30 09:27:18, sdefresne wrote: > Please put a call to this method in > ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm. Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:13: namespace ios { On 2015/10/30 09:27:19, sdefresne wrote: > nit: ditto, sort namespaces. Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:21: class GURL; On 2015/10/30 09:27:19, sdefresne wrote: > nit: ditto, please put before namespaces. Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:28: // large_icon_cache->GetCachedResult(...); On 2015/10/30 09:27:18, sdefresne wrote: > nit: > > scoped_ptr<favicon_base::LargeIconResult> icon = > large_icon_cache->GetCachedResult(...); Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:32: explicit LargeIconCache(); On 2015/10/30 09:27:19, sdefresne wrote: > style: do not use "explicit" for constructor without parameters (you should have > received a warning at "git cl upload"). Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:38: void SetCachedResult(const GURL url, const favicon_base::LargeIconResult&); On 2015/10/30 09:27:19, sdefresne wrote: > const GURL -> const GURL& Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:40: // Get a cached LargeIconResult. On 2015/10/30 09:27:19, sdefresne wrote: > s/Get/Returns/ Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:44: // Clone a LargeIconResult. On 2015/10/30 09:27:19, sdefresne wrote: > s/Clone/Clones/ Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:49: struct CacheEntry { On 2015/10/30 09:27:19, sdefresne wrote: > Please forward-declare: > > class LargeIconCacheEntry; > class LargeIconCache { > private: > base::OwningMRUCache<GURL, LargeIconCacheEntry*> cache_; > }; Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache.mm (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.mm:17: } // anonymous namespace On 2015/10/30 09:27:19, sdefresne wrote: > style: } // namespace Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.mm:24: GURL url, On 2015/10/30 09:27:19, sdefresne wrote: > GURL -> const GURL& Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache_factory.h (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.h:21: class KeyedService; On 2015/10/30 09:27:19, sdefresne wrote: > nit: ditto, move before base namespace. Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache_factory.mm (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.mm:22: return base::Singleton<LargeIconCacheFactory>::get(); On 2015/10/30 09:27:19, sdefresne wrote: > Please put a call to this method in > ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm. Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.mm:34: return make_scoped_ptr(new LargeIconCache()); On 2015/10/30 09:27:19, sdefresne wrote: > nit: return make_scoped_ptr(new LargeIconCache); Done. https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/ui/u... File ios/chrome/browser/ui/uikit_ui_util.h (right): https://codereview.chromium.org/1413903008/diff/40001/ios/chrome/browser/ui/u... ios/chrome/browser/ui/uikit_ui_util.h:13: #include "ui/gfx/color_analysis.h" On 2015/10/30 09:27:19, sdefresne wrote: > Why not #include "third_party/skia/include/core/SkColor.h"? Because it's not allowed during git cl upload.
https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.mm (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. It looks like this file is pure C++, can you rename it to ios/chrome/browser/favicon/ios_+chrome_large_icon_service_factory.cc? https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:14: nit: no blank line and sort, i.e. struct LargeIconCacheEntry; class GURL; https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache.mm (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. It looks like this file is pure C++, can you rename it to ios/chrome/browser/favicon/large_icon_cache.cc? https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.mm:7: #import <Foundation/Foundation.h> Is this required? https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache_factory.h (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.h:2: // Use of this source code is governed by a BSD-style license that can be This file should be named ios/chrome/browser/favicon/ios_chrome_large_icon_cache_factory.h. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.h:26: class LargeIconCacheFactory : public BrowserStateKeyedServiceFactory { Should be called IOSChromeLargeIconCacheFactory. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache_factory.mm (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. Ditto, this looks like a pure C++ file, and should be named ios/chrome/browser/favicon/large_icon_cache_factory.cc. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/ui/u... File ios/chrome/browser/ui/uikit_ui_util.h (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/ui/u... ios/chrome/browser/ui/uikit_ui_util.h:13: #include "ui/gfx/color_analysis.h" I still think we should include #include "third_party/skia/include/core/SkColor.h" and add a DEPS rule in ios/chrome/browser/ui/DEPS for that file. It is included in multiple locations in chrome/browser so I think it is okay (checked with blundell@). https://code.google.com/p/chromium/codesearch#search/&q=%23include%5C%20%22th...
justincohen@chromium.org changed reviewers: + junov@chromium.org
+junov@ for skia changes PTAL! https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.mm (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/30 14:19:37, sdefresne wrote: > It looks like this file is pure C++, can you rename it to > ios/chrome/browser/favicon/ios_+chrome_large_icon_service_factory.cc? Done. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.h:14: On 2015/10/30 14:19:37, sdefresne wrote: > nit: no blank line and sort, i.e. > > struct LargeIconCacheEntry; > class GURL; Done. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache.mm (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/30 14:19:37, sdefresne wrote: > It looks like this file is pure C++, can you rename it to > ios/chrome/browser/favicon/large_icon_cache.cc? Done. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache.mm:7: #import <Foundation/Foundation.h> On 2015/10/30 14:19:37, sdefresne wrote: > Is this required? Done. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache_factory.h (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.h:2: // Use of this source code is governed by a BSD-style license that can be On 2015/10/30 14:19:37, sdefresne wrote: > This file should be named > ios/chrome/browser/favicon/ios_chrome_large_icon_cache_factory.h. Done. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.h:26: class LargeIconCacheFactory : public BrowserStateKeyedServiceFactory { On 2015/10/30 14:19:37, sdefresne wrote: > Should be called IOSChromeLargeIconCacheFactory. Done. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/large_icon_cache_factory.mm (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/favi... ios/chrome/browser/favicon/large_icon_cache_factory.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/30 14:19:37, sdefresne wrote: > Ditto, this looks like a pure C++ file, and should be named > ios/chrome/browser/favicon/large_icon_cache_factory.cc. Done. https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/ui/u... File ios/chrome/browser/ui/uikit_ui_util.h (right): https://codereview.chromium.org/1413903008/diff/60001/ios/chrome/browser/ui/u... ios/chrome/browser/ui/uikit_ui_util.h:13: #include "ui/gfx/color_analysis.h" On 2015/10/30 14:19:37, sdefresne wrote: > I still think we should include #include > "third_party/skia/include/core/SkColor.h" and add a DEPS rule in > ios/chrome/browser/ui/DEPS for that file. It is included in multiple locations > in chrome/browser so I think it is okay (checked with blundell@). > > https://code.google.com/p/chromium/codesearch#search/&q=%23include%5C%20%22th... mioved to skia-utils-ios
lgtm with nits https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h (right): https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:9: #include "base/memory/ref_counted.h" nit: Unused :-) https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:18: class TaskRunner; nit: Unused :-) https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/large_icon_cache.cc (right): https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.cc:15: // Cache of LargeIconResult. nit: what is this comment doing here? Remove. https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.cc:40: base::OwningMRUCache<GURL, LargeIconCacheEntry*>::iterator iter = nit: you could have used "auto iter = cache_.Get(url);" https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.h:18: }; nit: no semi-colon after closing a namespace.
https://codereview.chromium.org/1413903008/diff/100001/skia/ext/skia_utils_ios.h File skia/ext/skia_utils_ios.h (right): https://codereview.chromium.org/1413903008/diff/100001/skia/ext/skia_utils_io... skia/ext/skia_utils_ios.h:42: SK_API UIColor* ColorFromSkColor(SkColor color, CGFloat alpha); Name: UIColorFromSkColor If this method converts from skia type to platform type, shouldn't alpha be an SkScalar? Finally, What do you need this for? I don't see this being called anywhere https://codereview.chromium.org/1413903008/diff/100001/skia/ext/skia_utils_io... File skia/ext/skia_utils_ios.mm (right): https://codereview.chromium.org/1413903008/diff/100001/skia/ext/skia_utils_io... skia/ext/skia_utils_ios.mm:130: alpha:alpha]; SkColor stores an alpha value, which this method completely ignores. Are you sure this is the right thing to do here?
Done, PTAL! https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h (right): https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:9: #include "base/memory/ref_counted.h" On 2015/10/30 17:20:14, sdefresne wrote: > nit: Unused :-) Done. https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h:18: class TaskRunner; On 2015/10/30 17:20:14, sdefresne wrote: > nit: Unused :-) Done. https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/large_icon_cache.cc (right): https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.cc:15: // Cache of LargeIconResult. On 2015/10/30 17:20:14, sdefresne wrote: > nit: what is this comment doing here? Remove. Done. https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.cc:40: base::OwningMRUCache<GURL, LargeIconCacheEntry*>::iterator iter = On 2015/10/30 17:20:14, sdefresne wrote: > nit: you could have used "auto iter = cache_.Get(url);" Done. https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/100001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.h:18: }; On 2015/10/30 17:20:14, sdefresne wrote: > nit: no semi-colon after closing a namespace. Done. https://codereview.chromium.org/1413903008/diff/100001/skia/ext/skia_utils_ios.h File skia/ext/skia_utils_ios.h (right): https://codereview.chromium.org/1413903008/diff/100001/skia/ext/skia_utils_io... skia/ext/skia_utils_ios.h:42: SK_API UIColor* ColorFromSkColor(SkColor color, CGFloat alpha); On 2015/10/30 18:18:13, Justin Novosad wrote: > Name: UIColorFromSkColor > If this method converts from skia type to platform type, shouldn't alpha be an > SkScalar? > Finally, What do you need this for? I don't see this being called anywhere Done + this is being used downstream for iOS. https://codereview.chromium.org/1413903008/diff/100001/skia/ext/skia_utils_io... File skia/ext/skia_utils_ios.mm (right): https://codereview.chromium.org/1413903008/diff/100001/skia/ext/skia_utils_io... skia/ext/skia_utils_ios.mm:130: alpha:alpha]; On 2015/10/30 18:18:13, Justin Novosad wrote: > SkColor stores an alpha value, which this method completely ignores. Are you > sure this is the right thing to do here? Probably not -- changed to use |color|'s alpha instead.
Added a test to skia_utils_ios_unittest
lgtm with nit https://codereview.chromium.org/1413903008/diff/150001/skia/ext/skia_utils_io... File skia/ext/skia_utils_ios_unittest.mm (right): https://codereview.chromium.org/1413903008/diff/150001/skia/ext/skia_utils_io... skia/ext/skia_utils_ios_unittest.mm:757: EXPECT_EQ(50, (int)(alpha * 255)); To be totally safe with respect to machine precision, you should probably round instead of truncate the values, and avoid c-style casts. Something like: static_cast<int>(alpha * 255 + 0.5f);
The CQ bit was checked by justincohen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, junov@chromium.org Link to the patchset: https://codereview.chromium.org/1413903008/#ps170001 (title: "Fix nits for unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413903008/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413903008/170001
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/af62fafc5f695c806ae5696d66f78643e082dc37 Cr-Commit-Position: refs/heads/master@{#357203}
Message was sent while issue was closed.
noyau@chromium.org changed reviewers: + noyau@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/large_icon_cache.cc (right): https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.cc:13: const int kMaxCacheSize = 12; Do you need to cache 12 of them on all platforms? Should this number be smaller on devices with a smaller form factor? https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.h:32: class LargeIconCache : public KeyedService { I would expect some tests for this class. https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.h:50: base::OwningMRUCache<GURL, LargeIconCacheEntry*> cache_; You cache all those images in memory? From our experience doing tiled image on bookmarks, keeping the cache on disk was sufficient: The costly bits where the image manipulations (resizes, filters...). Once it is a bitmap of the right size loading asynchronously is barely perceptible by the user.
Message was sent while issue was closed.
https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/large_icon_cache.cc (right): https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.cc:13: const int kMaxCacheSize = 12; On 2015/10/30 23:22:31, noyau wrote: > Do you need to cache 12 of them on all platforms? Should this number be smaller > on devices with a smaller form factor? Right now smaller form factors still load 12 favicons. Even if they load less, the cache only fills up if 12 unique GURLs are loaded. https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/large_icon_cache.h (right): https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.h:32: class LargeIconCache : public KeyedService { On 2015/10/30 23:22:31, noyau wrote: > I would expect some tests for this class. I will add this in a followup CL. https://codereview.chromium.org/1413903008/diff/170001/ios/chrome/browser/fav... ios/chrome/browser/favicon/large_icon_cache.h:50: base::OwningMRUCache<GURL, LargeIconCacheEntry*> cache_; On 2015/10/30 23:22:31, noyau wrote: > You cache all those images in memory? From our experience doing tiled image on > bookmarks, keeping the cache on disk was sufficient: The costly bits where the > image manipulations (resizes, filters...). Once it is a bitmap of the right size > loading asynchronously is barely perceptible by the user. The NTP slide-in animation needs the images loaded synchronously. Also, it took upwards of 200ms to load all the favicons on an iPhone 6s. |
