Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3224)

Issue 2715153006: Set desired_image_size when decoding images for NTP Tile icons. (Closed)

Created:
3 years, 5 months ago by tschumann
Modified:
3 years, 5 months ago
CC:
Aaron Boodman, abarth-chromium, blundell+watchlist_chromium.org, chromium-reviews, darin (slow to review), davemoore+watch_chromium.org, David Black, donnd+watch_chromium.org, droger+watchlist_chromium.org, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, Jered, jfweitz+watch_chromium.org, kmadhusu+watch_chromium.org, lhchavez+watch_chromium.org, melevin+watch_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, oshima+watch_chromium.org, qsr+mojo_chromium.org, samarth+watch_chromium.org, sdefresne+watchlist_chromium.org, skanuj+watch_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Specify desired_image_size when decoding image files for the NTP Tile icons. Currently, the size (0,0) is hard-coded which makes the decoder prefer small images in case multiple sizes are available -- e.g. in ICO files. However, this resolution is too low for NTP tiles and hence these tiles render as "scrabble" tiles. I looked at popular sites sets of 8 countries (especially EM countries), and the number of nicely rendered icons increased from 51 to 57 (out of 64). Here are the details: https://docs.google.com/document/d/1eNCDcsVkBEzG2hUm8uPh9RgMRpM3NvS9C0VhO_gpCJM Unfortunately, this change requires drilling through a number of layers. ImageFetcher (components/image_fetcher/image_fetcher_impl.h) is used by 6 different clients which is why I added a setter method on that class -- also because we already do this with other parameters. Review-Url: https://codereview.chromium.org/2715153006 Cr-Commit-Position: refs/heads/master@{#454341} Committed: https://chromium.googlesource.com/chromium/src/+/30ee1086df7c132ecafae15534946181b03b315c

Patch Set 1 #

Patch Set 2 : add mojo dependencies #

Patch Set 3 : ax #

Total comments: 8

Patch Set 4 : comments from treib@ and got comments in sync #

Patch Set 5 : rebase #

Patch Set 6 : ran git cl format as suggested #

Total comments: 8

Patch Set 7 : mathp comments #

Patch Set 8 : fix dependencies #

Total comments: 2

Patch Set 9 : comments + chromeos/ios fixes #

Patch Set 10 : ios test fixes #

Total comments: 2

Patch Set 11 : comment formatting #

Unified diffs Side-by-side diffs Delta from patch set Stats (+147 lines, -52 lines) Patch
M chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/image_decoder.h View 1 2 3 4 5 6 3 chunks +13 lines, -2 lines 0 comments Download
M chrome/browser/image_decoder.cc View 1 2 3 4 5 6 7 chunks +20 lines, -12 lines 0 comments Download
M chrome/browser/image_decoder_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +13 lines, -13 lines 0 comments Download
M chrome/browser/search/suggestions/image_decoder_impl.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/search/suggestions/image_decoder_impl.cc View 2 chunks +7 lines, -1 line 0 comments Download
M components/image_fetcher/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M components/image_fetcher/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M components/image_fetcher/image_decoder.h View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -0 lines 0 comments Download
M components/image_fetcher/image_fetcher.h View 1 2 3 4 5 6 7 8 2 chunks +9 lines, -0 lines 0 comments Download
M components/image_fetcher/image_fetcher_impl.h View 1 2 3 4 5 6 7 8 3 chunks +5 lines, -1 line 0 comments Download
M components/image_fetcher/image_fetcher_impl.cc View 1 2 3 4 5 2 chunks +7 lines, -4 lines 0 comments Download
M components/ntp_snippets/remote/remote_suggestions_provider_impl.cc View 1 2 3 4 2 chunks +6 lines, -2 lines 0 comments Download
M components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc View 1 2 3 4 3 chunks +3 lines, -0 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl_unittest.cc View 1 2 3 4 5 6 7 5 chunks +5 lines, -0 lines 0 comments Download
M components/suggestions/image_manager_unittest.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M ios/chrome/browser/suggestions/ios_image_decoder_impl.mm View 1 2 3 4 5 6 7 8 9 3 chunks +5 lines, -0 lines 0 comments Download
M ios/chrome/browser/suggestions/ios_image_decoder_impl_unittest.mm View 1 2 3 4 5 6 7 8 9 3 chunks +7 lines, -4 lines 0 comments Download
M services/image_decoder/image_decoder_impl.h View 1 2 3 4 5 2 chunks +7 lines, -6 lines 0 comments Download
M services/image_decoder/image_decoder_impl.cc View 1 2 3 4 5 3 chunks +2 lines, -2 lines 0 comments Download
M services/image_decoder/image_decoder_impl_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M services/image_decoder/public/cpp/decode.h View 1 2 3 4 5 6 7 8 2 chunks +13 lines, -3 lines 0 comments Download
M services/image_decoder/public/cpp/decode.cc View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M services/image_decoder/public/interfaces/BUILD.gn View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M services/image_decoder/public/interfaces/image_decoder.mojom View 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 71 (47 generated)
tschumann
Not sure what's best to expose this parameter. Given in how many cases the fetcher ...
3 years, 5 months ago (2017-02-28 20:16:09 UTC) #3
Luis Héctor Chávez
chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc lgtm
3 years, 5 months ago (2017-02-28 20:25:40 UTC) #6
Marc Treib
Code mostly looks good; one high-level comment below. https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/image_decoder.cc File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/image_decoder.cc#newcode142 chrome/browser/image_decoder.cc:142: const ...
3 years, 5 months ago (2017-03-01 09:17:26 UTC) #12
tschumann
https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/image_decoder.cc File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/image_decoder.cc#newcode142 chrome/browser/image_decoder.cc:142: const gfx::Size& desired_image_frame_size) { On 2017/03/01 09:17:26, Marc Treib ...
3 years, 5 months ago (2017-03-01 09:37:43 UTC) #13
Marc Treib
lgtm https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/suggestions/image_decoder_impl.h File chrome/browser/search/suggestions/image_decoder_impl.h (right): https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/suggestions/image_decoder_impl.h#newcode25 chrome/browser/search/suggestions/image_decoder_impl.h:25: const image_fetcher::ImageDecodedCallback& callback) override; On 2017/03/01 09:37:43, tschumann ...
3 years, 5 months ago (2017-03-01 09:41:56 UTC) #14
tschumann
(if I understood pre-submit checks correctly): +thakis for chrome/browser/image_decoder +vollick for components/image_fetcher/DEPS https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/suggestions/image_decoder_impl.h File chrome/browser/search/suggestions/image_decoder_impl.h ...
3 years, 5 months ago (2017-03-01 11:36:36 UTC) #16
Mathieu
Main comment about documenting what gfx::Size() will mean and when it should be used. Also, ...
3 years, 5 months ago (2017-03-01 14:36:23 UTC) #18
Ken Rockot(use gerrit already)
LGTM - maybe someday soon we should just delete chrome/browser/image_decoder* and update callers to use ...
3 years, 5 months ago (2017-03-01 17:46:18 UTC) #19
tschumann
On 2017/03/01 17:46:18, Ken Rockot wrote: > LGTM - maybe someday soon we should just ...
3 years, 5 months ago (2017-03-01 19:14:48 UTC) #20
Ken Rockot(use gerrit already)
FWIW the image_decoder client library came last, and I intentionally left the old ImageDecoder API ...
3 years, 5 months ago (2017-03-01 19:21:33 UTC) #21
tschumann
mathp, thanks for the careful review! Concerning testing (which I'm a big fan of): I ...
3 years, 5 months ago (2017-03-01 19:40:08 UTC) #22
Mathieu
Thanks, lgtm https://codereview.chromium.org/2715153006/diff/160001/components/image_fetcher/image_decoder.h File components/image_fetcher/image_decoder.h (right): https://codereview.chromium.org/2715153006/diff/160001/components/image_fetcher/image_decoder.h#newcode33 components/image_fetcher/image_decoder.h:33: // one in larger size if there's ...
3 years, 5 months ago (2017-03-01 20:24:30 UTC) #31
tschumann
https://codereview.chromium.org/2715153006/diff/160001/components/image_fetcher/image_decoder.h File components/image_fetcher/image_decoder.h (right): https://codereview.chromium.org/2715153006/diff/160001/components/image_fetcher/image_decoder.h#newcode33 components/image_fetcher/image_decoder.h:33: // one in larger size if there's no precise ...
3 years, 5 months ago (2017-03-01 20:55:06 UTC) #32
tschumann
+achuith@ for ChromeOS
3 years, 5 months ago (2017-03-01 21:15:52 UTC) #41
Mathieu
lgtm ios/chrome/browser/suggestions
3 years, 5 months ago (2017-03-01 21:54:55 UTC) #48
achuithb
only looked at hats_notification_controller_unittest.cc. lgtm
3 years, 5 months ago (2017-03-02 11:00:46 UTC) #51
tschumann
Nico, could you have a look at chrome/browser/image_decoder* ? Thanks!
3 years, 5 months ago (2017-03-02 11:03:50 UTC) #53
tschumann
Robert, can you approve for services/image_decoder/public/interfaces/image_decoder.mojom ? Thanks!
3 years, 5 months ago (2017-03-02 11:10:44 UTC) #55
Nico
My files lgtm, one optional nit below. https://codereview.chromium.org/2715153006/diff/210001/chrome/browser/image_decoder_browsertest.cc File chrome/browser/image_decoder_browsertest.cc (right): https://codereview.chromium.org/2715153006/diff/210001/chrome/browser/image_decoder_browsertest.cc#newcode192 chrome/browser/image_decoder_browsertest.cc:192: false /* ...
3 years, 5 months ago (2017-03-02 14:17:11 UTC) #56
Robert Sesek
mojom lgtm Also, consider wrapping your CL description to 80 columns, to better display in ...
3 years, 5 months ago (2017-03-02 15:07:27 UTC) #61
tschumann
https://codereview.chromium.org/2715153006/diff/210001/chrome/browser/image_decoder_browsertest.cc File chrome/browser/image_decoder_browsertest.cc (right): https://codereview.chromium.org/2715153006/diff/210001/chrome/browser/image_decoder_browsertest.cc#newcode192 chrome/browser/image_decoder_browsertest.cc:192: false /* shrink_to_fit */, gfx::Size() /* desired_image_frame_size */); On ...
3 years, 5 months ago (2017-03-02 16:25:52 UTC) #63
tschumann
On 2017/03/02 15:07:27, Robert Sesek wrote: > mojom lgtm > > Also, consider wrapping your ...
3 years, 5 months ago (2017-03-02 16:26:34 UTC) #64
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715153006/230001
3 years, 5 months ago (2017-03-02 18:12:16 UTC) #68
commit-bot: I haz the power
3 years, 5 months ago (2017-03-02 19:38:44 UTC) #71
Message was sent while issue was closed.
Committed patchset #11 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/30ee1086df7c132ecafae1553494...

Powered by Google App Engine
This is Rietveld 408576698