There is a cache already, but the images are getting evicted because it only keeps 5 images (hard-coded constant).  This is why sometimes the image flickers and other times it remains stable.  So I will fix this by letting the cache accept a parameter for how many images to keep, and pass in something more reasonable for entity suggestion use.  Suggestions for this number are welcome.  I think it should definitely be more than 6 because, even with only 6 suggestion lines, the images may change as the user types and some of these should remain stable.

This may help: https://cs.chromium.org/chromium/src/components/omnibox/browser/favicon_cache.cc?type=cs&g=0&l=19

Cache size change fix works and is in review at crrev.com/c/1150415

Just added petewil@ as owner reviewer to help it through.

After hashing things out through email with groby, petewil, and jdonnelly, we decided to leave Android as it is (because entities are desktop only) and to be more conservative with the desktop cache size increase for this release, giving just enough to stop the flickering for standard 6 suggestions.  Surprisingly, this turned out to be 17 but programmers and computers like even numbers so I gave it 18.

Here are some ideas Rachel had for long term solutions:

* Turn off the feature for Android
* Turn it off for svelte only.
* palettize images for Android (dropping us from 16K/pic to 4K/pic)
* Emptying the cache whenever the Omnibox isn't active
* Tying the feature into memory pressure signals

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffdec5902b4058b971faed828c17ac3300153e6b

commit ffdec5902b4058b971faed828c17ac3300153e6b
Author: Orin Jaworski <orinj@chromium.org>
Date: Wed Aug 01 00:13:13 2018

Increase BitmapFetcherService cache size

The larger cache prevents entity images from flickering while
typing. The new size is expected to consume ~288 kilobytes,
and is not changed for Android.

Bug:  866580 
Change-Id: Ifa53ffa82598335461c286547085620c122a44a1
Reviewed-on: https://chromium-review.googlesource.com/1150415
Reviewed-by: Peter Williamson <petewil@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Rachel Blum <groby@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579631}
[modify] https://crrev.com/ffdec5902b4058b971faed828c17ac3300153e6b/chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc

Verified in Win Canary (70.0.3511.0).

This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Is the change fully safe to merge to M69? Also pls justify this merge for M69. Thank you.

Yes, this change is very small and relatively safe (changing only the size of a cache). And it fixes a severe visual issue (image flickering as the user types).

Approving merge to M69 branch 3497 based on comment #10 and #13. Please merge. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb632196b1b9d17441b861c40be7259f50364203

commit bb632196b1b9d17441b861c40be7259f50364203
Author: Orin Jaworski <orinj@chromium.org>
Date: Fri Aug 03 20:36:56 2018

Increase BitmapFetcherService cache size

The larger cache prevents entity images from flickering while
typing. The new size is expected to consume ~288 kilobytes,
and is not changed for Android.

Bug:  866580 
Change-Id: Ifa53ffa82598335461c286547085620c122a44a1
Reviewed-on: https://chromium-review.googlesource.com/1150415
Reviewed-by: Peter Williamson <petewil@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Rachel Blum <groby@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#579631}(cherry picked from commit ffdec5902b4058b971faed828c17ac3300153e6b)
Reviewed-on: https://chromium-review.googlesource.com/1162524
Reviewed-by: Justin Donnelly (OOO until Aug 13) <jdonnelly@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#390}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/bb632196b1b9d17441b861c40be7259f50364203/chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc

+abdulsyed@ fyi, M69 merges taken for Proj-MdRefresh .

The fix above was and still is valid, and eliminated most flicker caused by image cache misses.  But some flicker can still happen, seemingly because of a deeper bug.  jdonnelly@ and I have both repro'd in Canary by starting to type "chris".  Backing up and re-typing the 'i', the top two images appear instantly (suggesting they are ready/cached at time of keypress) and THEN, maybe 600+ milliseconds later, they blink out (for ~50ms) and re-appear unchanged.  This suggests that something is coming in late, setting placeholders again and reloading the image.

It may be that something is wiping out the cache with other images that don't get shown because I repro'd in dev on Linux and then resolved the problem by increasing cache size from 18 to 20.  So another cache size tweak can eliminate symptoms easily, but until we resolve the underlying cause of cache-overwrite | image-reset, we can't be sure this is fully fixed.

It may be useful to split this into two bugs: one for cache size resolution and the other for deeper logic resolution.  Do we still want to get a bigger cache into M69?  18 is *way* better than 5, but my original 20 would have been just enough - at least for this test case.

I'd suggest you investigate a bit more. If you find some distinct issue that should be fixed, then open a new bug.

In the meantime, I'm a bit skeptical that 20 is necessarily better than 18. It feels like maybe that is just working in some specific set of circumstances that wouldn't necessarily produce better overall behavior.

A cache large enough to handle the overwrites will effectively mask the blinking symptoms because the images will be immediately available when reloaded.  I will look into what's causing the reloading &| cache misses (the deeper issue), but for sure 20 is better than 18.  And quite possibly X > 20 is even better, able to mask blinking for other cases, but "chris" is the worst known to me so far.  For some reason, testing with "rachel" before gave a full set of entity suggestions (as does "chris") but with no blinking.

The cache is getting blown away by entity image Prefetch.  We receive lots of images for entities that never end up getting shown.  Should we even be prefetching all these images for results that get culled?  Even on mobile devices where data consumption may be an issue?

drive-by: we receive a lot of suggestions from the server.  In fact, I think the number of suggestions it can return is 20.  Most of these will go unused.  I don't think we should be fetching, in reaction to each response, more entities than we might be able to show from the given response, i.e., 6 (?).

Thanks mpearson@, that's just what I'm seeing: 20 entity images get prefetched and only a few get shown.  I am looking now to see if there's a way to prefetch only the top N most relevant entity images that will get shown (default N = 6).  If not, we may experiment with turning off prefetch.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1ed30e2c2f8a60715324463c7ffd85597e4c6c7

commit e1ed30e2c2f8a60715324463c7ffd85597e4c6c7
Author: Orin Jaworski <orinj@chromium.org>
Date: Tue Aug 21 03:11:25 2018

[omnibox] Limit suggestion image prefetch to most relevant

Images for all suggestions were being prefetched, but only the few
most relevant were being shown.  This was not only wasteful but
also damaging to the user experience because the image cache was
getting overwritten, causing entity suggestion images to flicker
while reloading.  This CL fixes the problem by limiting the prefetch
to only the first N, most relevant results, where N is the maximum
number of match results to be shown (default N = 6).

Bug:  866580 
Change-Id: If04013be9b2801e7ce6a6177dbc973f4c3fd884f
Reviewed-on: https://chromium-review.googlesource.com/1176165
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584620}
[modify] https://crrev.com/e1ed30e2c2f8a60715324463c7ffd85597e4c6c7/components/omnibox/browser/base_search_provider.cc
[modify] https://crrev.com/e1ed30e2c2f8a60715324463c7ffd85597e4c6c7/components/omnibox/browser/search_provider.cc
[modify] https://crrev.com/e1ed30e2c2f8a60715324463c7ffd85597e4c6c7/components/omnibox/browser/search_provider.h
[modify] https://crrev.com/e1ed30e2c2f8a60715324463c7ffd85597e4c6c7/components/omnibox/browser/search_suggestion_parser.cc
[modify] https://crrev.com/e1ed30e2c2f8a60715324463c7ffd85597e4c6c7/components/omnibox/browser/search_suggestion_parser.h

Requesting merge for change in #24. Or do we need to open a new bug because there's already been a merge here?

Risk: The CL above is low risk. While it does touch a few different files and changes the logic in a non-trivial way, this is mitigated by the fact that the system it's affecting is the scheduling of prefetching of images. So any issues that might possibly arise would probably be limited to not prefetching images when desired or fetching too many or too often.

Justification: We probably can't launch the feature in question (rich entity suggestions, http://crbug.com/806995, which is part of our birthday plans) without this fix.

This bug requires manual review: We are only 13 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Pls update bug with canary result tomorrow. 

Will do.

The NextAction date has arrived: 2018-08-22

I've confirmed that the latest canary (70.0.3530.0) on both Windows and Mac does not have the flickering and the requested merge (comment 24) is working as intended.

Approving merge to M69 branch 3497 based on comment #25 and #31. Pls merge. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5f35302e8e076ff3a309551a4facc8ac4f0f96e

commit f5f35302e8e076ff3a309551a4facc8ac4f0f96e
Author: Orin Jaworski <orinj@chromium.org>
Date: Wed Aug 22 16:22:11 2018

Reduce bitmap fetcher image cache size

The increase from 5 to 18 entries significantly reduced
entity image flicker, but the problem was not eliminated
completely until the underlying cause of cache overwrite
was addressed.  With image prefetch logic fixed, the cache
size can be reduced to a tighter bound.  5 is still not
enough, but 12 is sufficient (see comment).

Bug:  866580 
Change-Id: Ifc3f7d84ee59eea277756d5f32d302cc7f5949cc
Reviewed-on: https://chromium-review.googlesource.com/1182610
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Peter Williamson <petewil@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585037}
[modify] https://crrev.com/f5f35302e8e076ff3a309551a4facc8ac4f0f96e/chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc
[modify] https://crrev.com/f5f35302e8e076ff3a309551a4facc8ac4f0f96e/components/omnibox/browser/search_suggestion_parser.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/574792cc76ec59974ddb57a11b575650b1dbc53b

commit 574792cc76ec59974ddb57a11b575650b1dbc53b
Author: Orin Jaworski <orinj@chromium.org>
Date: Wed Aug 22 17:17:37 2018

[omnibox] Limit suggestion image prefetch to most relevant

Images for all suggestions were being prefetched, but only the few
most relevant were being shown.  This was not only wasteful but
also damaging to the user experience because the image cache was
getting overwritten, causing entity suggestion images to flicker
while reloading.  This CL fixes the problem by limiting the prefetch
to only the first N, most relevant results, where N is the maximum
number of match results to be shown (default N = 6).

Bug:  866580 
Change-Id: If04013be9b2801e7ce6a6177dbc973f4c3fd884f
Reviewed-on: https://chromium-review.googlesource.com/1176165
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#584620}(cherry picked from commit e1ed30e2c2f8a60715324463c7ffd85597e4c6c7)
Reviewed-on: https://chromium-review.googlesource.com/1183822
Cr-Commit-Position: refs/branch-heads/3497@{#770}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/574792cc76ec59974ddb57a11b575650b1dbc53b/components/omnibox/browser/base_search_provider.cc
[modify] https://crrev.com/574792cc76ec59974ddb57a11b575650b1dbc53b/components/omnibox/browser/search_provider.cc
[modify] https://crrev.com/574792cc76ec59974ddb57a11b575650b1dbc53b/components/omnibox/browser/search_provider.h
[modify] https://crrev.com/574792cc76ec59974ddb57a11b575650b1dbc53b/components/omnibox/browser/search_suggestion_parser.cc
[modify] https://crrev.com/574792cc76ec59974ddb57a11b575650b1dbc53b/components/omnibox/browser/search_suggestion_parser.h

Fixed and merged.