Re: Seeking advice: avoiding disallowed types in Blink

[Daniel Cheng]

If this were my CL, I would punch a hole through the Blink public API for //content/renderer to provide the URLLoaderFactory to Blink (option 1). But I think there will be varying opinions on what the right approach is here; we have been actively trying to reduce the size of //content/renderer, and option 1 runs counter to that long-term goal.

So I suspect the general recommendation should be option 3.

There is another alternative, which is to:

- define C++ types in //third_party/blink/public/common

- typemap the Mojo types to the C++ types for both regular and non-Blink variants

- use the type on both sides

But I think allowlisting the Mojo type (option 3) would probably be better here; I can't see how an additional C++ type would add benefit.

Daniel

On Wed, 9 Oct 2024 at 11:01, Alex Yang <ayc...@google.com> wrote:

We are adding experimental infrastructure to allow WebUI renderer processes to load resources without needing to communicate with the browser process. Rationale Design sketch

In our prototype, we added a URLLoaderFactory-like thing in Blink. This is an object that lives in the renderer process and responds to URL requests with data retrieved from the ResourceBundle. Prototype (see local_resource_url_loader_factory.cc)

This causes presubmit to warn that we are using disallowed types in Blink. Presubmit warnings. Some warnings are about using existing types in Blink, e.g. network::ResourceRequest. Some are about using the non-Blink variant of our own type, mojom::LocalResourceLoaderConfigPtr. We used the non-Blink variant because this object is initially passed from the browser.

What is the recommended way to structure this change? Should we:

1. Move the URLLoaderFactory-like thing out of Blink
2. Leave the URLLoaderFactory-like thing in Blink and fix it to use only allowed types (e.g. unmarshal the non-Blink variant LocalResourceLoaderConfig or convert to the Blink variant)
3. Update the presubmit to allow these types

Thanks!

[Alex Yang]

[Daniel Cheng]

(resending from the right email address; sorry for the spam)

If this were my CL, I would punch a hole through the Blink public API for //content/renderer to provide the URLLoaderFactory to Blink (option 1). But I think there will be varying opinions on what the right approach is here; we have been actively trying to reduce the size of //content/renderer, and option 1 runs counter to that long-term goal.

So I suspect the general recommendation should be option 3.

There is another alternative, which is to:
- define C++ types in //third_party/blink/public/common
- typemap the Mojo types to the C++ types for both regular and non-Blink variants
- use the type on both sides

But I think allowlisting the Mojo type (option 3) would probably be better here; I can't see how an additional C++ type would add benefit.

Daniel

On Wed, 9 Oct 2024 at 11:01, Alex Yang <ayc...@google.com> wrote:

[Kentaro Hara]

I agree that Option 3 is okay.

> - define C++ types in //third_party/blink/public/common

I think this is a good option too. blink/common is intended to be used by both browser and renderer and can use non-Blink types.

--
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/CAF3XrKpHajuQE6C5ASOdUdxyq5HtGef-nBQbfNtPnzy0MsKe%3Dg%40mail.gmail.com.

--

Kentaro Hara, Tokyo

[Alex Yang]

Thanks for the input Daniel and Kentaro! We'll go with option 3.