Adding a content dependency on components/image_fetcher

[Christian Biesinger]

Hello,

as per the comment in content/browser/DEPS, I am emailing you.

I would like to add a dependency on image_fetcher to content, which is relevant to the implementation of the web platform.

This will be used by the FedCM implementation to download the profile pictures from the server. It is important for privacy reasons that all the pictures get downloaded, which is why this belongs in content.

Please see the CL here:

https://chromium-review.googlesource.com/c/chromium/src/+/5574105

Thanks,

Christian

[danakj]

Are you aware of WebContents::DownloadImage? https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents.h;l=1202-1207;drc=f0e48627a874a75ff702a7c5aaf90b9dc33a6119

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAPTJ0XH9MapYJNVp2NHEAnPf4WohcG049k8%3D%3DhpuX0bPEpLUfA%40mail.gmail.com.

[Christian Biesinger]

Yes, but that does not let us request the image without credentials and with a specific network isolation key which we need to do for privacy reasons. (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/image_downloader/multi_resolution_image_resource_fetcher.cc;l=134;bpv=1;bpt=1)

Christian

[Nasko Oskov]

Looking at the dependencies of image_fetcher the one that stands out to me is "//components/keyed_service/core". I tend to think of keyed services as a //chrome/ (or embedder) layer concept, so it is a tad bit strange to see //content depend on it. The good news is that there isn't a circular dependency, since the image_fetcher depends only on the "core/" part of the keyed services, which is independent of content. It does bring with it "//components/prefs", which is another concept that tends to be at a level above content. It is also clean from circular dependencies, so it also seems ok.

Based on this, it looks mechanically "safe", but on a logical level it is not very clear to me whether //content/ depending on keyed services and prefs is an ok thing to do. Any other thoughts from other content/OWNERS?

Thanks!

Nasko

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAPTJ0XE9AaJwxRCBphKK%2Bck8cRdNpw1m1VriEatSNF1JiVhMmg%40mail.gmail.com.

[Dave Tapuska]

I think image decoder could be pulled out of the image fetcher could it not? Then we wouldn't depend on the keyed service.  It looks like the ImageFetcherService is what is hauling in the keyed service and that isn't used in Christian's CL. The fetching is still done via it's own custom code. 

My proposal would be to create a image decoder component from the image fetcher component. Is that possible?

Dave

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAA%3DmyAuuUruzmRybXW3qSGJ%2BLBbUTqi_g7kGiUZ_Pu0fWKwKEw%40mail.gmail.com.

[Christian Biesinger]

Urg this keyed service is unfortunate. I actually hate the way ImageFetcherService works in general, it is a weird semi-android-only thing (https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/image_fetcher/image_fetcher_service_factory.cc;l=80;drc=c0265133106c7647e90f9aaa4377d28190b1a6a9;bpv=0;bpt=1)

Maybe I should just add a DecodeImage function to one of the fedcm delegates that are already implemented in //chrome...

Christian

[danakj]

On Wed, May 29, 2024 at 12:12 PM Christian Biesinger <cbies...@google.com> wrote:

Urg this keyed service is unfortunate. I actually hate the way ImageFetcherService works in general, it is a weird semi-android-only thing (https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/image_fetcher/image_fetcher_service_factory.cc;l=80;drc=c0265133106c7647e90f9aaa4377d28190b1a6a9;bpv=0;bpt=1)

Maybe I should just add a DecodeImage function to one of the fedcm delegates that are already implemented in //chrome...

Maybe the Chrome layer is doing something important, and you were speaking about the need to fetch the image, so maybe you're thinking more of a FetchAndDecodeImage. But can content call the DataDecoder service directly? https://source.chromium.org/chromium/chromium/src/+/main:services/data_decoder/public/cpp/decode_image.h;l=55-61?q=f:services%20decodeimage I am not sure if this is helpful... but just in case.

[Christian Biesinger]

Oh that's a good idea! It looks like content already depends on data_decoder thankfully, so I should be able to make that work.

[Christian Biesinger]

Thanks, that worked https://chromium-review.googlesource.com/c/chromium/src/+/5574105)

I am withdrawing my request to add this dependency.

Christian

[danakj]

Cool :)