Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
262543002:
android: add ThumbnailCache (Closed)
|
Created:
6 years, 7 months ago by powei Modified:
6 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, no sievers Base URL:
https://chromium.googlesource.com/chromium/src.git@codec Visibility:
Public. |
Descriptionandroid: add ThumbnailCache
The purpose of the thumbnail cache is to save resources and use
static image instead of live pages when contents of a tab are
needed for the browser UI.
There are three forms of caching:
- High-res
- Low-res
- On-disk
If a high-res thumbnail is not available, then a low-res thumbnail
is used while the on-disk version (always high-res) is being read.
Eviction from the high/low-res caches is determined by a
priority-ordered list of tab ids that is updated by the user.
Compression of the thumbnail bitmaps are supported through
third_party/android_opengl/etc1. The raw bitmaps are discarded
once a compressed version is available. Compressed bitmaps are
written to disk and uploaded as UIResource. Once both of tasks
are complete, the compressed bitmap is also discarded.
depends on: https://codereview.chromium.org/287593002/
android= https://chrome-internal-review.googlesource.com/#/c/160907/
BUG=341406, 357740
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284861
Patch Set 1 #Patch Set 2 : upstreamed #Patch Set 3 : remove ntp and pinned #Patch Set 4 : removed stacked reads #Patch Set 5 : renamed ThumbnailCacheCore to ThumbnailCache #
Total comments: 2
Patch Set 6 : separate into multiple files #
Total comments: 34
Patch Set 7 : addressed some of dtrainor's comments #Patch Set 8 : addressed comments #
Total comments: 47
Patch Set 9 : addressed comments #Patch Set 10 : addressed more comments #Patch Set 11 : nits #Patch Set 12 : rename ThumbnailCache to ThumbnailStore to avoid downstream conflict #Patch Set 13 : added DEPS file #Messages
Total messages: 26 (0 generated)
PTAL. tedchoc@ for tab_android changes aelias@ and dtrainor@ for thumbnail_cache Sorry for the humongous patch. Feel free to take your time with this. This is mostly a port of the downstream version. You should be able to find 1-1 correspondence between the methods here and the old java code. Suggestions on how to split it up are also welcomed. The things I took out are the specialized code for ntp and the "pinned" thumbnails. These are paths I don't think are hit anymore. I removed them and did not notice any major visual artifacts.
High level comments on implementation. It looks like mostly a Java -> C++ rewrite. Since the Java was patched multiple times to keep it up to date with the renderer changes, can we make improvements here? I don't want to carry all of the baggage over since we're rewriting it now anyway. Some high level design ideas that might be helpful/that I was considering when I wanted to do some cleanup here: - Pull out LRU expiring cache to it's own file - Pull out a WorkerQueue class and move read/write/compress to their own instances of the worker queues? Do we already have a class that does this in Chromium? - Pull out the approximations to their own instance. A specific cache could only know about a single directory, a scale and a max size. Then something else could manage the two different caches for high/low res. That way we could get approximations being saved to disk for free and the code would be more straight forward IMO. - Need to port all of the unit tests over (or at least some set of tests).
PTAL again. Thanks! Simplified and separated the core file into multiple files. TODOs include a design doc and porting the tests.
https://chromiumcodereview.appspot.com/262543002/diff/100001/chrome/browser/a... File chrome/browser/android/tab_thumbnail_provider.h (right): https://chromiumcodereview.appspot.com/262543002/diff/100001/chrome/browser/a... chrome/browser/android/tab_thumbnail_provider.h:14: class TabThumbnailProvider { We might not need this interface at this point... especially if we're looking at cleaning this up more in the immediate future. https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... File chrome/browser/android/thumbnail/lru_expiring_cache.h (right): https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/lru_expiring_cache.h:52: private: DISALLOW_COPY_AND_ASSIGN https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/lru_expiring_cache.h:55: iterator it = map_.begin(); Just checking, is the cache supposed to expire based on insert ordering or query ordering? What does a LinkedHashMap do by default for begin()? https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:177: for (TabIdList::const_iterator visible_iter = visible_ids_.begin(), A while loop might look a bit cleaner. https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:199: std::find(read_queue_.begin(), read_queue_.end(), tab_id) == Is this to check if the same id is in the list twice? https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:306: if (write_thumbnail->tab_id() != tab_id) Why not if (==) iter.remove()? Same below and for RemoveDuplicateIdsFromQueueHelper https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:358: void ThumbnailCache::WriteNextThumbnail() { What if we're already waiting for a write to finish? Also (not sure if other version handled this) what if the thumbail should be removed (tab closed) during the write task? I assume the delete task will be queued behind this? https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:369: thumbnail->PassDataToNewThumbnail(); Won't this steal the reference from the object in the cache? Also, why are we passing a new instance of this in here? The class should be thread safe or we should just pass the ref counted pixel data. https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:392: RemoveFileFromDisk(file_path); Is posting this safe? What if the app dies before this task runs? Then we have an invalid file on disk. Since we're already on the FILE thread can we just delete it here? https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:445: void ThumbnailCache::CompressNextThumbnail() { Shouldn't this queue up the tasks and only run one at once? Otherwise what's the point of the compression_queue_/size limit/the ability to cancel stuff that's waiting to be compressed? https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:451: thumbnail->PassDataToNewThumbnail(); Doesn't this also steal the bitmap/data from the thumbnail in the cache? https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail_impl.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_impl.cc:29: gfx::Size GetEncodedSize(gfx::Size bitmap_size) { const gfx::Size& https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_impl.cc:80: void ThumbnailImpl::Compress(scoped_refptr<ThumbnailImpl> in_thumbnail, I reviewed this before the actual cache, but why have a separate object if this class holds both types of data internally? Threading simplifications? https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_impl.cc:178: gfx::Size content_size = gfx::Size(width, height); gfx::Size content_size(width, height); Same for below
https://codereview.chromium.org/262543002/diff/100001/chrome/browser/android/... File chrome/browser/android/tab_thumbnail_provider.h (right): https://codereview.chromium.org/262543002/diff/100001/chrome/browser/android/... chrome/browser/android/tab_thumbnail_provider.h:14: class TabThumbnailProvider { On 2014/06/17 08:12:10, David Trainor wrote: > We might not need this interface at this point... especially if we're looking at > cleaning this up more in the immediate future. Done. Removed in the next ps. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... File chrome/browser/android/thumbnail/lru_expiring_cache.h (right): https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/lru_expiring_cache.h:52: private: On 2014/06/17 08:12:11, David Trainor wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/lru_expiring_cache.h:55: iterator it = map_.begin(); On 2014/06/17 08:12:11, David Trainor wrote: > Just checking, is the cache supposed to expire based on insert ordering or query > ordering? What does a LinkedHashMap do by default for begin()? LinkedHashMap is insertion ordered. I think that's the same as the original Java code IIRC. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:177: for (TabIdList::const_iterator visible_iter = visible_ids_.begin(), On 2014/06/17 08:12:11, David Trainor wrote: > A while loop might look a bit cleaner. Done. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:199: std::find(read_queue_.begin(), read_queue_.end(), tab_id) == On 2014/06/17 08:12:11, David Trainor wrote: > Is this to check if the same id is in the list twice? Yes it is. We probably don't need this if we're sure that ids in |priority| are unique. Removed. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:306: if (write_thumbnail->tab_id() != tab_id) On 2014/06/17 08:12:11, David Trainor wrote: > Why not if (==) iter.remove()? Same below and for > RemoveDuplicateIdsFromQueueHelper Done. changed to "iter = write_queue_.erase(iter);" https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:358: void ThumbnailCache::WriteNextThumbnail() { On 2014/06/17 08:12:11, David Trainor wrote: > What if we're already waiting for a write to finish? Also (not sure if other > version handled this) what if the thumbail should be removed (tab closed) during > the write task? I assume the delete task will be queued behind this? Are you thinking about how the size of the write_queue_ does not reflect the number of write tasks in-flight? Maybe I can add a count of the currently in-flight write-tasks. So then the block in WriteThumbnailIfNecessary() would be if (write_queue_.size() + executing_write_task_count_ > write_queue_max_size_) return; or something similar. The delete task is posted to the same FILE thread. So delete and write tasks are sequenced. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:369: thumbnail->PassDataToNewThumbnail(); On 2014/06/17 08:12:11, David Trainor wrote: > Won't this steal the reference from the object in the cache? Also, why are we > passing a new instance of this in here? The class should be thread safe or we > should just pass the ref counted pixel data. The point is to steal the reference. This way we're sure about the lifetime of the pixels. After the |in_thumbnail| is written it goes out of scope and the pixels are gone. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:392: RemoveFileFromDisk(file_path); On 2014/06/17 08:12:11, David Trainor wrote: > Is posting this safe? What if the app dies before this task runs? Then we have > an invalid file on disk. Since we're already on the FILE thread can we just > delete it here? Done. Good catch. called DeleteFile directly instead of posting. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:445: void ThumbnailCache::CompressNextThumbnail() { On 2014/06/17 08:12:11, David Trainor wrote: > Shouldn't this queue up the tasks and only run one at once? Otherwise what's > the point of the compression_queue_/size limit/the ability to cancel stuff > that's waiting to be compressed? see comment for write task. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:451: thumbnail->PassDataToNewThumbnail(); On 2014/06/17 08:12:11, David Trainor wrote: > Doesn't this also steal the bitmap/data from the thumbnail in the cache? This does steal the reference, and once compression is complete, the reference goes out of scope and pixels are gone. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... File chrome/browser/android/thumbnail/thumbnail_impl.cc (right): https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_impl.cc:29: gfx::Size GetEncodedSize(gfx::Size bitmap_size) { On 2014/06/17 08:12:11, David Trainor wrote: > const gfx::Size& Done. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_impl.cc:80: void ThumbnailImpl::Compress(scoped_refptr<ThumbnailImpl> in_thumbnail, On 2014/06/17 08:12:11, David Trainor wrote: > I reviewed this before the actual cache, but why have a separate object if this > class holds both types of data internally? Threading simplifications? This is just to make this method be more functional. I tried to make all thumbnail data manipulation methods static so we can be more certain of the lifetime and content (raw or compressed) of the thumbnail. Maybe it'll be better to make this method const and non-static? https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_impl.cc:178: gfx::Size content_size = gfx::Size(width, height); On 2014/06/17 08:12:11, David Trainor wrote: > gfx::Size content_size(width, height); > > Same for below Done. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_impl.cc:270: BuildUIResource(); Just realized this can be called on the wrong thread. Moved the creation to ui_resource_id() so the resource is created when it is demanded.
https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:358: void ThumbnailCache::WriteNextThumbnail() { On 2014/06/19 23:05:59, powei wrote: > On 2014/06/17 08:12:11, David Trainor wrote: > > What if we're already waiting for a write to finish? Also (not sure if other > > version handled this) what if the thumbail should be removed (tab closed) > during > > the write task? I assume the delete task will be queued behind this? > > Are you thinking about how the size of the write_queue_ does not reflect the > number of write tasks in-flight? Maybe I can add a count of the currently > in-flight write-tasks. So then the block in WriteThumbnailIfNecessary() would > be > > if (write_queue_.size() + executing_write_task_count_ > write_queue_max_size_) > return; > > or something similar. > > The delete task is posted to the same FILE thread. So delete and write tasks > are sequenced. I'm more worried that we have a write queue that doesn't really represent what's actually queued up. The idea of the queue was that we don't write more than one thing at a time. Can this queue ever be non-empty? It looks like we just push stuff on the queue and immediately pop it and post the task to the IO thread. Then we also need to look at how easy/hard it is to cancel write tasks if we need to (ie: how would we cancel a pending write task if the tab was closed after we grabbed a thumbnail? how would we clean that up if we hit that somewhat common case?). https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:369: thumbnail->PassDataToNewThumbnail(); On 2014/06/19 23:05:58, powei wrote: > On 2014/06/17 08:12:11, David Trainor wrote: > > Won't this steal the reference from the object in the cache? Also, why are we > > passing a new instance of this in here? The class should be thread safe or we > > should just pass the ref counted pixel data. > > The point is to steal the reference. This way we're sure about the lifetime of > the pixels. After the |in_thumbnail| is written it goes out of scope and the > pixels are gone. But since we're building the ui resource id on demand who's to say the ThumnailImpl in the cache doesn't need those pixels? 1. cache_->put(new ThumbnailImpl); 2. WriteTask() 3. cache_->get() (pixels dead, need to build the ui resource id). https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:451: thumbnail->PassDataToNewThumbnail(); On 2014/06/19 23:05:59, powei wrote: > On 2014/06/17 08:12:11, David Trainor wrote: > > Doesn't this also steal the bitmap/data from the thumbnail in the cache? > > This does steal the reference, and once compression is complete, the reference > goes out of scope and pixels are gone. Still not sure how this works if we compress before building the resource id. https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail_impl.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_impl.cc:80: void ThumbnailImpl::Compress(scoped_refptr<ThumbnailImpl> in_thumbnail, On 2014/06/19 23:05:59, powei wrote: > On 2014/06/17 08:12:11, David Trainor wrote: > > I reviewed this before the actual cache, but why have a separate object if > this > > class holds both types of data internally? Threading simplifications? > > This is just to make this method be more functional. I tried to make all > thumbnail data manipulation methods static so we can be more certain of the > lifetime and content (raw or compressed) of the thumbnail. > > Maybe it'll be better to make this method const and non-static? Could it still act on the same class that's passed in? It could still be a static method I think. https://chromiumcodereview.appspot.com/262543002/diff/140001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_impl.cc:270: BuildUIResource(); On 2014/06/19 23:05:59, powei wrote: > Just realized this can be called on the wrong thread. Moved the creation to > ui_resource_id() so the resource is created when it is demanded. Won't this defeat the purpose of using the UI resource id instead of the bitmap in an image layer? We want to drop the bitmap/compressed data as soon as we can so we don't eat up more memory.
more comments addressed. Offline discussion led to using raw data instead of the wrapper class ThumbnailImpl for compression/write. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:358: void ThumbnailCache::WriteNextThumbnail() { On 2014/06/25 07:40:15, David Trainor wrote: > On 2014/06/19 23:05:59, powei wrote: > > On 2014/06/17 08:12:11, David Trainor wrote: > > > What if we're already waiting for a write to finish? Also (not sure if > other > > > version handled this) what if the thumbail should be removed (tab closed) > > during > > > the write task? I assume the delete task will be queued behind this? > > > > Are you thinking about how the size of the write_queue_ does not reflect the > > number of write tasks in-flight? Maybe I can add a count of the currently > > in-flight write-tasks. So then the block in WriteThumbnailIfNecessary() would > > be > > > > if (write_queue_.size() + executing_write_task_count_ > write_queue_max_size_) > > return; > > > > or something similar. > > > > The delete task is posted to the same FILE thread. So delete and write tasks > > are sequenced. > > I'm more worried that we have a write queue that doesn't really represent what's > actually queued up. The idea of the queue was that we don't write more than one > thing at a time. Can this queue ever be non-empty? It looks like we just push > stuff on the queue and immediately pop it and post the task to the IO thread. > > Then we also need to look at how easy/hard it is to cancel write tasks if we > need to (ie: how would we cancel a pending write task if the tab was closed > after we grabbed a thumbnail? how would we clean that up if we hit that > somewhat common case?). Agreed offline that we'd keep a count instead of an actual queue. Canceling is tabled fo rnow. https://codereview.chromium.org/262543002/diff/140001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:369: thumbnail->PassDataToNewThumbnail(); On 2014/06/25 07:40:16, David Trainor wrote: > On 2014/06/19 23:05:58, powei wrote: > > On 2014/06/17 08:12:11, David Trainor wrote: > > > Won't this steal the reference from the object in the cache? Also, why are > we > > > passing a new instance of this in here? The class should be thread safe or > we > > > should just pass the ref counted pixel data. > > > > The point is to steal the reference. This way we're sure about the lifetime > of > > the pixels. After the |in_thumbnail| is written it goes out of scope and the > > pixels are gone. > > But since we're building the ui resource id on demand who's to say the > ThumnailImpl in the cache doesn't need those pixels? > > 1. cache_->put(new ThumbnailImpl); > 2. WriteTask() > 3. cache_->get() (pixels dead, need to build the ui resource id). Done. Moved to more explicit passing of ref-counted pixels.
https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail.cc:93: bitmap_ = cc::UIResourceBitmap(kSmallHolderBitmap); Not sure if this is a problem (haven't seen other code yet) but what happens if we try to query the pixels to compress/write to disk after GetBitmap() is called? Can we just explicitly clear the bitmap when we're done writing it to disk by building the ui resource and freeing the bitmap? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail.h (right): https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail.h:31: class ThumbnailManager { ThumbnailDelegate maybe? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail.h:51: gfx::SizeF GetScaledContentSize() const { return scaled_content_size_; } const gfx::SizeF& and scaled_content_size (or put the definition in cc file). Same with below. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail.h:56: gfx::Size content_size); const gfx::Size& https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:112: cache_.Put(tab_id, thumbnail); I'm not a huge fan of using references instead of pointers since if you/someone else decided to touch thumbnail after this line it would be acting on a different instance of the object. thumbnail will be destroyed when this function leaves scope. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:136: const Thumbnail& ThumbnailCache::Get(TabId tab_id) { This feels a bit heavyweight, as in someone has to check if the thumbnail exists before calling Get even though NULL is expected for some tabs. Can this return a pointer/NULL instead? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:143: if (written_thumbnails_.find(tab_id) != written_thumbnails_.end() && written_thumbnails_ won't be up to date if we restart. Do we need this variable? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:282: if (write_tasks_count_ >= write_queue_max_size_) What do we do if we're over this limit? Do we just never write it? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:291: compressed_data, Huh is skia::RefPtr<> thread safe? I just realized from looking at your CompressedDataTransport https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:308: if (compression_tasks_count_ >= compression_queue_max_size_) What do we do if we're over this limit? If we just return here we won't write/compress the data ever right? Should we just drop the thumbnail completely? What do you think we should do? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:323: transport); Is this the only way to do this? Do you need to know all the parameters when calling back to the reply? That seems odd (or not, I don't really now the APIs for this base::Closure). I wonder if you need to create this object here. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:617: cache_.Put(tab_id, thumbnail); thumbnail.CreateUIResource?
https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail.cc:93: bitmap_ = cc::UIResourceBitmap(kSmallHolderBitmap); On 2014/07/14 20:30:45, David Trainor wrote: > Not sure if this is a problem (haven't seen other code yet) but what happens if > we try to query the pixels to compress/write to disk after GetBitmap() is > called? Can we just explicitly clear the bitmap when we're done writing it to > disk by building the ui resource and freeing the bitmap? The write task is a ref-holder of the same pixels, and once the task and this thumbnail in the cache are done with the pixels, they each release the reference, and the bitmap is released. In this way, the write task is oblivious of what happens with the thumbnail in |cache_|. I guess my thinking is that it is cleaner to keep the logic of the tasks apart from the logic here. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:143: if (written_thumbnails_.find(tab_id) != written_thumbnails_.end() && On 2014/07/14 20:30:46, David Trainor wrote: > written_thumbnails_ won't be up to date if we restart. Do we need this > variable? This use to fallback to just checking the disk, but that created a pretty noticeable regression. Maybe we can check the disk on ThumbnailCache creation to keep written_thumbnails_ update to date? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:282: if (write_tasks_count_ >= write_queue_max_size_) On 2014/07/14 20:30:46, David Trainor wrote: > What do we do if we're over this limit? Do we just never write it? Yes, it is just never written. Maybe we'll need to drop the thumbnail like with compression? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:291: compressed_data, On 2014/07/14 20:30:46, David Trainor wrote: > Huh is skia::RefPtr<> thread safe? I just realized from looking at your > CompressedDataTransport SkRefCnt does use atomic operations and barrier for thread safety. https://cs.corp.google.com/#clankium/src/third_party/skia/include/core/SkRefC... https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:308: if (compression_tasks_count_ >= compression_queue_max_size_) On 2014/07/14 20:30:46, David Trainor wrote: > What do we do if we're over this limit? If we just return here we won't > write/compress the data ever right? Should we just drop the thumbnail > completely? What do you think we should do? Yeah, maybe we'll need to drop if the approximated version is not desirable. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:323: transport); On 2014/07/14 20:30:46, David Trainor wrote: > Is this the only way to do this? Do you need to know all the parameters when > calling back to the reply? That seems odd (or not, I don't really now the APIs > for this base::Closure). I wonder if you need to create this object here. Closure should have no unbound parameters (https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h). And PostTaskAndReply takes reply as a closure. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:617: cache_.Put(tab_id, thumbnail); On 2014/07/14 20:30:46, David Trainor wrote: > thumbnail.CreateUIResource? Just to make sure. This means we prefer cpu mem over video mem?
https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... File chrome/browser/android/thumbnail/thumbnail.h (right): https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail.h:31: class ThumbnailManager { On 2014/07/14 20:30:45, David Trainor wrote: > ThumbnailDelegate maybe? Done. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail.h:51: gfx::SizeF GetScaledContentSize() const { return scaled_content_size_; } On 2014/07/14 20:30:46, David Trainor wrote: > const gfx::SizeF& and scaled_content_size (or put the definition in cc file). > Same with below. Done. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail.h:56: gfx::Size content_size); On 2014/07/14 20:30:45, David Trainor wrote: > const gfx::Size& Done. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:112: cache_.Put(tab_id, thumbnail); On 2014/07/14 20:30:46, David Trainor wrote: > I'm not a huge fan of using references instead of pointers since if you/someone > else decided to touch thumbnail after this line it would be acting on a > different instance of the object. thumbnail will be destroyed when this > function leaves scope. Done. Note that lru_expiring_cache is now scoped_ptr_expiring_cache https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:136: const Thumbnail& ThumbnailCache::Get(TabId tab_id) { On 2014/07/14 20:30:46, David Trainor wrote: > This feels a bit heavyweight, as in someone has to check if the thumbnail exists > before calling Get even though NULL is expected for some tabs. Can this return > a pointer/NULL instead? Done. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:143: if (written_thumbnails_.find(tab_id) != written_thumbnails_.end() && On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:46, David Trainor wrote: > > written_thumbnails_ won't be up to date if we restart. Do we need this > > variable? > > This use to fallback to just checking the disk, but that created a pretty > noticeable regression. Maybe we can check the disk on ThumbnailCache creation > to keep written_thumbnails_ update to date? |written_thumbnails_| is a misnomer. It's more like a subset of the written thumbnails. If a thumbnail is not in this subset, then we do not want to schedule a read for that thumbnail here (but it might later be read in UpdateVisibleIds). Renamed to |written_thumbnails_subset_| for now. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:282: if (write_tasks_count_ >= write_queue_max_size_) On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:46, David Trainor wrote: > > What do we do if we're over this limit? Do we just never write it? > > Yes, it is just never written. Maybe we'll need to drop the thumbnail like with > compression? Actually, I think we can live with not writing it and just have the compressed version, which should be in |cache_|. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:308: if (compression_tasks_count_ >= compression_queue_max_size_) On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:46, David Trainor wrote: > > What do we do if we're over this limit? If we just return here we won't > > write/compress the data ever right? Should we just drop the thumbnail > > completely? What do you think we should do? > > Yeah, maybe we'll need to drop if the approximated version is not desirable. Done. Added line to remove the cached version. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:617: cache_.Put(tab_id, thumbnail); On 2014/07/14 20:30:46, David Trainor wrote: > thumbnail.CreateUIResource? Done.
lgtm https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail.cc:93: bitmap_ = cc::UIResourceBitmap(kSmallHolderBitmap); On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:45, David Trainor wrote: > > Not sure if this is a problem (haven't seen other code yet) but what happens > if > > we try to query the pixels to compress/write to disk after GetBitmap() is > > called? Can we just explicitly clear the bitmap when we're done writing it to > > disk by building the ui resource and freeing the bitmap? > > The write task is a ref-holder of the same pixels, and once the task and this > thumbnail in the cache are done with the pixels, they each release the > reference, and the bitmap is released. > > In this way, the write task is oblivious of what happens with the thumbnail in > |cache_|. I guess my thinking is that it is cleaner to keep the logic of the > tasks apart from the logic here. Yeah that's fine with me :). It looks like you aren't querying this reference at all in your pre-task functions so it's fine. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:143: if (written_thumbnails_.find(tab_id) != written_thumbnails_.end() && On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:46, David Trainor wrote: > > written_thumbnails_ won't be up to date if we restart. Do we need this > > variable? > > This use to fallback to just checking the disk, but that created a pretty > noticeable regression. Maybe we can check the disk on ThumbnailCache creation > to keep written_thumbnails_ update to date? What kind of regression? Couldn't the read task just return null if the file didn't exist and we just don't add the thumbnail? This seemed to work fine in the Java version I'd almost not rather try to keep another list in sync. If it's a significant regression we probably also don't want to delay startup time. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:282: if (write_tasks_count_ >= write_queue_max_size_) On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:46, David Trainor wrote: > > What do we do if we're over this limit? Do we just never write it? > > Yes, it is just never written. Maybe we'll need to drop the thumbnail like with > compression? Do these max write queue/compress queue checks do anything useful? I wonder if the code to track how often to grab a thumbnail bitmap end up being enough and we can remove these. Could you do some tests to see if we can hit these? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:308: if (compression_tasks_count_ >= compression_queue_max_size_) On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:46, David Trainor wrote: > > What do we do if we're over this limit? If we just return here we won't > > write/compress the data ever right? Should we just drop the thumbnail > > completely? What do you think we should do? > > Yeah, maybe we'll need to drop if the approximated version is not desirable. Yeah that makes sense :) https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:323: transport); On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:46, David Trainor wrote: > > Is this the only way to do this? Do you need to know all the parameters when > > calling back to the reply? That seems odd (or not, I don't really now the > APIs > > for this base::Closure). I wonder if you need to create this object here. > > Closure should have no unbound parameters > (https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h). > And PostTaskAndReply takes reply as a closure. Do you have to use PostTaskAndReply? Would it be cleaner to have the task post another task back to the UI thread when it's done with the right arguments? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:331: if (read_queue_.empty()) I'm still a bit confused by this queue :(. It might/might not actually represent what's queued up to be read (we could have 4-5 tasks posted to the io thread depending on how you call certain methods... which means this queue might not actually represent what's happening). That means you could in theory read in the same tab a bunch of times right? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:458: file.Close(); Do you have to close this before you delete the file? Maybe simulate a failed write and make sure this works? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:522: return; Do we need to clean anything up here? https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:617: cache_.Put(tab_id, thumbnail); On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:46, David Trainor wrote: > > thumbnail.CreateUIResource? > > Just to make sure. This means we prefer cpu mem over video mem? Yes. At least when it was in Dalvik that was the case. Maybe add a flag/constant we can tweak to determine whether or not this happens then we can run some tests.
On 2014/07/15 20:08:24, David Trainor wrote: > lgtm > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > File chrome/browser/android/thumbnail/thumbnail.cc (right): > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > chrome/browser/android/thumbnail/thumbnail.cc:93: bitmap_ = > cc::UIResourceBitmap(kSmallHolderBitmap); > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:45, David Trainor wrote: > > > Not sure if this is a problem (haven't seen other code yet) but what happens > > if > > > we try to query the pixels to compress/write to disk after GetBitmap() is > > > called? Can we just explicitly clear the bitmap when we're done writing it > to > > > disk by building the ui resource and freeing the bitmap? > > > > The write task is a ref-holder of the same pixels, and once the task and this > > thumbnail in the cache are done with the pixels, they each release the > > reference, and the bitmap is released. > > > > In this way, the write task is oblivious of what happens with the thumbnail in > > |cache_|. I guess my thinking is that it is cleaner to keep the logic of the > > tasks apart from the logic here. > > Yeah that's fine with me :). It looks like you aren't querying this reference > at all in your pre-task functions so it's fine. > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > chrome/browser/android/thumbnail/thumbnail_cache.cc:143: if > (written_thumbnails_.find(tab_id) != written_thumbnails_.end() && > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:46, David Trainor wrote: > > > written_thumbnails_ won't be up to date if we restart. Do we need this > > > variable? > > > > This use to fallback to just checking the disk, but that created a pretty > > noticeable regression. Maybe we can check the disk on ThumbnailCache creation > > to keep written_thumbnails_ update to date? > > What kind of regression? Couldn't the read task just return null if the file > didn't exist and we just don't add the thumbnail? This seemed to work fine in > the Java version I'd almost not rather try to keep another list in sync. If > it's a significant regression we probably also don't want to delay startup time. > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > chrome/browser/android/thumbnail/thumbnail_cache.cc:282: if (write_tasks_count_ > >= write_queue_max_size_) > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:46, David Trainor wrote: > > > What do we do if we're over this limit? Do we just never write it? > > > > Yes, it is just never written. Maybe we'll need to drop the thumbnail like > with > > compression? > > Do these max write queue/compress queue checks do anything useful? I wonder if > the code to track how often to grab a thumbnail bitmap end up being enough and > we can remove these. Could you do some tests to see if we can hit these? > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > chrome/browser/android/thumbnail/thumbnail_cache.cc:308: if > (compression_tasks_count_ >= compression_queue_max_size_) > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:46, David Trainor wrote: > > > What do we do if we're over this limit? If we just return here we won't > > > write/compress the data ever right? Should we just drop the thumbnail > > > completely? What do you think we should do? > > > > Yeah, maybe we'll need to drop if the approximated version is not desirable. > > Yeah that makes sense :) > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > chrome/browser/android/thumbnail/thumbnail_cache.cc:323: transport); > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:46, David Trainor wrote: > > > Is this the only way to do this? Do you need to know all the parameters > when > > > calling back to the reply? That seems odd (or not, I don't really now the > > APIs > > > for this base::Closure). I wonder if you need to create this object here. > > > > Closure should have no unbound parameters > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h). > > > And PostTaskAndReply takes reply as a closure. > > Do you have to use PostTaskAndReply? Would it be cleaner to have the task post > another task back to the UI thread when it's done with the right arguments? > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > chrome/browser/android/thumbnail/thumbnail_cache.cc:331: if > (read_queue_.empty()) > I'm still a bit confused by this queue :(. It might/might not actually > represent what's queued up to be read (we could have 4-5 tasks posted to the io > thread depending on how you call certain methods... which means this queue might > not actually represent what's happening). That means you could in theory read > in the same tab a bunch of times right? > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > chrome/browser/android/thumbnail/thumbnail_cache.cc:458: file.Close(); > Do you have to close this before you delete the file? Maybe simulate a failed > write and make sure this works? > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > chrome/browser/android/thumbnail/thumbnail_cache.cc:522: return; > Do we need to clean anything up here? > > https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... > chrome/browser/android/thumbnail/thumbnail_cache.cc:617: cache_.Put(tab_id, > thumbnail); > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:46, David Trainor wrote: > > > thumbnail.CreateUIResource? > > > > Just to make sure. This means we prefer cpu mem over video mem? > > Yes. At least when it was in Dalvik that was the case. Maybe add a > flag/constant we can tweak to determine whether or not this happens then we can > run some tests. Gah err not lgtm quite yet sorry haha! Just meant to post more comments... plz wait until we've resolved the threads before landing! Soon though.
https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... File chrome/browser/android/thumbnail/thumbnail.cc (right): https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail.cc:93: bitmap_ = cc::UIResourceBitmap(kSmallHolderBitmap); On 2014/07/15 20:08:23, David Trainor wrote: > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:45, David Trainor wrote: > > > Not sure if this is a problem (haven't seen other code yet) but what happens > > if > > > we try to query the pixels to compress/write to disk after GetBitmap() is > > > called? Can we just explicitly clear the bitmap when we're done writing it > to > > > disk by building the ui resource and freeing the bitmap? > > > > The write task is a ref-holder of the same pixels, and once the task and this > > thumbnail in the cache are done with the pixels, they each release the > > reference, and the bitmap is released. > > > > In this way, the write task is oblivious of what happens with the thumbnail in > > |cache_|. I guess my thinking is that it is cleaner to keep the logic of the > > tasks apart from the logic here. > > Yeah that's fine with me :). It looks like you aren't querying this reference > at all in your pre-task functions so it's fine. Acknowledged. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:143: if (written_thumbnails_.find(tab_id) != written_thumbnails_.end() && On 2014/07/15 20:08:24, David Trainor wrote: > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:46, David Trainor wrote: > > > written_thumbnails_ won't be up to date if we restart. Do we need this > > > variable? > > > > This use to fallback to just checking the disk, but that created a pretty > > noticeable regression. Maybe we can check the disk on ThumbnailCache creation > > to keep written_thumbnails_ update to date? > > What kind of regression? Couldn't the read task just return null if the file > didn't exist and we just don't add the thumbnail? This seemed to work fine in > the Java version I'd almost not rather try to keep another list in sync. If > it's a significant regression we probably also don't want to delay startup time. Currently we ask for the *static page* (i.e. thumbnail) even when only the live page is visible. The java code had a flag that only goes to disk when that flag is true. I've added that flag back in and removed this |written_thumbnails_| https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:282: if (write_tasks_count_ >= write_queue_max_size_) On 2014/07/15 20:08:23, David Trainor wrote: > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:46, David Trainor wrote: > > > What do we do if we're over this limit? Do we just never write it? > > > > Yes, it is just never written. Maybe we'll need to drop the thumbnail like > with > > compression? > > Do these max write queue/compress queue checks do anything useful? I wonder if > the code to track how often to grab a thumbnail bitmap end up being enough and > we can remove these. Could you do some tests to see if we can hit these? I did some manual testing. We do hit the compression check (compression_queue_max_size_ == 5 for the debug build). And since the write happens only after compression and write seems faster, I've never seen this write check getting hit. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:323: transport); On 2014/07/15 20:08:24, David Trainor wrote: > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:46, David Trainor wrote: > > > Is this the only way to do this? Do you need to know all the parameters > when > > > calling back to the reply? That seems odd (or not, I don't really now the > > APIs > > > for this base::Closure). I wonder if you need to create this object here. > > > > Closure should have no unbound parameters > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h). > > > And PostTaskAndReply takes reply as a closure. > > Do you have to use PostTaskAndReply? Would it be cleaner to have the task post > another task back to the UI thread when it's done with the right arguments? Done. Pass the Post*Task() as callback params to to *Task() methods. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:331: if (read_queue_.empty()) On 2014/07/15 20:08:24, David Trainor wrote: > I'm still a bit confused by this queue :(. It might/might not actually > represent what's queued up to be read (we could have 4-5 tasks posted to the io > thread depending on how you call certain methods... which means this queue might > not actually represent what's happening). That means you could in theory read > in the same tab a bunch of times right? I added a boolean to indicate that a read is currently being carried out. So only one read should be in flight at any time. The read queue is not dequeued until the read is finished (PostReadTask), and there are checks to ensure the queue does not contain duplicates. So after adding the above bool flag, we won't see multiple reads for the same tab in the queue. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:458: file.Close(); On 2014/07/15 20:08:23, David Trainor wrote: > Do you have to close this before you delete the file? Maybe simulate a failed > write and make sure this works? Done. Moved close to before delete. https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:522: return; On 2014/07/15 20:08:24, David Trainor wrote: > Do we need to clean anything up here? Done.
Aaaah lgtm. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:282: if (write_tasks_count_ >= write_queue_max_size_) On 2014/07/18 00:51:49, powei wrote: > On 2014/07/15 20:08:23, David Trainor wrote: > > On 2014/07/14 23:56:25, powei wrote: > > > On 2014/07/14 20:30:46, David Trainor wrote: > > > > What do we do if we're over this limit? Do we just never write it? > > > > > > Yes, it is just never written. Maybe we'll need to drop the thumbnail like > > with > > > compression? > > > > Do these max write queue/compress queue checks do anything useful? I wonder > if > > the code to track how often to grab a thumbnail bitmap end up being enough and > > we can remove these. Could you do some tests to see if we can hit these? > > I did some manual testing. We do hit the compression check > (compression_queue_max_size_ == 5 for the debug build). And since the write > happens only after compression and write seems faster, I've never seen this > write check getting hit. Oh interesting... if we're peaked out in the compression list we should probably drop the texture altogether so we don't just sit with the uncompressed version forever... I'm a bit surprised we hit it. We shouldn't be grabbing a thumbnail if we don't really have anything to show (and I can't really imagine us having stuff to show if we're swapping that fast). That was probably a bug in the old version too. https://chromiumcodereview.appspot.com/262543002/diff/210001/chrome/browser/a... chrome/browser/android/thumbnail/thumbnail_cache.cc:291: compressed_data, On 2014/07/14 23:56:25, powei wrote: > On 2014/07/14 20:30:46, David Trainor wrote: > > Huh is skia::RefPtr<> thread safe? I just realized from looking at your > > CompressedDataTransport > > SkRefCnt does use atomic operations and barrier for thread safety. > https://cs.corp.google.com/#clankium/src/third_party/skia/include/core/SkRefC... w00t
ping tedchoc@: Owner review for content/browser/android, content/public/browser, chrome/chrome_browser.gypi, and chrome/chrome_tests_unit.gypi Thanks! https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... chrome/browser/android/thumbnail/thumbnail_cache.cc:617: cache_.Put(tab_id, thumbnail); On 2014/07/15 20:08:23, David Trainor wrote: > On 2014/07/14 23:56:25, powei wrote: > > On 2014/07/14 20:30:46, David Trainor wrote: > > > thumbnail.CreateUIResource? > > > > Just to make sure. This means we prefer cpu mem over video mem? > > Yes. At least when it was in Dalvik that was the case. Maybe add a > flag/constant we can tweak to determine whether or not this happens then we can > run some tests. Done. Forgot the flag in the last patch. Now added.
On 2014/07/22 18:16:19, powei wrote: > ping tedchoc@: Owner review for content/browser/android, content/public/browser, > chrome/chrome_browser.gypi, and chrome/chrome_tests_unit.gypi > > Thanks! > > https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... > File chrome/browser/android/thumbnail/thumbnail_cache.cc (right): > > https://codereview.chromium.org/262543002/diff/210001/chrome/browser/android/... > chrome/browser/android/thumbnail/thumbnail_cache.cc:617: cache_.Put(tab_id, > thumbnail); > On 2014/07/15 20:08:23, David Trainor wrote: > > On 2014/07/14 23:56:25, powei wrote: > > > On 2014/07/14 20:30:46, David Trainor wrote: > > > > thumbnail.CreateUIResource? > > > > > > Just to make sure. This means we prefer cpu mem over video mem? > > > > Yes. At least when it was in Dalvik that was the case. Maybe add a > > flag/constant we can tweak to determine whether or not this happens then we > can > > run some tests. > > Done. Forgot the flag in the last patch. Now added. owners lgtm
The CQ bit was checked by powei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/262543002/330001
ping aelias@ for chrome/browser/android/DEPS. chromium_presubmit needs an OWNER of cc/resources for the DEPS change. Thanks.
Seems OK at a high level, but could you add a DEPS files to thumbnail/ directory and make a nonspecific rule for those dependencies instead?
On 2014/07/22 23:50:35, aelias wrote: > Seems OK at a high level, but could you add a DEPS files to thumbnail/ directory > and make a nonspecific rule for those dependencies instead? Done.
DEPS lgtm
The CQ bit was checked by powei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/262543002/390001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 284861 |
