Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2294983002:
[Download Home] Add image thumbnail support (Closed)
Description[Download Home] Add image thumbnail support
First pass at making a basic thumbnail cache for Download Home.
Still a little janky, but that can be fixed once the skeleton is
in.
Screenshot that enforces that the biggest size is at least X and then crops out the rest:
https://drive.google.com/open?id=0B7c8ZkXVwskDN2FTTmEwOVdWekE
* Add a new ThumbnailProvider[Impl] class that produces thumbnails
for files displayed in Download Home.
- The Java ThumbnailProviderImpl maintains a queue of thumbnail
requests to process and a cache of thumbnails that have already
been processed.
- The native ThumbnailProvider uses the ImageDecoder class to
resize images in the utility process into bite-sized thumbnails
for display. It is owned by the Java class.
- Thumbnails are retrieved asynchronously and sent back via a
callback in ThumbnailRequest.
- The LRU thumbnail cache is statically shared amongst all
instances of the ThumbnailProviderImpl and is garbage collected
whenever Android deems necessary. It's capped at 5 MB to
prevent the cache size from spiralling out of control.
* DownloadManagerUi now maintains a ThumbnailProviderImpl in its
BackendProvider and destroys it when necessary.
* DownloadHistoryAdapter's ItemViewHolder now implements
ThumbnailRequest, which lets it update any ImageViews that have
placeholders for the thumbnails.
X Tests are still forthcoming. Want to make sure the class looks
like it makes sense before writing them.
BUG=616324
TBR=sky
Committed: https://crrev.com/e5755e4b83bedb55d800a1eed02d0d45aa78458a
Cr-Commit-Position: refs/heads/master@{#415836}
Patch Set 1 #Patch Set 2 : Changed how the image is resized #
Total comments: 31
Patch Set 3 : Comments, FILE thread #Patch Set 4 : Comments #Patch Set 5 : [Download Home] Add image thumbnail support #Patch Set 6 : TODO #
Total comments: 14
Patch Set 7 : Comments, rethreading #Patch Set 8 : Remove an unnecessary thread hop #Patch Set 9 : GYP is gone all of a sudden #Patch Set 10 : Missed the CC files in the GN system #Messages
Total messages: 36 (19 generated)
Description was changed from
==========
[Download Home] Add image thumbnail support
First pass at making a basic thumbnail cache for Download Home.
Still a little janky, but that can be fixed once the skeleton is
in.
* Add a new ThumbnailProvider[Impl] class that produces thumbnails
for files displayed in Download Home.
- The Java ThumbnailProviderImpl maintains a queue of thumbnail
requests to process and a cache of thumbnails that have already
been processed.
- The native ThumbnailProvider uses the ImageDecoder class to
resize images in the utility process into bite-sized thumbnails
for display. It is owned by the Java class.
- Thumbnails are retrieved asynchronously and sent back via a
callback in ThumbnailRequest.
- The LRU thumbnail cache is statically shared amongst all
instances of the ThumbnailProviderImpl and is garbage collected
whenever Android deems necessary. It's capped at 5 MB to
prevent the cache size from spiralling out of control.
* DownloadManagerUi now maintains a ThumbnailProviderImpl in its
BackendProvider and destroys it when necessary.
* DownloadHistoryAdapter's ItemViewHolder now implements
ThumbnailRequest, which lets it update any ImageViews that have
placeholders for the thumbnails.
X Tests are still forthcoming. Want to make sure the class looks
like it makes sense before writing them.
BUG=616324
==========
to
==========
[Download Home] Add image thumbnail support
First pass at making a basic thumbnail cache for Download Home.
Still a little janky, but that can be fixed once the skeleton is
in.
Screenshot: https://drive.google.com/open?id=0B7c8ZkXVwskDMXpnR084ZEpJVVU
* Add a new ThumbnailProvider[Impl] class that produces thumbnails
for files displayed in Download Home.
- The Java ThumbnailProviderImpl maintains a queue of thumbnail
requests to process and a cache of thumbnails that have already
been processed.
- The native ThumbnailProvider uses the ImageDecoder class to
resize images in the utility process into bite-sized thumbnails
for display. It is owned by the Java class.
- Thumbnails are retrieved asynchronously and sent back via a
callback in ThumbnailRequest.
- The LRU thumbnail cache is statically shared amongst all
instances of the ThumbnailProviderImpl and is garbage collected
whenever Android deems necessary. It's capped at 5 MB to
prevent the cache size from spiralling out of control.
* DownloadManagerUi now maintains a ThumbnailProviderImpl in its
BackendProvider and destroys it when necessary.
* DownloadHistoryAdapter's ItemViewHolder now implements
ThumbnailRequest, which lets it update any ImageViews that have
placeholders for the thumbnails.
X Tests are still forthcoming. Want to make sure the class looks
like it makes sense before writing them.
BUG=616324
==========
[email protected] changed reviewers: + [email protected], [email protected], [email protected]
Dominick: You're more comfortable than I am with native asynchronicity; can you take a look and make sure I'm handling the weak refs right? Ian: Can you see if the ThumbnailProvider's cache and design make sense? Didn't switch to a LinkedHashMap yet, but I don't expect too many items to be in the list at any given time. Theresa: Can you make sure everything else is in order?
Description was changed from ========== [Download Home] Add image thumbnail support First pass at making a basic thumbnail cache for Download Home. Still a little janky, but that can be fixed once the skeleton is in. Screenshot: https://drive.google.com/open?id=0B7c8ZkXVwskDMXpnR084ZEpJVVU * Add a new ThumbnailProvider[Impl] class that produces thumbnails for files displayed in Download Home. - The Java ThumbnailProviderImpl maintains a queue of thumbnail requests to process and a cache of thumbnails that have already been processed. - The native ThumbnailProvider uses the ImageDecoder class to resize images in the utility process into bite-sized thumbnails for display. It is owned by the Java class. - Thumbnails are retrieved asynchronously and sent back via a callback in ThumbnailRequest. - The LRU thumbnail cache is statically shared amongst all instances of the ThumbnailProviderImpl and is garbage collected whenever Android deems necessary. It's capped at 5 MB to prevent the cache size from spiralling out of control. * DownloadManagerUi now maintains a ThumbnailProviderImpl in its BackendProvider and destroys it when necessary. * DownloadHistoryAdapter's ItemViewHolder now implements ThumbnailRequest, which lets it update any ImageViews that have placeholders for the thumbnails. X Tests are still forthcoming. Want to make sure the class looks like it makes sense before writing them. BUG=616324 ========== to ========== [Download Home] Add image thumbnail support First pass at making a basic thumbnail cache for Download Home. Still a little janky, but that can be fixed once the skeleton is in. Screenshot that enforces that the biggest size is at least X and then crops out the rest: https://drive.google.com/open?id=0B7c8ZkXVwskDN2FTTmEwOVdWekE * Add a new ThumbnailProvider[Impl] class that produces thumbnails for files displayed in Download Home. - The Java ThumbnailProviderImpl maintains a queue of thumbnail requests to process and a cache of thumbnails that have already been processed. - The native ThumbnailProvider uses the ImageDecoder class to resize images in the utility process into bite-sized thumbnails for display. It is owned by the Java class. - Thumbnails are retrieved asynchronously and sent back via a callback in ThumbnailRequest. - The LRU thumbnail cache is statically shared amongst all instances of the ThumbnailProviderImpl and is garbage collected whenever Android deems necessary. It's capped at 5 MB to prevent the cache size from spiralling out of control. * DownloadManagerUi now maintains a ThumbnailProviderImpl in its BackendProvider and destroys it when necessary. * DownloadHistoryAdapter's ItemViewHolder now implements ThumbnailRequest, which lets it update any ImageViews that have placeholders for the thumbnails. X Tests are still forthcoming. Want to make sure the class looks like it makes sense before writing them. BUG=616324 ==========
[email protected] changed reviewers: + [email protected]
Forgot Dominick was out; adding Ted. You guys were mostly right, aesthetically. Resizing and then cropping looks better. Previous patch: https://drive.google.com/open?id=0B7c8ZkXVwskDMXpnR084ZEpJVVU New patch: https://drive.google.com/open?id=0B7c8ZkXVwskDN2FTTmEwOVdWekE
Tried my best reading the native code and didn't find anything. :P https://codereview.chromium.org/2294983002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2294983002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:238: if (fileType == DownloadFilter.FILTER_IMAGE) { nit: move #236 - #242 to inside of the ItemHolder#getThumbnail()? Item holder can check its own types. https://codereview.chromium.org/2294983002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java (right): https://codereview.chromium.org/2294983002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:70: mRequestQueue.addLast(request); Optional nit: use offer() here because it's a queue? If so, in #92, poll() should be used as well to make them in pair. https://codereview.chromium.org/2294983002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:106: private void onThumbnailRetrieved(String filePath, Bitmap bitmap) { What if the decoding failed and the bitmap is empty or null? Shouldn't we check that either in here or in the item holder? https://codereview.chromium.org/2294983002/diff/20001/chrome/browser/android/... File chrome/browser/android/download/ui/thumbnail_provider.h (right): https://codereview.chromium.org/2294983002/diff/20001/chrome/browser/android/... chrome/browser/android/download/ui/thumbnail_provider.h:14: // The native-side ThumbnailProvider is owned by the Java-side and can be nit: simplify the comments to be "This class is owned by the java-side counterpart"? https://codereview.chromium.org/2294983002/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2294983002/diff/20001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:852: 'browser/android/download/ui/thumbnail_provider.cc', IIRC this is no longer needed. If you don't want to bother the top reviewer, you could safely remove the changes in here?
https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java (right): https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:34: private static WeakReference<LruCache<String, Bitmap>> sBitmapCache = new WeakReference<>(null); i was thinking you could ref count this, but I guess you want to optimize coming back to the download manager quickly I was thinking destroy must be called then we could just keep a static int..but this is fine so let's leave this be. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:107: if (!isInitialized()) return; I'm confused how we can hit this. destroy() is called on the UI thread, which will invalidate the weak ptr natively (which in turn invalidates the java reference). To me, this is inaccessible once destroy is called so this just seems unnecessary/misleading. It makes since in processNextRequest one though since that is async-ly triggered from processQueue. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... File chrome/browser/android/download/ui/thumbnail_provider.cc (right): https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:39: if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)) { let's pull this out to a separate function and make each method be only valid on a particular thread. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:47: // Re-check the file's size because it may have changed. I think "// Ensure the file does not exceed our max size threshold" https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:77: thumbnail = skia::ImageOperations::Resize( it'd be nice to not do this on the UI thread...don't know if that is possible. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:78: decoded_image, this looks wonky to me. I think it should be 4 from the start of the line, but this is probably what git cl format does in c++ and git cl format wins there (vs it's general failing in java land) https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:96: content::BrowserThread::PostTask( ideally the same comment as above and have anyone that is on a different thread (looking at you Start() above) post this manually This makes it a bit too easy to be sloppy with threading imo. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:126: weak_factory_.InvalidateWeakPtrs(); This happens as a result of destructing the WeakPtrFactory, which owns a WeakReferenceOwner, ~WeakReferenceOwner calls Invalidate.
https://codereview.chromium.org/2294983002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java (right): https://codereview.chromium.org/2294983002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:106: private void onThumbnailRetrieved(String filePath, Bitmap bitmap) { On 2016/08/31 00:08:10, Ian Wen wrote: > What if the decoding failed and the bitmap is empty or null? Shouldn't we check > that either in here or in the item holder? I vote for in the item holder, so that we can just leave the default background color/icon set. https://codereview.chromium.org/2294983002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:112: processQueue(); We should be able to send multiple requests at once since the C++ ImageDecoder is running in batch mode as per this CL: https://codereview.chromium.org/931993002/ The code that made the most use of batch mode was in bookmarks and has since been removed, as per this CL (which may still have some clues about how we were using batch mode -- check in bookmark_image_service_android.cc which called into BitmapFetcher which uses the ImageDecoder): https://codereview.chromium.org/1202713002/
[email protected] changed reviewers: - [email protected]
Looks like Ted is perfectly good at pointing out all of my native faults. Moved dominickn@ to the CC list. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:238: if (fileType == DownloadFilter.FILTER_IMAGE) { On 2016/08/31 00:08:10, Ian Wen wrote: > nit: move #236 - #242 to inside of the ItemHolder#getThumbnail()? Item holder > can check its own types. Leaving here as per offline discussion https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java (right): https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:34: private static WeakReference<LruCache<String, Bitmap>> sBitmapCache = new WeakReference<>(null); On 2016/08/31 00:34:34, Ted C wrote: > i was thinking you could ref count this, but I guess you want to optimize coming > back to the download manager quickly > > I was thinking destroy must be called then we could just keep a static int..but > this is fine so let's leave this be. Acknowledged. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:70: mRequestQueue.addLast(request); On 2016/08/31 00:08:10, Ian Wen wrote: > Optional nit: use offer() here because it's a queue? If so, in #92, poll() > should be used as well to make them in pair. Done. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:106: private void onThumbnailRetrieved(String filePath, Bitmap bitmap) { On 2016/08/31 00:08:10, Ian Wen wrote: > What if the decoding failed and the bitmap is empty or null? Shouldn't we check > that either in here or in the item holder? If it's either, then further attempts to decode it will likely fail. Probably better to cache the bad value than keep retrying, I think. I can edit the item holder, though. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:107: if (!isInitialized()) return; On 2016/08/31 00:34:34, Ted C wrote: > I'm confused how we can hit this. destroy() is called on the UI thread, which > will invalidate the weak ptr natively (which in turn invalidates the java > reference). > > To me, this is inaccessible once destroy is called so this just seems > unnecessary/misleading. It makes since in processNextRequest one though since > that is async-ly triggered from processQueue. Yar, it used to be possible but I changed how that was called. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:112: processQueue(); On 2016/08/31 00:37:11, Theresa Wellington wrote: > We should be able to send multiple requests at once since the C++ ImageDecoder > is running in batch mode as per this CL: > https://codereview.chromium.org/931993002/ > > The code that made the most use of batch mode was in bookmarks and has since > been removed, as per this CL (which may still have some clues about how we were > using batch mode -- check in bookmark_image_service_android.cc which called into > BitmapFetcher which uses the ImageDecoder): > https://codereview.chromium.org/1202713002/ I started to look into this, but that'll require redoing how the queue is managed because multiple requests can be made for the same file. Are you cool with a follow up CL? https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... File chrome/browser/android/download/ui/thumbnail_provider.cc (right): https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:39: if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)) { On 2016/08/31 00:34:35, Ted C wrote: > let's pull this out to a separate function and make each method be only valid on > a particular thread. Turns out you can force all of this to be on the FILE thread. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:47: // Re-check the file's size because it may have changed. On 2016/08/31 00:34:34, Ted C wrote: > I think "// Ensure the file does not exceed our max size threshold" Did some variant of this sentence. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:77: thumbnail = skia::ImageOperations::Resize( On 2016/08/31 00:34:35, Ted C wrote: > it'd be nice to not do this on the UI thread...don't know if that is possible. Done. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:78: decoded_image, On 2016/08/31 00:34:34, Ted C wrote: > this looks wonky to me. I think it should be 4 from the start of the line, but > this is probably what git cl format does in c++ and git cl format wins there (vs > it's general failing in java land) Actually ran git cl format this time. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:96: content::BrowserThread::PostTask( On 2016/08/31 00:34:35, Ted C wrote: > ideally the same comment as above and have anyone that is on a different thread > (looking at you Start() above) post this manually > > This makes it a bit too easy to be sloppy with threading imo. Done. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.cc:126: weak_factory_.InvalidateWeakPtrs(); On 2016/08/31 00:34:35, Ted C wrote: > This happens as a result of destructing the WeakPtrFactory, which owns a > WeakReferenceOwner, ~WeakReferenceOwner calls Invalidate. Done. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... File chrome/browser/android/download/ui/thumbnail_provider.h (right): https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/browser/a... chrome/browser/android/download/ui/thumbnail_provider.h:14: // The native-side ThumbnailProvider is owned by the Java-side and can be On 2016/08/31 00:08:10, Ian Wen wrote: > nit: simplify the comments to be "This class is owned by the java-side > counterpart"? Wanted to be explicitly clear about how the ImageRequests are handled inside the CC part of the file. Those are fired to the ImageDecoder and its utility process and manage their own lifecycles, even if this class is destroyed. https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/chrome_br... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/chrome_br... chrome/chrome_browser.gypi:852: 'browser/android/download/ui/thumbnail_provider.cc', On 2016/08/31 00:08:10, Ian Wen wrote: > IIRC this is no longer needed. > > If you don't want to bother the top reviewer, you could safely remove the > changes in here? Eh, might as well do it. We still need the bit below for JNI bindings.
lgtm https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java (right): https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:112: processQueue(); On 2016/08/31 01:35:04, dfalcantara wrote: > On 2016/08/31 00:37:11, Theresa Wellington wrote: > > We should be able to send multiple requests at once since the C++ ImageDecoder > > is running in batch mode as per this CL: > > https://codereview.chromium.org/931993002/ > > > > The code that made the most use of batch mode was in bookmarks and has since > > been removed, as per this CL (which may still have some clues about how we > were > > using batch mode -- check in bookmark_image_service_android.cc which called > into > > BitmapFetcher which uses the ImageDecoder): > > https://codereview.chromium.org/1202713002/ > > I started to look into this, but that'll require redoing how the queue is > managed because multiple requests can be made for the same file. Are you cool > with a follow up CL? Yes, add a TODO?
https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java (right): https://chromiumcodereview.appspot.com/2294983002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:112: processQueue(); On 2016/08/31 17:09:54, Theresa Wellington wrote: > On 2016/08/31 01:35:04, dfalcantara wrote: > > On 2016/08/31 00:37:11, Theresa Wellington wrote: > > > We should be able to send multiple requests at once since the C++ > ImageDecoder > > > is running in batch mode as per this CL: > > > https://codereview.chromium.org/931993002/ > > > > > > The code that made the most use of batch mode was in bookmarks and has since > > > been removed, as per this CL (which may still have some clues about how we > > were > > > using batch mode -- check in bookmark_image_service_android.cc which called > > into > > > BitmapFetcher which uses the ImageDecoder): > > > https://codereview.chromium.org/1202713002/ > > > > I started to look into this, but that'll require redoing how the queue is > > managed because multiple requests can be made for the same file. Are you cool > > with a follow up CL? > > Yes, add a TODO? Done.
lgtm
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2294983002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java (right): https://codereview.chromium.org/2294983002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:54: public ThumbnailProviderImpl(int iconSize) { I would suffix this with Px or Dp depending on what is expected https://codereview.chromium.org/2294983002/diff/100001/chrome/browser/android... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/2294983002/diff/100001/chrome/browser/android... chrome/browser/android/chrome_jni_registrar.cc:357: {"ThumbnailProvider", ThumbnailProvider::RegisterJNI}, Register is more common here. Or Register<FileName>. Or one lone, Register<FileName>Jni... I'd vote for one of the first two. https://codereview.chromium.org/2294983002/diff/100001/chrome/browser/android... File chrome/browser/android/download/ui/thumbnail_provider.cc (right): https://codereview.chromium.org/2294983002/diff/100001/chrome/browser/android... chrome/browser/android/download/ui/thumbnail_provider.cc:88: base::Unretained(this), thumbnail)); same question as below, should you be passing the weak_ptr here instead of a reference to this? Could be a static as well. Could this also just post to ThumbnailProvider::OnThumbnailRetrieved? I think Bind would handle the case where the weak_ptr has gone away. But I guess that depends on whether you need DeleteSoon to be called on the UI thread. https://codereview.chromium.org/2294983002/diff/100001/chrome/browser/android... chrome/browser/android/download/ui/thumbnail_provider.cc:107: int icon_size) icon_size isn't used https://codereview.chromium.org/2294983002/diff/100001/chrome/browser/android... chrome/browser/android/download/ui/thumbnail_provider.cc:134: void ThumbnailProvider::RetrieveThumbnail(JNIEnv* env, What is the longer term plan for this? Should it handle thumbnails for all file types or just images? If the former, then this is good, if the latter I wonder if this should be called ImageThumbnailProvider (vs VideoThumbnailProvider, OfflineThumbnailProvider) https://codereview.chromium.org/2294983002/diff/100001/chrome/browser/android... chrome/browser/android/download/ui/thumbnail_provider.cc:146: base::Unretained(this), path, icon_size)); I think you need to pass the weak_ptr here. You are accessing weak_factory_ in the FILE thread below and it might have been deleted on the UI thread by Destroy().
https://codereview.chromium.org/2294983002/diff/100001/chrome/browser/android... File chrome/browser/android/download/ui/thumbnail_provider.cc (right): https://codereview.chromium.org/2294983002/diff/100001/chrome/browser/android... chrome/browser/android/download/ui/thumbnail_provider.cc:88: base::Unretained(this), thumbnail)); On 2016/08/31 20:14:40, Ted C wrote: > same question as below, should you be passing the weak_ptr here instead of a > reference to this? > > Could be a static as well. > > Could this also just post to ThumbnailProvider::OnThumbnailRetrieved? I think > Bind would handle the case where the weak_ptr has gone away. But I guess that > depends on whether you need DeleteSoon to be called on the UI thread. IIRC, weak_ptr is only better than Unretained() if we are posting on to the same thread. If on the same thread, weak_ptr will cancel the task itself, rather than only cancelling the callback. Yet since this is a multi-threaded case, Unretained(this) should be okay, as they both only cancel the callback, should "this" is deleted? https://www.chromium.org/developers/design-documents/threading#base_WeakPtr_a...
Well, threading is always a fun nightmare. PTAL. I DCHECKED everything to make sure that things are run on only the correct threads, and made sure that the ThumbnailProvider is mangled only on the UI thread, ensuring that its weak pointer stays valid and removing the need to PostTasks there. PTAL https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java (right): https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/download/ui/ThumbnailProviderImpl.java:54: public ThumbnailProviderImpl(int iconSize) { On 2016/08/31 20:14:40, Ted C wrote: > I would suffix this with Px or Dp depending on what is expected Done. https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/browser/... File chrome/browser/android/chrome_jni_registrar.cc (right): https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/browser/... chrome/browser/android/chrome_jni_registrar.cc:357: {"ThumbnailProvider", ThumbnailProvider::RegisterJNI}, On 2016/08/31 20:14:40, Ted C wrote: > Register is more common here. Or Register<FileName>. > > Or one lone, Register<FileName>Jni... > > I'd vote for one of the first two. Done. https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/browser/... chrome/browser/android/chrome_jni_registrar.cc:357: {"ThumbnailProvider", ThumbnailProvider::RegisterJNI}, On 2016/08/31 20:14:40, Ted C wrote: > Register is more common here. Or Register<FileName>. > > Or one lone, Register<FileName>Jni... > > I'd vote for one of the first two. Done, but we have a bunch of terrible and inconsistently-used conventions: Static register function on the class itself, that doesn't indicate what it's registering for: WebApkUpdateManager::Register Static register function on the class itself, which names itself in case you forgot what you were registering: SQLiteCursor::RegisterSqliteCursor Register function that is outside of the class but still defined in the header: ReigsterSSLClientCertificateRequestAndroid https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/browser/... File chrome/browser/android/download/ui/thumbnail_provider.cc (right): https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/browser/... chrome/browser/android/download/ui/thumbnail_provider.cc:88: base::Unretained(this), thumbnail)); On 2016/08/31 20:43:30, Ian Wen wrote: > On 2016/08/31 20:14:40, Ted C wrote: > > same question as below, should you be passing the weak_ptr here instead of a > > reference to this? > > > > Could be a static as well. > > > > Could this also just post to ThumbnailProvider::OnThumbnailRetrieved? I think > > Bind would handle the case where the weak_ptr has gone away. But I guess that > > depends on whether you need DeleteSoon to be called on the UI thread. > > IIRC, weak_ptr is only better than Unretained() if we are posting on to the same > thread. If on the same thread, weak_ptr will cancel the task itself, rather than > only cancelling the callback. > > Yet since this is a multi-threaded case, Unretained(this) should be okay, as > they both only cancel the callback, should "this" is deleted? > > https://www.chromium.org/developers/design-documents/threading#base_WeakPtr_a... This should be fine because the ImageThumbnailRequest in the current patch continues to handle its own deletion. https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/browser/... chrome/browser/android/download/ui/thumbnail_provider.cc:107: int icon_size) On 2016/08/31 20:14:41, Ted C wrote: > icon_size isn't used Whoops. Removed. https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/browser/... chrome/browser/android/download/ui/thumbnail_provider.cc:134: void ThumbnailProvider::RetrieveThumbnail(JNIEnv* env, On 2016/08/31 20:14:41, Ted C wrote: > What is the longer term plan for this? Should it handle thumbnails for all file > types or just images? If the former, then this is good, if the latter I wonder > if this should be called ImageThumbnailProvider (vs VideoThumbnailProvider, > OfflineThumbnailProvider) I'm planning on having this class delegate to different retrieval classes. Main reason why there's a separate ImageThumbnailRequest class above. https://chromiumcodereview.appspot.com/2294983002/diff/100001/chrome/browser/... chrome/browser/android/download/ui/thumbnail_provider.cc:146: base::Unretained(this), path, icon_size)); On 2016/08/31 20:14:40, Ted C wrote: > I think you need to pass the weak_ptr here. You are accessing weak_factory_ in > the FILE thread below and it might have been deleted on the UI thread by > Destroy(). Blarghhh. Ian and I walked through the wonkiness here and found a way to keep the ThumbnailProvider entirely on the UI thread while keeping the ImageThumbnailRequest on the FILE thread as much as possible.
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [Download Home] Add image thumbnail support First pass at making a basic thumbnail cache for Download Home. Still a little janky, but that can be fixed once the skeleton is in. Screenshot that enforces that the biggest size is at least X and then crops out the rest: https://drive.google.com/open?id=0B7c8ZkXVwskDN2FTTmEwOVdWekE * Add a new ThumbnailProvider[Impl] class that produces thumbnails for files displayed in Download Home. - The Java ThumbnailProviderImpl maintains a queue of thumbnail requests to process and a cache of thumbnails that have already been processed. - The native ThumbnailProvider uses the ImageDecoder class to resize images in the utility process into bite-sized thumbnails for display. It is owned by the Java class. - Thumbnails are retrieved asynchronously and sent back via a callback in ThumbnailRequest. - The LRU thumbnail cache is statically shared amongst all instances of the ThumbnailProviderImpl and is garbage collected whenever Android deems necessary. It's capped at 5 MB to prevent the cache size from spiralling out of control. * DownloadManagerUi now maintains a ThumbnailProviderImpl in its BackendProvider and destroys it when necessary. * DownloadHistoryAdapter's ItemViewHolder now implements ThumbnailRequest, which lets it update any ImageViews that have placeholders for the thumbnails. X Tests are still forthcoming. Want to make sure the class looks like it makes sense before writing them. BUG=616324 ========== to ========== [Download Home] Add image thumbnail support First pass at making a basic thumbnail cache for Download Home. Still a little janky, but that can be fixed once the skeleton is in. Screenshot that enforces that the biggest size is at least X and then crops out the rest: https://drive.google.com/open?id=0B7c8ZkXVwskDN2FTTmEwOVdWekE * Add a new ThumbnailProvider[Impl] class that produces thumbnails for files displayed in Download Home. - The Java ThumbnailProviderImpl maintains a queue of thumbnail requests to process and a cache of thumbnails that have already been processed. - The native ThumbnailProvider uses the ImageDecoder class to resize images in the utility process into bite-sized thumbnails for display. It is owned by the Java class. - Thumbnails are retrieved asynchronously and sent back via a callback in ThumbnailRequest. - The LRU thumbnail cache is statically shared amongst all instances of the ThumbnailProviderImpl and is garbage collected whenever Android deems necessary. It's capped at 5 MB to prevent the cache size from spiralling out of control. * DownloadManagerUi now maintains a ThumbnailProviderImpl in its BackendProvider and destroys it when necessary. * DownloadHistoryAdapter's ItemViewHolder now implements ThumbnailRequest, which lets it update any ImageViews that have placeholders for the thumbnails. X Tests are still forthcoming. Want to make sure the class looks like it makes sense before writing them. BUG=616324 TBR=sky ==========
[email protected] changed reviewers: + [email protected]
TBRing +sky: I had to add ThumbnailProviderImpl.java to BUILD.gn for JNI bindings.
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected], [email protected] Link to the patchset: https://chromiumcodereview.appspot.com/2294983002/#ps180001 (title: "Missed the CC files in the GN system")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Download Home] Add image thumbnail support First pass at making a basic thumbnail cache for Download Home. Still a little janky, but that can be fixed once the skeleton is in. Screenshot that enforces that the biggest size is at least X and then crops out the rest: https://drive.google.com/open?id=0B7c8ZkXVwskDN2FTTmEwOVdWekE * Add a new ThumbnailProvider[Impl] class that produces thumbnails for files displayed in Download Home. - The Java ThumbnailProviderImpl maintains a queue of thumbnail requests to process and a cache of thumbnails that have already been processed. - The native ThumbnailProvider uses the ImageDecoder class to resize images in the utility process into bite-sized thumbnails for display. It is owned by the Java class. - Thumbnails are retrieved asynchronously and sent back via a callback in ThumbnailRequest. - The LRU thumbnail cache is statically shared amongst all instances of the ThumbnailProviderImpl and is garbage collected whenever Android deems necessary. It's capped at 5 MB to prevent the cache size from spiralling out of control. * DownloadManagerUi now maintains a ThumbnailProviderImpl in its BackendProvider and destroys it when necessary. * DownloadHistoryAdapter's ItemViewHolder now implements ThumbnailRequest, which lets it update any ImageViews that have placeholders for the thumbnails. X Tests are still forthcoming. Want to make sure the class looks like it makes sense before writing them. BUG=616324 TBR=sky ========== to ========== [Download Home] Add image thumbnail support First pass at making a basic thumbnail cache for Download Home. Still a little janky, but that can be fixed once the skeleton is in. Screenshot that enforces that the biggest size is at least X and then crops out the rest: https://drive.google.com/open?id=0B7c8ZkXVwskDN2FTTmEwOVdWekE * Add a new ThumbnailProvider[Impl] class that produces thumbnails for files displayed in Download Home. - The Java ThumbnailProviderImpl maintains a queue of thumbnail requests to process and a cache of thumbnails that have already been processed. - The native ThumbnailProvider uses the ImageDecoder class to resize images in the utility process into bite-sized thumbnails for display. It is owned by the Java class. - Thumbnails are retrieved asynchronously and sent back via a callback in ThumbnailRequest. - The LRU thumbnail cache is statically shared amongst all instances of the ThumbnailProviderImpl and is garbage collected whenever Android deems necessary. It's capped at 5 MB to prevent the cache size from spiralling out of control. * DownloadManagerUi now maintains a ThumbnailProviderImpl in its BackendProvider and destroys it when necessary. * DownloadHistoryAdapter's ItemViewHolder now implements ThumbnailRequest, which lets it update any ImageViews that have placeholders for the thumbnails. X Tests are still forthcoming. Want to make sure the class looks like it makes sense before writing them. BUG=616324 TBR=sky ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Download Home] Add image thumbnail support First pass at making a basic thumbnail cache for Download Home. Still a little janky, but that can be fixed once the skeleton is in. Screenshot that enforces that the biggest size is at least X and then crops out the rest: https://drive.google.com/open?id=0B7c8ZkXVwskDN2FTTmEwOVdWekE * Add a new ThumbnailProvider[Impl] class that produces thumbnails for files displayed in Download Home. - The Java ThumbnailProviderImpl maintains a queue of thumbnail requests to process and a cache of thumbnails that have already been processed. - The native ThumbnailProvider uses the ImageDecoder class to resize images in the utility process into bite-sized thumbnails for display. It is owned by the Java class. - Thumbnails are retrieved asynchronously and sent back via a callback in ThumbnailRequest. - The LRU thumbnail cache is statically shared amongst all instances of the ThumbnailProviderImpl and is garbage collected whenever Android deems necessary. It's capped at 5 MB to prevent the cache size from spiralling out of control. * DownloadManagerUi now maintains a ThumbnailProviderImpl in its BackendProvider and destroys it when necessary. * DownloadHistoryAdapter's ItemViewHolder now implements ThumbnailRequest, which lets it update any ImageViews that have placeholders for the thumbnails. X Tests are still forthcoming. Want to make sure the class looks like it makes sense before writing them. BUG=616324 TBR=sky ========== to ========== [Download Home] Add image thumbnail support First pass at making a basic thumbnail cache for Download Home. Still a little janky, but that can be fixed once the skeleton is in. Screenshot that enforces that the biggest size is at least X and then crops out the rest: https://drive.google.com/open?id=0B7c8ZkXVwskDN2FTTmEwOVdWekE * Add a new ThumbnailProvider[Impl] class that produces thumbnails for files displayed in Download Home. - The Java ThumbnailProviderImpl maintains a queue of thumbnail requests to process and a cache of thumbnails that have already been processed. - The native ThumbnailProvider uses the ImageDecoder class to resize images in the utility process into bite-sized thumbnails for display. It is owned by the Java class. - Thumbnails are retrieved asynchronously and sent back via a callback in ThumbnailRequest. - The LRU thumbnail cache is statically shared amongst all instances of the ThumbnailProviderImpl and is garbage collected whenever Android deems necessary. It's capped at 5 MB to prevent the cache size from spiralling out of control. * DownloadManagerUi now maintains a ThumbnailProviderImpl in its BackendProvider and destroys it when necessary. * DownloadHistoryAdapter's ItemViewHolder now implements ThumbnailRequest, which lets it update any ImageViews that have placeholders for the thumbnails. X Tests are still forthcoming. Want to make sure the class looks like it makes sense before writing them. BUG=616324 TBR=sky Committed: https://crrev.com/e5755e4b83bedb55d800a1eed02d0d45aa78458a Cr-Commit-Position: refs/heads/master@{#415836} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e5755e4b83bedb55d800a1eed02d0d45aa78458a Cr-Commit-Position: refs/heads/master@{#415836} |
