Set desired_image_size when decoding images for NTP Tile icons.

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

[tschumann, 2017-02-28 20:13:53]

Description was changed from

==========
linter


hitting mojo issue

BUG=
==========

to

==========
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.


BUG=690029
==========

[tschumann, 2017-02-28 20:13:53]

tschumann@chromium.org changed reviewers:
+ lhchavez@lhchavez.com, markusheintz@google.com, mathp@google.com,
rockot@chromium.org, treib@chromium.org

[tschumann, 2017-02-28 20:16:09]

Not sure what's best to expose this parameter.
Given in how many cases the fetcher is used, I'm not too comfortable just
changing the hard-coded value.
Let me know what you think.

Thanks!

--Tim

[tschumann, 2017-02-28 20:18:29]

tschumann@chromium.org changed reviewers:
+ dcheng@chromium.org, mkwst@chromium.org

[Luis Héctor Chávez, 2017-02-28 20:25:40]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Luis Héctor Chávez, 2017-02-28 20:25:40]

chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc lgtm

[tschumann, 2017-02-28 23:25:29]

The CQ bit was checked by tschumann@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 23:25:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 23:29:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 23:30:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2017-02-28 23:30:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Marc Treib, 2017-03-01 09:17:26]

Code mostly looks good; one high-level comment below.

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:142: const gfx::Size& desired_image_frame_size)
{
nit: Is this "git cl format"ted? I think each param should go on its own line.

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/s...
File chrome/browser/search/suggestions/image_decoder_impl.h (right):

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/s...
chrome/browser/search/suggestions/image_decoder_impl.h:25: const
image_fetcher::ImageDecodedCallback& callback) override;
Hm, one high-level comment: This returns a gfx::Image, which has support for
multiple sizes. If that's possible, it might be nicer (API-wise) to simply
decode all of them. WDYT?

https://codereview.chromium.org/2715153006/diff/40001/components/image_fetche...
File components/image_fetcher/image_fetcher_impl.cc (right):

https://codereview.chromium.org/2715153006/diff/40001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.cc:45: void
ImageFetcherImpl::StartOrQueueNetworkRequest(
nit: empty line before

[tschumann, 2017-03-01 09:37:43]

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:142: const gfx::Size& desired_image_frame_size)
{
On 2017/03/01 09:17:26, Marc Treib wrote:
> nit: Is this "git cl format"ted? I think each param should go on its own line.

reformatted.

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/s...
File chrome/browser/search/suggestions/image_decoder_impl.h (right):

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/s...
chrome/browser/search/suggestions/image_decoder_impl.h:25: const
image_fetcher::ImageDecodedCallback& callback) override;
On 2017/03/01 09:17:26, Marc Treib wrote:
> Hm, one high-level comment: This returns a gfx::Image, which has support for
> multiple sizes. If that's possible, it might be nicer (API-wise) to simply
> decode all of them. WDYT?

I totally agree -- just that's how the whole image decoding stack works :-)

If there's a place where we need multiple versions of the image, we should
follow up. Right now, I don't have a clear use case. Plus, that's a much larger
change. 

Let's fix the concrete issue here, and then get the larger discussion started.

https://codereview.chromium.org/2715153006/diff/40001/components/image_fetche...
File components/image_fetcher/image_fetcher_impl.cc (right):

https://codereview.chromium.org/2715153006/diff/40001/components/image_fetche...
components/image_fetcher/image_fetcher_impl.cc:45: void
ImageFetcherImpl::StartOrQueueNetworkRequest(
On 2017/03/01 09:17:26, Marc Treib wrote:
> nit: empty line before

Done.

[Marc Treib, 2017-03-01 09:41:56]

lgtm

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/s...
File chrome/browser/search/suggestions/image_decoder_impl.h (right):

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/s...
chrome/browser/search/suggestions/image_decoder_impl.h:25: const
image_fetcher::ImageDecodedCallback& callback) override;
On 2017/03/01 09:37:43, tschumann wrote:
> On 2017/03/01 09:17:26, Marc Treib wrote:
> > Hm, one high-level comment: This returns a gfx::Image, which has support for
> > multiple sizes. If that's possible, it might be nicer (API-wise) to simply
> > decode all of them. WDYT?
> 
> I totally agree -- just that's how the whole image decoding stack works :-)
> 
> If there's a place where we need multiple versions of the image, we should
> follow up. Right now, I don't have a clear use case. Plus, that's a much
larger
> change. 
> 
> Let's fix the concrete issue here, and then get the larger discussion started.

I think in terms of API, it would actually be a smaller change: no need to pass
the gfx::Size around everywhere. Just on the lowest level, we'd decode all
available frames.

But probably, an API that allows to request a specific size is useful anyway, so
I'm okay with this too.

[tschumann, 2017-03-01 11:34:18]

tschumann@chromium.org changed reviewers:
+ vollick@chromium.org

[tschumann, 2017-03-01 11:36:36]

(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/s...
File chrome/browser/search/suggestions/image_decoder_impl.h (right):

https://codereview.chromium.org/2715153006/diff/40001/chrome/browser/search/s...
chrome/browser/search/suggestions/image_decoder_impl.h:25: const
image_fetcher::ImageDecodedCallback& callback) override;
On 2017/03/01 09:41:56, Marc Treib wrote:
> On 2017/03/01 09:37:43, tschumann wrote:
> > On 2017/03/01 09:17:26, Marc Treib wrote:
> > > Hm, one high-level comment: This returns a gfx::Image, which has support
for
> > > multiple sizes. If that's possible, it might be nicer (API-wise) to simply
> > > decode all of them. WDYT?
> > 
> > I totally agree -- just that's how the whole image decoding stack works :-)
> > 
> > If there's a place where we need multiple versions of the image, we should
> > follow up. Right now, I don't have a clear use case. Plus, that's a much
> larger
> > change. 
> > 
> > Let's fix the concrete issue here, and then get the larger discussion
started.
> 
> I think in terms of API, it would actually be a smaller change: no need to
pass
> the gfx::Size around everywhere. Just on the lowest level, we'd decode all
> available frames.
> 
> But probably, an API that allows to request a specific size is useful anyway,
so
> I'm okay with this too.

I agree. I'm mostly concerned about changing semantics though and regressions --
given how many parties use this API. If we have a strong use case, I'm
absolutely willing to push that idea forward (just as a separate effort).

[Mathieu, 2017-03-01 14:23:36]

mathp@chromium.org changed reviewers:
+ mathp@chromium.org
- mathp@google.com

[Mathieu, 2017-03-01 14:36:23]

Main comment about documenting what gfx::Size() will mean and when it should be
used.

Also, is there an existing test that we could improve?

https://codereview.chromium.org/2715153006/diff/100001/chrome/browser/image_d...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/2715153006/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder.cc:123: image_request, std::move(image_data),
image_codec, shrink_to_fit);
sorry I'm confused, why don't we need to pass the desired_image_frame_size here?

https://codereview.chromium.org/2715153006/diff/100001/chrome/browser/image_d...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/2715153006/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder.h:92: // one in larger size if there's no precise
match).
mention that people can pass gfx::Size() and what happens then.

https://codereview.chromium.org/2715153006/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2715153006/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:221:
/*desired_image_frame_size=*/gfx::Size(),
it's a nit, but I would put a space between "/" and "gfx"

https://codereview.chromium.org/2715153006/diff/100001/services/image_decoder...
File services/image_decoder/image_decoder_impl_unittest.cc (right):

https://codereview.chromium.org/2715153006/diff/100001/services/image_decoder...
services/image_decoder/image_decoder_impl_unittest.cc:56: gfx::Size(),
add a comment: no preferred size, want the biggest, etc?

[Ken Rockot(use gerrit already), 2017-03-01 17:46:18]

LGTM - maybe someday soon we should just delete chrome/browser/image_decoder*
and update callers to use image_decoder::Decode directly...

[tschumann, 2017-03-01 19:14:48]

On 2017/03/01 17:46:18, Ken Rockot wrote:
> LGTM - maybe someday soon we should just delete chrome/browser/image_decoder*
> and update callers to use image_decoder::Decode directly...

Yes, there's a surprising amount of indirections involved here :-)

[Ken Rockot(use gerrit already), 2017-03-01 19:21:33]

FWIW the image_decoder client library came last, and I intentionally left
the old ImageDecoder API intact as a (now seemingly quite awkward) wrapper
around it to avoid updating existing users. I've filed
https://crbug.com/697557 to get this cleanup on my actual TODO list.

On Wed, Mar 1, 2017 at 11:14 AM, <tschumann@chromium.org> wrote:

> On 2017/03/01 17:46:18, Ken Rockot wrote:
> > LGTM - maybe someday soon we should just delete
> chrome/browser/image_decoder*
> > and update callers to use image_decoder::Decode directly...
>
> Yes, there's a surprising amount of indirections involved here :-)
>
> https://codereview.chromium.org/2715153006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tschumann, 2017-03-01 19:40:08]

mathp, thanks for the careful review!
Concerning testing (which I'm a big fan of):
I added a unit test of components/ntp_tiles/icon_cacher_impl for now.
More importantly, i'm working on a proper e2e test here; passing in a hard-coded
ico file and making sure we choose the proper size.
However, this takes longer as expected (need to inject a fake ImageDataFetcher
into the ImageFetcherImpl) and I'd like to get this fix in before BP.

https://codereview.chromium.org/2715153006/diff/100001/chrome/browser/image_d...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/2715153006/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder.cc:123: image_request, std::move(image_data),
image_codec, shrink_to_fit);
On 2017/03/01 14:36:23, Mathieu Perreault wrote:
> sorry I'm confused, why don't we need to pass the desired_image_frame_size
here?

thanks for the catch! that's why we should have a proper test (see my other
comment). This got lost when rebasing this morning (fixed).

https://codereview.chromium.org/2715153006/diff/100001/chrome/browser/image_d...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/2715153006/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder.h:92: // one in larger size if there's no precise
match).
On 2017/03/01 14:36:23, Mathieu Perreault wrote:
> mention that people can pass gfx::Size() and what happens then.

Done.

https://codereview.chromium.org/2715153006/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2715153006/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:221:
/*desired_image_frame_size=*/gfx::Size(),
On 2017/03/01 14:36:23, Mathieu Perreault wrote:
> it's a nit, but I would put a space between "/" and "gfx"

We adopted this format because there's a clang-tidy check
(https://g3doc.corp.google.com/devtools/cymbal/clang_tidy/g3doc/checks/misc-ar...)
which can verify this. We chose the format without space to minimize space (and
it's how the clang-tidy example are).

https://codereview.chromium.org/2715153006/diff/100001/services/image_decoder...
File services/image_decoder/image_decoder_impl_unittest.cc (right):

https://codereview.chromium.org/2715153006/diff/100001/services/image_decoder...
services/image_decoder/image_decoder_impl_unittest.cc:56: gfx::Size(),
On 2017/03/01 14:36:23, Mathieu Perreault wrote:
> add a comment: no preferred size, want the biggest, etc?

it's actually the smallest :-) I added a comment -- in particular because the
code doesn't care.

[tschumann, 2017-03-01 19:41:24]

The CQ bit was checked by tschumann@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 19:41:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 19:46:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 19:46:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tschumann, 2017-03-01 19:56:15]

The CQ bit was checked by tschumann@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 19:57:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 20:07:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 20:07:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Mathieu, 2017-03-01 20:24:30]

Thanks, lgtm

https://codereview.chromium.org/2715153006/diff/160001/components/image_fetch...
File components/image_fetcher/image_decoder.h (right):

https://codereview.chromium.org/2715153006/diff/160001/components/image_fetch...
components/image_fetcher/image_decoder.h:33: // one in larger size if there's no
precise match).
I would repeat the comment about gfx::Size() everywhere it's relevant.

[tschumann, 2017-03-01 20:55:06]

https://codereview.chromium.org/2715153006/diff/160001/components/image_fetch...
File components/image_fetcher/image_decoder.h (right):

https://codereview.chromium.org/2715153006/diff/160001/components/image_fetch...
components/image_fetcher/image_decoder.h:33: // one in larger size if there's no
precise match).
On 2017/03/01 20:24:30, Mathieu Perreault wrote:
> I would repeat the comment about gfx::Size() everywhere it's relevant.

Done.

[tschumann, 2017-03-01 20:57:08]

The CQ bit was checked by tschumann@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 20:57:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 21:07:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 21:07:48]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tschumann, 2017-03-01 21:13:00]

Patchset #8 (id:140001) has been deleted

[tschumann, 2017-03-01 21:13:28]

The CQ bit was checked by tschumann@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 21:14:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tschumann, 2017-03-01 21:15:51]

tschumann@chromium.org changed reviewers:
- lhchavez@lhchavez.com

[tschumann, 2017-03-01 21:15:52]

+achuith@ for ChromeOS

[tschumann, 2017-03-01 21:23:34]

tschumann@chromium.org changed reviewers:
+ achuith@chromium.org

[commit-bot: I haz the power, 2017-03-01 21:24:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 21:24:56]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tschumann, 2017-03-01 21:26:45]

Patchset #10 (id:100002) has been deleted

[tschumann, 2017-03-01 21:26:56]

The CQ bit was checked by tschumann@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 21:27:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-03-01 21:54:55]

lgtm ios/chrome/browser/suggestions

[commit-bot: I haz the power, 2017-03-01 22:43:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 22:43:08]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-03-02 11:00:46]

only looked at hats_notification_controller_unittest.cc.

lgtm

[tschumann, 2017-03-02 11:01:17]

tschumann@chromium.org changed reviewers:
+ thakis@chromium.org

[tschumann, 2017-03-02 11:03:50]

Nico, could you have a look at chrome/browser/image_decoder* ?

Thanks!

[tschumann, 2017-03-02 11:10:42]

tschumann@chromium.org changed reviewers:
+ rsesek@chromium.org

[tschumann, 2017-03-02 11:10:44]

Robert, can you approve for
services/image_decoder/public/interfaces/image_decoder.mojom ?

Thanks!

[Nico, 2017-03-02 14:17:11]

My files lgtm, one optional nit below.

https://codereview.chromium.org/2715153006/diff/210001/chrome/browser/image_d...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/2715153006/diff/210001/chrome/browser/image_d...
chrome/browser/image_decoder_browsertest.cc:192: false /* shrink_to_fit */,
gfx::Size() /* desired_image_frame_size */);
nit: can you change the /**/ comments to look like so please?

  /*shrink_to_fit=*/false

clang-format knows not to put a space between the comment and the thing after it
when the comment ends in =, and this way it's clear that shrink_to_fit is set to
false. Currently it could also mean "we pass false for this parameter, which has
the effect of enabling shrink to fit".

IMO the /* desired_image_frame_size */ comment doesn't add much, i'd just omit
that one.

Also for the rest of this file.

[tschumann, 2017-03-02 14:57:43]

The CQ bit was checked by tschumann@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 14:58:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 15:03:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 15:03:12]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-03-02 15:07:27]

mojom lgtm

Also, consider wrapping your CL description to 80 columns, to better display in
`git log` in terminals.

[tschumann, 2017-03-02 15:30:11]

Description was changed from

==========
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.


BUG=690029
==========

to

==========
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.
==========

[tschumann, 2017-03-02 16:25:52]

https://codereview.chromium.org/2715153006/diff/210001/chrome/browser/image_d...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/2715153006/diff/210001/chrome/browser/image_d...
chrome/browser/image_decoder_browsertest.cc:192: false /* shrink_to_fit */,
gfx::Size() /* desired_image_frame_size */);
On 2017/03/02 14:17:11, Nico wrote:
> nit: can you change the /**/ comments to look like so please?
> 
>   /*shrink_to_fit=*/false
> 
> clang-format knows not to put a space between the comment and the thing after
it
> when the comment ends in =, and this way it's clear that shrink_to_fit is set
to
> false. Currently it could also mean "we pass false for this parameter, which
has
> the effect of enabling shrink to fit".
> 
> IMO the /* desired_image_frame_size */ comment doesn't add much, i'd just omit
> that one.
> 
> Also for the rest of this file.

I left the comment for desired_image_frame_size for consistency with
shrink_to_fit.
Changed the comment styles (nice benefit: clang-tidy checks know about it and
make sure they stay in sync
(https://g3doc.corp.google.com/devtools/cymbal/clang_tidy/g3doc/checks/misc-ar...)

[tschumann, 2017-03-02 16:26:34]

On 2017/03/02 15:07:27, Robert Sesek wrote:
> mojom lgtm
> 
> Also, consider wrapping your CL description to 80 columns, to better display
in
> `git log` in terminals.

Done. Thanks!

[tschumann, 2017-03-02 18:11:24]

tschumann@chromium.org changed reviewers:
- dcheng@chromium.org, markusheintz@google.com, mkwst@chromium.org,
vollick@chromium.org

[tschumann, 2017-03-02 18:11:51]

The CQ bit was checked by tschumann@chromium.org

[tschumann, 2017-03-02 18:11:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, treib@chromium.org, rockot@chromium.org,
mathp@chromium.org, thakis@chromium.org, rsesek@chromium.org,
achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2715153006/#ps230001
(title: "comment formatting")

[commit-bot: I haz the power, 2017-03-02 18:12:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 19:38:01]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1488478311586440,
"parent_rev": "08eddf09b057e24f14e20ed473836970d0d8021b", "commit_rev":
"30ee1086df7c132ecafae15534946181b03b315c"}

[commit-bot: I haz the power, 2017-03-02 19:38:41]

Description was changed from

==========
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.
==========

to

==========
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/+/30ee1086df7c132ecafae1553494...
==========

[commit-bot: I haz the power, 2017-03-02 19:38:44]

Committed patchset #11 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/30ee1086df7c132ecafae1553494...