[NTP Tiles] Avoid duplicate requests from IconCacherImpl

[NTP Tiles] Avoid duplicate requests from IconCacherImpl

This CL adds recording of requests in-flight so that we do not issue the
same network request twice. The cacher is often triggered both for PS
and for ML several times in a quick succession.

This is a kind of a hack to make experimentation with fetching favicons
for MostLikely tiles in M60 safer (not issuing too many network
requests). This should be removed once the underlying issue in
most_visited_sites is addressed.

BUG=720536
Review-Url: https://codereview.chromium.org/2873403002
Cr-Commit-Position: refs/heads/master@{#472861}
Committed: https://chromium.googlesource.com/chromium/src/+/fc015b3cb9d91a89707e479635fad2cd3695cd1d

[jkrcal, 2017-05-10 17:53:14]

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

[jkrcal, 2017-05-10 17:54:10]

jkrcal@chromium.org changed reviewers:
+ sfiera@chromium.org

[jkrcal, 2017-05-10 17:54:10]

Chris, could you PTAL?
(feel free to suggest another reviewer if you want to get out of this business
;)

[commit-bot: I haz the power, 2017-05-10 17:54:56]

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-05-10 19:41:15]

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

[commit-bot: I haz the power, 2017-05-10 19:41:16]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sfiera, 2017-05-15 15:13:31]

This implementation feels like it violates the expected API of the icon cacher.
You might start a fetch and not get back a response when the icon is cached.

How much more work would it be to have a std::map<GURL, std::vector<Callback>>
in_flight_requests_, and notify all pending callers when a request completes?

[jkrcal, 2017-05-17 08:48:39]

Thanks! Could you PTAL, again?

[sfiera, 2017-05-17 09:11:51]

LGTM with optional suggestions

https://codereview.chromium.org/2873403002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl.cc (right):

https://codereview.chromium.org/2873403002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:214: if (newly_available && callback) {
Feels wrong to have newly_available checked inside this loop. I would go for
something more like:

1. Move vector into local variable and erase it from the map
2. If !newly_available, we're done
3. Notify callbacks.

https://codereview.chromium.org/2873403002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl_unittest.cc (right):

https://codereview.chromium.org/2873403002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl_unittest.cc:394: EXPECT_CALL(done,
Run()).WillOnce(Quit(&loop));
You can chain:

EXPECT_CALL(…)
    .WillOnce(…)
    .WillOnce(…);

[jkrcal, 2017-05-17 11:41:08]

Thanks!

https://codereview.chromium.org/2873403002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl.cc (right):

https://codereview.chromium.org/2873403002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:214: if (newly_available && callback) {
On 2017/05/17 09:11:51, sfiera wrote:
> Feels wrong to have newly_available checked inside this loop. I would go for
> something more like:
> 
> 1. Move vector into local variable and erase it from the map
> 2. If !newly_available, we're done
> 3. Notify callbacks.

Huh, thanks!
I've thought about it but then I dropped it because I did not realize I can
_move_ it out.

Done.

https://codereview.chromium.org/2873403002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl_unittest.cc (right):

https://codereview.chromium.org/2873403002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl_unittest.cc:394: EXPECT_CALL(done,
Run()).WillOnce(Quit(&loop));
On 2017/05/17 09:11:51, sfiera wrote:
> You can chain:
> 
> EXPECT_CALL(…)
>     .WillOnce(…)
>     .WillOnce(…);

Done.

[jkrcal, 2017-05-17 11:41:14]

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

[commit-bot: I haz the power, 2017-05-17 11:41:29]

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-05-17 12:08:21]

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

[commit-bot: I haz the power, 2017-05-17 12:08:22]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jkrcal, 2017-05-17 13:17:20]

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

[commit-bot: I haz the power, 2017-05-17 13:17:41]

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-05-17 14:44:09]

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

[commit-bot: I haz the power, 2017-05-17 14:44:10]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-05-18 15:27:00]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-05-18 15:27:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2873403002/#ps60001
(title: "Fix build for windows")

[commit-bot: I haz the power, 2017-05-18 15:27:39]

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-05-18 17:43:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495121220366020,
"parent_rev": "265357c7a152a62f98e998111f8b77c3779a234a", "commit_rev":
"fc015b3cb9d91a89707e479635fad2cd3695cd1d"}

[commit-bot: I haz the power, 2017-05-18 17:43:35]

Description was changed from

==========
[NTP Tiles] Avoid duplicate requests from IconCacherImpl

This CL adds recording of requests in-flight so that we do not issue the
same network request twice. The cacher is often triggered both for PS
and for ML several times in a quick succession.

This is a kind of a hack to make experimentation with fetching favicons
for MostLikely tiles in M60 safer (not issuing too many network
requests). This should be removed once the underlying issue in
most_visited_sites is addressed.

BUG=720536
==========

to

==========
[NTP Tiles] Avoid duplicate requests from IconCacherImpl

This CL adds recording of requests in-flight so that we do not issue the
same network request twice. The cacher is often triggered both for PS
and for ML several times in a quick succession.

This is a kind of a hack to make experimentation with fetching favicons
for MostLikely tiles in M60 safer (not issuing too many network
requests). This should be removed once the underlying issue in
most_visited_sites is addressed.

BUG=720536

Review-Url: https://codereview.chromium.org/2873403002
Cr-Commit-Position: refs/heads/master@{#472861}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc015b3cb9d91a89707e479635fa...
==========

[commit-bot: I haz the power, 2017-05-18 17:43:37]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/fc015b3cb9d91a89707e479635fa...