Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1257103003:
Add Java-side LRU cache for bookmark favicons (Closed)
DescriptionAdd Java-side LRU cache for bookmark favicons
Add Java-side LRU cache for bookmark favicons to avoid refetching favicons from native. This provides a smoother experience; previously if you scrolled down then scrolled back up the favicons would be blank before loading.
BUG=510874
Committed: https://crrev.com/6af62b828e32f4b5e2070f148dd6f18512cb8dd5
Cr-Commit-Position: refs/heads/master@{#340710}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 6
Patch Set 3 : Changes from ianwen@ review #
Total comments: 4
Patch Set 4 : Remove null check in getLargeIcon, update comment #
Total comments: 2
Patch Set 5 : Clean up spacing #Messages
Total messages: 15 (3 generated)
twellington@chromium.org changed reviewers: + ianwen@chromium.org, kkimlabs@chromium.org
ptal Very similar to the LRU cache we were using for salient images (removed in this CL: https://codereview.chromium.org/1202713002/patch/80001/90020) with a few changes to support fallback color.
Ignore this for now, I need to rebase, will email when it's ready.
ptal - rebase done
https://codereview.chromium.org/1257103003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/1257103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:50: private static WeakReference<LruCache<String, Pair<Bitmap, Integer>>> sLruCache; Usually we place member variables in this order: 1. static final 2. static 3. normal Could you move #51 to #33 and #50 to #48? https://codereview.chromium.org/1257103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:51: private static final int FAVICON_MAX_CACHE_SIZE = 32 * 1024 * 1024; // 32MB Do we still need such a large cache? A 128*128 PNG favicon is roughly 10KB, and 32MB is sufficient to store 3276 favicons in memory. How about keeping a 10MB cache? I haven't check the real size of those bitmaps we show, but I suspect they are even smaller than 10kb in memory. https://codereview.chromium.org/1257103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:52: private LruCache<String, Pair<Bitmap, Integer>> mFaviconCache; Why do we need two cache references, one strong one weak? We might just do something similar in https://codereview.chromium.org/922943002/, and have only one weak reference. Another way is that since we no longer need images in detail activity, it won't hurt to just use strong references. It's up to you to make the decision then.
https://codereview.chromium.org/1257103003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/1257103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:50: private static WeakReference<LruCache<String, Pair<Bitmap, Integer>>> sLruCache; On 2015/07/27 18:36:05, Ian Wen wrote: > Usually we place member variables in this order: > 1. static final > 2. static > 3. normal > > Could you move #51 to #33 and #50 to #48? Done. https://codereview.chromium.org/1257103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:51: private static final int FAVICON_MAX_CACHE_SIZE = 32 * 1024 * 1024; // 32MB On 2015/07/27 18:36:05, Ian Wen wrote: > Do we still need such a large cache? A 128*128 PNG favicon is roughly 10KB, and > 32MB is sufficient to store 3276 favicons in memory. How about keeping a 10MB > cache? > > I haven't check the real size of those bitmaps we show, but I suspect they are > even smaller than 10kb in memory. Done. The bitmaps currently are only 1KB (according to my logged based testing; the favicons we're getting are only 16x16 right now). This will change once we use larger icons, but I think a 10MB cache is sufficient for even larger icons. https://codereview.chromium.org/1257103003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:52: private LruCache<String, Pair<Bitmap, Integer>> mFaviconCache; On 2015/07/27 18:36:05, Ian Wen wrote: > Why do we need two cache references, one strong one weak? > > We might just do something similar in > https://codereview.chromium.org/922943002/, and have only one weak reference. > > Another way is that since we no longer need images in detail activity, it won't > hurt to just use strong references. It's up to you to make the decision then. Done. Removed the weak reference.
lgtm after nits. Thanks! https://codereview.chromium.org/1257103003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/1257103003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:164: * @see LargeIconBridge#getLargeIconForUrl(Profile, String, int, LargeIconCallback) Update the comments and mention we have java cache layer here? https://codereview.chromium.org/1257103003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:170: if (mFaviconCache != null) { Is this null check still necessary? If destroy() is called before getLargeIcon then we had better let it crash to unveil the bug.
https://codereview.chromium.org/1257103003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/1257103003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:164: * @see LargeIconBridge#getLargeIconForUrl(Profile, String, int, LargeIconCallback) On 2015/07/27 20:22:39, Ian Wen wrote: > Update the comments and mention we have java cache layer here? Done. https://codereview.chromium.org/1257103003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:170: if (mFaviconCache != null) { On 2015/07/27 20:22:39, Ian Wen wrote: > Is this null check still necessary? If destroy() is called before getLargeIcon > then we had better let it crash to unveil the bug. Done.
lgtm could you comment the purpose of reintroducing the lru cache in the CL description? (I assume it's not due to the performance this time) https://codereview.chromium.org/1257103003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/1257103003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:76: return size; nit: two spaces after return.
https://codereview.chromium.org/1257103003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/1257103003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:76: return size; On 2015/07/28 07:05:22, Kibeom Kim wrote: > nit: two spaces after return. Done.
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org, kkimlabs@chromium.org Link to the patchset: https://codereview.chromium.org/1257103003/#ps80001 (title: "Clean up spacing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1257103003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1257103003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6af62b828e32f4b5e2070f148dd6f18512cb8dd5 Cr-Commit-Position: refs/heads/master@{#340710} |
