Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
916783003:
Restrict salient image size before storing (Closed)
DescriptionRestrict salient image size before storing
It is a waste of storage to save images that are larger than device
display size. On tablet we should be even more aggressive about resizing
those images, because showing them won't take up the entire screen.
BUG=454623
Committed: https://crrev.com/f7b6773a958ef2cc742174a4dc9c1ab60e746bca
Cr-Commit-Position: refs/heads/master@{#317628}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 5
Patch Set 4 : use pixel instead of dp #
Total comments: 8
Patch Set 5 : Switch to dynamic size determination #
Total comments: 4
Patch Set 6 : Fix comments #
Total comments: 1
Messages
Total messages: 20 (4 generated)
ianwen@chromium.org changed reviewers: + kkimlabs@chromium.org, noyau@chromium.org
noyau, could you please take a look at the changes in bookmark_image_service.h? Thanks!
ianwen@chromium.org changed reviewers: + twellington@chromium.org
https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc (right): https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:264: base::Callback<gfx::Image(void)> task = Could you move this code in the superclass? With a virtual implementation for resizeImage? This would make it easier for us to implement the same thing in iOS.
Noyau@, I made ResizeImage a pure virtual method in bookmark image service and hope you like this change better. :) kkimlabs@, could you ptal at bookmark_image_service_android? https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc (right): https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:264: base::Callback<gfx::Image(void)> task = On 2015/02/12 08:34:42, noyau wrote: > Could you move this code in the superclass? With a virtual implementation for > resizeImage? > > This would make it easier for us to implement the same thing in iOS. Done.
https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc (right): https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:52: max_length /= display_info.GetDIPScale(); IIRC, gfx::Image's DPI concept doesn't depend on the running device setting automatically, and in our case I think it's set to be "no scale", at gfx::ImageSkia::CreateFrom1xBitmap() and no DPI information is added. Can you double check? I compared bitmap->width() inside | BookmarkImageServiceAndroid::BitmapFetcherHandler::OnFetchComplet| and image.Width() inside |BookmarkImageServiceAndroid::ResizeImage| and it was the same. https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h (right): https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h:62: scoped_ptr<gfx::Size> max_size_; looks like this doesn't have to be scoped_ptr? (Also since ios will be doing it too, I think this can just go BookmarkImageService. But maybe in a follow-up ios CL) https://codereview.chromium.org/916783003/diff/40001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc (right): https://codereview.chromium.org/916783003/diff/40001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:215: gfx::Image resized_uimage = image; Q: what does |uimage| stand for? https://codereview.chromium.org/916783003/diff/40001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:225: int dest_height = static_cast<int>(resize_ratio * image.Height() + 0.5f); Eventually, I think it's good to share computing dest_* size logic and the above max_size_ logic with ios by putting at BookmarkImageService. I'm OK with leaving here for now since there will be a follow up ios CL. https://codereview.chromium.org/916783003/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_image_service.cc (right): https://codereview.chromium.org/916783003/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_image_service.cc:197: &BookmarkImageService::ResizeImage, base::Unretained(this), image); How about just resizing inside BookmarkImageService::StoreImage then we don't need to post another task or make |ProcessNewImageInternal|. (need to change |StoreImage| function name if we do that.)
https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc (right): https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:52: max_length /= display_info.GetDIPScale(); On 2015/02/12 23:24:15, Kibeom Kim wrote: > IIRC, gfx::Image's DPI concept doesn't depend on the running device setting > automatically, and in our case I think it's set to be "no scale", at > gfx::ImageSkia::CreateFrom1xBitmap() and no DPI information is added. Can you > double check? > > I compared bitmap->width() inside | > BookmarkImageServiceAndroid::BitmapFetcherHandler::OnFetchComplet| and > image.Width() inside |BookmarkImageServiceAndroid::ResizeImage| and it was the > same. Yes you were right. The size of the dialog is always that large, which tricked me into believing it represents the size of the image correctly. Done. https://codereview.chromium.org/916783003/diff/40001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc (right): https://codereview.chromium.org/916783003/diff/40001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:215: gfx::Image resized_uimage = image; On 2015/02/12 23:24:15, Kibeom Kim wrote: > Q: what does |uimage| stand for? Typo. Done. https://codereview.chromium.org/916783003/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_image_service.cc (right): https://codereview.chromium.org/916783003/diff/40001/components/enhanced_book... components/enhanced_bookmarks/bookmark_image_service.cc:197: &BookmarkImageService::ResizeImage, base::Unretained(this), image); On 2015/02/12 23:24:15, Kibeom Kim wrote: > How about just resizing inside BookmarkImageService::StoreImage then we don't > need to post another task or make |ProcessNewImageInternal|. (need to change > |StoreImage| function name if we do that.) Done.
maybe forgot to upload the next patchset? https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h (right): https://codereview.chromium.org/916783003/diff/20001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h:62: scoped_ptr<gfx::Size> max_size_; On 2015/02/12 23:24:15, Kibeom Kim wrote: > looks like this doesn't have to be scoped_ptr? (Also since ios will be doing it > too, I think this can just go BookmarkImageService. But maybe in a follow-up ios > CL) ping?
https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc (right): https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:54: max_length = std::min(max_length, int(500 * display_info.GetDIPScale())); The comment says 600, the code uses 500. Is the same condition used to change the UI to 3/4 of the size on tablet? Should this condition be extracted in some common code so the image is never reduced to something too small the day they make a giant phone even more giant that the ones we have now? https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:215: if (max_size_->width() > 0 && max_size_->height() > 0 && You've already done a DCHECK for this condition in the constructor, no need to test it again. https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:217: image.Height() > max_size_->height())) { If you resize an image that is really tall, or an image that is really thin, you will end up with an unexploitable image after the resize. This condition should probably be a &&, to keep images that already have a dimension that is too small as is. https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h (right): https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h:58: scoped_ptr<gfx::Size> max_size_; Need comment. And why is this a scoped ptr and not just a gfx::Size?
https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc (right): https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:54: max_length = std::min(max_length, int(500 * display_info.GetDIPScale())); On 2015/02/13 09:57:42, noyau wrote: > The comment says 600, the code uses 500. > > Is the same condition used to change the UI to 3/4 of the size on tablet? Should > this condition be extracted in some common code so the image is never reduced to > something too small the day they make a giant phone even more giant that the > ones we have now? 500 was used intentionally, because I wanted to make resizing to be more aggressive, given that large images might cause janky scrolling for grid view, and most users won't go to the detail page anyway. In the next patch, I changed the logic to ratio cutoff, instead of a fixed value. And depending on tablet or on phone, it adopts different sizes. (Android has a large number of different devices...) https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:215: if (max_size_->width() > 0 && max_size_->height() > 0 && On 2015/02/13 09:57:42, noyau wrote: > You've already done a DCHECK for this condition in the constructor, no need to > test it again. Done. https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:217: image.Height() > max_size_->height())) { On 2015/02/13 09:57:42, noyau wrote: > If you resize an image that is really tall, or an image that is really thin, you > will end up with an unexploitable image after the resize. This condition should > probably be a &&, to keep images that already have a dimension that is too small > as is. Done. https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h (right): https://codereview.chromium.org/916783003/diff/60001/chrome/browser/enhanced_... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.h:58: scoped_ptr<gfx::Size> max_size_; On 2015/02/13 09:57:42, noyau wrote: > Need comment. And why is this a scoped ptr and not just a gfx::Size? Changed to gfx::Size.
lgtm
lgtm with nits https://codereview.chromium.org/916783003/diff/80001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_image_service.cc (right): https://codereview.chromium.org/916783003/diff/80001/components/enhanced_book... components/enhanced_bookmarks/bookmark_image_service.cc:217: const GURL& page_url) { How about putting DCHECK(!CalledOnValidThread()); here? https://codereview.chromium.org/916783003/diff/80001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_image_service.h (right): https://codereview.chromium.org/916783003/diff/80001/components/enhanced_book... components/enhanced_bookmarks/bookmark_image_service.h:130: // background sequenced worker pool for this object. nit: update comment to explain resize? Or I think just mentioning the above |ResizeImage| function is fine.
https://codereview.chromium.org/916783003/diff/80001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_image_service.cc (right): https://codereview.chromium.org/916783003/diff/80001/components/enhanced_book... components/enhanced_bookmarks/bookmark_image_service.cc:217: const GURL& page_url) { On 2015/02/17 21:32:59, Kibeom Kim wrote: > How about putting DCHECK(!CalledOnValidThread()); here? I think the convention here is that if a function does not contain this DCHECK, it is not supposed to run on UI thread. Adding an opposite DCHECK might be a bit confusing, I even did not notice the exclamation mark in your comment at first glance, which confused me for a while... https://codereview.chromium.org/916783003/diff/80001/components/enhanced_book... File components/enhanced_bookmarks/bookmark_image_service.h (right): https://codereview.chromium.org/916783003/diff/80001/components/enhanced_book... components/enhanced_bookmarks/bookmark_image_service.h:130: // background sequenced worker pool for this object. On 2015/02/17 21:32:59, Kibeom Kim wrote: > nit: update comment to explain resize? Or I think just mentioning the above > |ResizeImage| function is fine. Done.
New patchsets have been uploaded after l-g-t-m from noyau@chromium.org,kkimlabs@chromium.org
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/916783003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f7b6773a958ef2cc742174a4dc9c1ab60e746bca Cr-Commit-Position: refs/heads/master@{#317628}
Message was sent while issue was closed.
https://codereview.chromium.org/916783003/diff/100001/chrome/browser/enhanced... File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc (right): https://codereview.chromium.org/916783003/diff/100001/chrome/browser/enhanced... chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:221: ((float)max_size_.height()) / image.Height()); This should be std::max, not std::min. What you are doing here is resizing the image so its larger size fits into the max_size rect. What you want is the smaller size to match, hence the need to pick the larger ratio. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
