Re: New //component deps in //content/public

[Elias Klim]

Sorry for the late reply, I was on vacation. 

+Florian Jacky (the 4st person who is looking into this topic).

+Christian Dullweber +Balazs Engedy (more content_settings owners)

The permissions pipeline ended up in a peculiar state because part of our logic exists in `//content` (permission is part of Web Platform) and another part exists in content embedders and `//components` because of dependency on `components/content_settings`. Our goal was to move all security & privacy relevant logic to `//content` so that all embedders automatically benefit from it. 

We decided not to add `components/content_settings/core/common/` dependency to `//content/public/`. I wasn't able to convince myself that this code would naturally fit in `//content/public` because here is a comment that says "It should not depend on ... chrome implementation details such as settings ...". To avoid deps issue, we use a delegate that is declared in `//content/public` but initialized in `//chrome`. 

If we decide that it is OK to depend on `components/content_settings/core/common/` because it is used in many Web Platform features, it would significantly simplify our life and allow us to implement features that we previously were not able to do (without too much complexity).   

Many thanks,

Elias

On Mon, Nov 18, 2024 at 11:35 AM Arthur Sonzogni <arthurs...@google.com> wrote:

@Elias Klim Gentle ping. What is the status?

+CC @Ryan Tarpine who also faces a similar dependency issue today (discussion)

In total, I see at least 3 developers (Elias, Ryan, Florent) trying to move features from //chrome to //content, but being blocked by `components/content_settings/core/common/` not being available from content/.

We should consider adding it as a content/dependency, because:

• It only depends on targets that are already dependencies of content/, except for //components/privacy_sandbox (?). The latter might be removed easily. It is a one-liner checking one base::feature state.
• It is already a transitive dependency of content/ via //services/network.

Before / After:

|

@content-owners (FYI) It feels weird content/ to depend on a subtarget of a component, while the whole component depends on content/. Please shout if this is the wrong direction and suggest alternatives.

@Martin Šrámek: As OWNER of `components/content_settings/core/common/`, are you okay to make it a dependency of content/?  This means not being able to use dependencies content/ can't itself be used. This means only the common things for every web browser (Android Webview, Chrome, ChromeCast, etc...). Given the //services/network dependency, I guess this is probably already the expectation. somehow We might want to add a README to explain what ../core/common represent.

Arthur @arthursonzogni

On Wed, Nov 6, 2024 at 6:29 PM Arthur Sonzogni <arthurs...@google.com> wrote:

+CC @Florent Castelli: here is a useful discussion. Florent was interested using ContentSettingsPattern from //content.

@Elias Klim What is the status of providing content embedders permissions support for free?

Arthur @arthursonzogni

On Fri, Apr 26, 2024 at 6:35 PM Arthur Sonzogni <arthurs...@google.com> wrote:

Good to know this class has only basic dependencies. Yes, `content` already depends on all of them, so this should be fine.

Let see the dependants:

arthursonzogni@arthursonzogni:~/chromium/src$ git grep -l ContentSettingsPattern | grep -v test | cut -d'/' -f1-2 | sort | uniq -c | sort -n
      1 chrome/app
      1 chromecast/browser
      1 components/browsing_data
      1 components/page_info
      1 services/network
      1 tools/ipc_fuzzer
      2 components/background_sync
      2 components/safe_browsing
      2 components/security_interstitials
      2 components/supervised_user
      3 components/privacy_sandbox
      4 components/signin
      4 components/tpcd
      4 ui/webui
      5 ios/chrome
      6 components/browser_ui
      6 extensions/browser
     10 components/permissions
     51 components/content_settings
     69 chrome/browser

It looks like there are many, including `ios`. So, you won't easily be able to move the definition toward `content/public`.
I guess this is what led you to what you propose.

Could you describe how content/ and its embedder are going to use the structure on both sides of the interface?
I wonder if `content` really needs to know about it fully, or if we could only expose some interface to be implemented by embedders. What member function will be used from content/ ?

Arthur @arthursonzogni

On Fri, Apr 26, 2024 at 5:53 PM Elias Klim <el...@google.com> wrote:

Thank you Arthur!

answers are inline. 

On Fri, Apr 26, 2024 at 2:43 PM Arthur Sonzogni <arthurs...@google.com> wrote:

Hi Eliais,

I don't really know much about this component. So making a reply will be difficult.

Please don't take my replies as authoritative, I am only making deductions from code/READMEs.

____

I see the dependency over components/privacy_sandbox.

It seems to be used for Topics. What is the plan with this component? Is this going to be integrated into content/ as well, or will it be left out?

We plan to create a new minimalistic build target which will include only files that are needed for ContentSettingsPattern to function.  Based on the includes, it is 

- base

- net

- url

and content_settings_pattern_parser.h which uses

- base

- url

My understanding is that all of the above mentioned deps are already available in //content. So technically we're talking only about 2 extra files content_settings_pattern.h and 

content_settings_pattern_parser.h.

____

components/README.md says component are used for:

1. Features shared by Chrome on iOS and Chrome on other platforms.
2. Features shared in between multiple content/ embedders
3. Features shared between Blink and the browser process process.

I guess you won't support Chrome on iOS. Isn't it?

That's right. Chrome on iOS does not use permissions infrastructure from //content. 

It looks like this shouldn't be a component/ anymore, but a subdirectory in `content`. This will be part of the WebPlatform. What do you think about moving it fully inside a content/ subdirectory and not adding the `content/public` dependency over the component?

As I'm not an owner of neither the files nor the component, let's leave it as the final option. I'm worried that content_settings_pattern.h is mostly used in components/content_settings and it conceptually belongs there.

____

There is a class [1] in //components that represents a set of rules for an origin. When permission’s status changes, we use it to match with a list of origins. I would like to allow `content/public/browser/permission_controller.h` [2] depending on it. We will make the new dependency extremely tiny.

Do you need content::PermissionController or content::PermissionControllerImpl to depend on it?

The logic will be placed in content::PermissionControllerImpl but content::PermissionController needs to be aware of it. The plan is to add a new method with ContentSettingsPattern as an argument. So as soon as permission's status changes, we can call BrowserContext->PermissionController->OnPremissoinChanged(Permission, ContentSettingsPattern).

Arthur @arthursonzogni

On Fri, Apr 26, 2024 at 11:10 AM Elias Klim <el...@google.com> wrote:

Hi Arthur!

I need little advice from you as a //content owner. 

TL;DR How hard is it to add new deps for //components in //content/public?

We’re moving permissions Web Platform-related code from //component to //content so that all content embedders get permissions support for free. 

There is a class [1] in //components that represents a set of rules for an origin. When permission’s status changes, we use it to match with a list of origins. I would like to allow `content/public/browser/permission_controller.h` [2] depending on it. We will make the new dependency extremely tiny.

Do you think it is realistic?

[1] https://source.chromium.org/chromium/chromium/src/+/main:components/content_settings/core/common/content_settings_pattern.h;l=29;drc=3c631ad94427c96745a01727f3a0085785afdac6 

[2] https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/permission_controller.h;l=32;drc=33b441e83b1f70381158fcafb0ecde9168b79524 

Many thanks,

Elias

--

Elias Klim Software Engineer el...@google.com

[Arthur Sonzogni]

[Christian Dullweber]

I think it is fine to add this dependency. There is an indirect dependency already anyways and I wouldn't see the content settings infrastructure as something specific to Chrome settings.

components/content_settings is not a build target, only its subfolders are. I'm not sure why components/content_settings/ is listed in the graphs? If you extend this to the whole name of the build targets, it looks a little less confusing.

[Florent Castelli]

Do we have any time estimate on when this might be done or any issue we can track about it?

+Guido Urdaneta (Adding my TL for tracking this task)

[Arthur Sonzogni]

Thanks Elias and Christian, it looks like everyone agrees.

> Do we have any time estimate on when this might be done or any issue we can track about it?

I just opened a task. This should be quite straightforward for whoever feels interested. To avoid duplicated work, don't forget to assign yourself.

Arthur @arthursonzogni