Probably can just wire OnExternalCacheHit on top of NetworkContext mojo interface or something? Though it feels a little cluttery...

That's what I was thinking.  There are too many URLLoaderFactory implementations to go that route, and doesn't seem worth a dedicated interface.  This should be pretty trivial to do, though if we want to test it, that may be more difficult.

Thanks for finding this. Matt: do you have time to fix this? If not, can you assign to morlovich@ if he has time?

I don't think it's worth fixing this until we figure out what to do with the media cache.

I see, so are you fine with us shipping to stable without this notification being plumbed? 

Comparing HttpCache.Pattern.* may help decide the impact, but that somehow has very different event counts (15-18% difference!) even if I fix the cache backend.

I don't think it makes any sense to ship before the media cache is figured out.

Re comment 8, is there a specific bug for the media cache experiments that we could have this conversation on and look at metrics?

It's issue 789657.

Unassigning myself from network service issues.

I could take this, but it may also be a nice starter exercise.

Hi Steven, I've talked to Maks and this could be a good first bug to tackle.

To elaborate, there are actually two possible approaches:
1) Straightforward one, which what I carelessly thought was all that was needed originally:
   Fix up the (browser-process) code in WebContentsImpl to talk to NetworkService.  This is straightforward (basically can extend the existing NetworkContext API), but the problem is that we end up doing IPCs  Renderer -> Browser -> Network Service, which is wasteful.

2) Change the renderer to talk to network service directly. This requires more finesse in the IPC design since the renderer is untrusted (so can't hand it objects that have dangerous primitives), and also to wire up the renderer side, which may be a bit tricky since not too many people here are familiar with that code (and I am not one of them).

Sure thing, I'll start on this as soon as I can. Probably a couple days from now

This doesn't happen with high frequency, so 1) should be enough.

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

commit 674fb0828b680f4521ab24a56616ede79de2511a
Author: Steven Bingler <bingler@chromium.org>
Date: Wed Dec 12 18:04:52 2018

OnDidLoadResourceFromMemoryCache notifies network service cache correctly

OnDidLoadResourceFromMemoryCache calls new NotifyExternalCacheHit
when network service is active as to properly update the cache with an external hit.

Bug:  893323 
Change-Id: Ia21b3441122a55d8eb5a8c7e508b4243ab500cda
Reviewed-on: https://chromium-review.googlesource.com/c/1366435
Commit-Queue: Steven Bingler <bingler@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615956}
[modify] https://crrev.com/674fb0828b680f4521ab24a56616ede79de2511a/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/674fb0828b680f4521ab24a56616ede79de2511a/net/BUILD.gn
[modify] https://crrev.com/674fb0828b680f4521ab24a56616ede79de2511a/net/http/mock_http_cache.cc
[modify] https://crrev.com/674fb0828b680f4521ab24a56616ede79de2511a/net/http/mock_http_cache.h
[modify] https://crrev.com/674fb0828b680f4521ab24a56616ede79de2511a/services/network/network_context.cc
[modify] https://crrev.com/674fb0828b680f4521ab24a56616ede79de2511a/services/network/network_context.h
[modify] https://crrev.com/674fb0828b680f4521ab24a56616ede79de2511a/services/network/network_context_unittest.cc
[modify] https://crrev.com/674fb0828b680f4521ab24a56616ede79de2511a/services/network/public/mojom/network_context.mojom
[modify] https://crrev.com/674fb0828b680f4521ab24a56616ede79de2511a/services/network/test/test_network_context.h

Commit https://chromium.googlesource.com/chromium/src/+/674fb0828b680f4521ab24a56616ede79de2511a is merged. Closing as fixed