Using non-Blink Mojo interfaces/structs in Blink for loader's onion soup

[Makoto Shimazu]

Hi members,

I'm planning to add some code in platform/loader/ to use non-Blink Mojo headers (*.mojom.h) in Blink for Onion Soup for loaders.

Explainer: https://docs.google.com/document/d/1pGZEtHsQkuRTr2dNHQiqD0Qm_xMpP_HT7vfszG2NiPE/edit#

If you have any questions, suggestions, comments, or concerns, please let me know.

Thanks,

Makoto

[Kentaro Hara]

Thanks for reaching out to platform-architecture-dev! :)

Let me ask two questions:

- Are the mojo types used in the browser process? If yes, they should go to blink/common/. blink/common/ should use non-Blink mojo types.

- If no, what's a reason you cannot onion-soup all of the call sites to Blink?

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHg_a%2BsfvTaYmqhBtherGBG0G5%2B1gmKVGU%3Drz-%3Dskc_EJSSryA%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

[Makoto Shimazu]

2019年8月26日(月) 23:10 Kentaro Hara <har...@chromium.org>:

Thanks for reaching out to platform-architecture-dev! :)

Let me ask two questions:

- Are the mojo types used in the browser process? If yes, they should go to blink/common/. blink/common/ should use non-Blink mojo types.

- If no, what's a reason you cannot onion-soup all of the call sites to Blink?

The Mojo structs (network.mojom.URLRequest/URLResponseHead) are used in both of the processes, but the usage of them in Blink are limited in the renderer.

The reason why I want to expose them to the outside of Blink is because there's a logic for some fields in the network requests/responses to be modified by URLLoaderThrottles which are implemented in chrome/, component/ etc. 

Technically we can expose those values to the throttles by converting from Blink type to non-Blink type and put them back to the original Blink struct after calling a method of the URLLoaderThrottle, but apparently it needs extra cost of type conversions.

[Kentaro Hara]

If the mojo structs are used in browser and renderer, they should be put in blink/common/. blink/common/ should follow Chromium's coding style (not Blink's coding style) and use non-blink mojom types.

Does it solve your problem? :)

[Makoto Shimazu]

Thanks!

Let me just clarify that the Mojo structs are defined outside of Blink (services/network/public/mojom/). The problem is where we use them and how to use them.

Putting code using the Mojo types in blink/common/ might work, but I'm wondering if the benefit of the Onion Soup would be limited because we cannot use the mojom interfaces/structs from the loader code in Blink and need to keep the blink/common layer as a thin Mojo wrapper.

2019年8月27日(火) 16:15 Kentaro Hara <har...@chromium.org>:

[Kentaro Hara]

Would there be any option to let //components/ use blink::mojom types? We're already doing it.

I might not be understanding the problem well -- shall we chat offline tomorrow? :-)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHg_a%2BvahR7j_%3D%2BdATiD3VgeRQ2rM7kpd0kGXKMt0eUXdANJbA%40mail.gmail.com.

[Kentaro Hara]

Discussed with Shimazu-san offline.

TL;DR: +1 to shimazu-san's proposal. To avoid introducing type mappings and/or type conversion mechanisms between Blink and Chromium, we need to allow non-blink mojom types in platform/loader/.

The distinction between blink mojom types and non-blink mojom types has led to a lot of type mappings and type conversion mechanisms [Note: The goal of Onion Soup is to remove these unnecessary abstraction mechanisms]. I think that it's time to rethink about whether we really need blink mojom types. I'm not planning to dig into this deep discussion on this thread, but overall I think it's more constructive to think about ways to reduce type mappings and type conversion mechanisms rather than sticking to the strict distinction between blink mojom types and non-blink mojom types.

Any thoughts are welcome :)

[Yutaka Hirano]

I'd like more details about the boundary - where should we use blink::ResourceRequest and where should we use network::mojom::URLRequest?

This is not explicitly described in the document, but I believe we would like to remove blink::WebURLRequest as well in the future. Currently we can convert blink::ResourceRequest to blink::WebURLRequest with virtually 0 cost with blink::WrappedResourceRequest, but that will not be possible with network::mojom::URLRequest. That is why I think the use of network::mojom::URLRequest will spread and it's good to think about the boundary problem sooner rather than later.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jzi9DhqRkkwgKFs69tR1uGGBy6sGESmzMRfN%2B64nYUGng%40mail.gmail.com.

[Kentaro Hara]

Feel free to disagree:

where should we use blink::ResourceRequest and where should we use network::mojom::URLRequest?

In the end state, I think we should use network::mojom::URLRequest everywhere. Then deprecate blink::ResourceRequest.

In more general, we can rephrase the question as follows: Where should we use WTF::String/Vector/etc and where should we use std::string/std::vector/etc?

The current rule is: We should use WTF::String/Vector/etc in Blink. We should use std::string/std::vector/etc otherwise.

My proposal is to change the rule to: We should use WTF::String/Vector/etc in necessary places in Blink. We should use std::string/std::vector/etc otherwise.

This enables us to pass around network::mojom::URLRequest everywhere. We can convert std::string/std::vector/etc in network::mojom::URLRequest to WTF::String/Vector/etc where necessary. In other words, I'm proposing to change the definition of the "boundary" from "Blink / other places" to "necessary places in Blink / other places". This will enable us to remove a huge amount of type mappings, type conversions, Blink public APIs and other unnecessary abstraction layers.

However, I'm understanding that this will need further discussion :)

[Makoto Shimazu]

Yeah as Yutaka mentioned, my proposal includes removal of blink::WebURLRequest.

I'm not quite sure if we can really replace blink::ResourceRequest with network::mojom::URLRequest because network::mojom::URLRequest contains only the info exposed to the network service.

As an intermediate state, there are a couple of options in my mind.

1. Restrict usages of std types as small as possible, like only in ResourceLoader (and ThrottlingURLLoader, which will be added during the onion soup work). Convert std types to WTF types as earlier as possible, at mostly the same places where currently we are doing (e.g. DidReceiveResponse).

2. Use network::mojom::URLRequest as an internal container of blink::ResourceRequest. This would be similar to blink::WrappedResourceRequest but it's opposite direction (non-Blink type is the internal data container, and Blink type is the view of the internal data).

Option 1 would change smaller number of codes, and we will continue to use blink::ResourceRequest and WTF types elsewhere.

Option 2 is probably a good intermediate step to go for what Haraken is suggesting.

So far I'm thinking that option 1 might be better until we reach a consensus because there'd be minimal friction of the API surface and we'd be able to split the task into two smaller steps.

2019年8月28日(水) 15:00 Kentaro Hara <har...@chromium.org>:

[Kentaro Hara]

I'm not quite sure if we can really replace blink::ResourceRequest with network::mojom::URLRequest because network::mojom::URLRequest contains only the info exposed to the network service.

Ah, I was thinking that blink::ResourceRequest is needed just for wrapping network::mojom::URLRequest because std types are not available in Blink. If blink::ResourceRequest needs more info, it makes a lot of sense to keep them separate.

+1 to the option 1. It aligns with the existing guideline of Blink (except you'll introduce non-Blink mojom types in some places in Blink).

(Just to clarify, my point in the previous email is that if you have to introduce some abstraction layers due to the difference between std types and WTF types, it should be fixed. This is why I'm even thinking about deprecating Blink mojom types by changing the definition of the "boundary". I need to think more if it's really a good idea or not though.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHg_a%2BturXD6DjWxTWd0nEZ3MWFUbuFddAXx9aV2kiTQLCFb2g%40mail.gmail.com.

[Lucas Gadani]

Thanks for putting that design doc. Your proposal LGTM, and maintaining blink::ResourceRequest makes sense for now, and if necessary, we can revisit that in the future once the onion soup work is completed.

The only request that I have is that this is properly documented through class comments, explaining why we are not using the blink mojom types, in order to avoid confusion for readers of the code.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jwNc7ZZie10sR3cgFHiLYHaXDwQqQ%2BZh4NuT91uEupnXg%40mail.gmail.com.

[Makoto Shimazu]

So far there seems to be no objection for this proposal. I'd like to continue in that direction.

Feel free to interrupt me if someone has any thoughts.

Thanks all!

2019年8月28日(水) 23:40 Lucas Gadani <l...@chromium.org>:

Thanks for putting that design doc. Your proposal LGTM, and maintaining blink::ResourceRequest makes sense for now, and if necessary, we can revisit that in the future once the onion soup work is completed.

The only request that I have is that this is properly documented through class comments, explaining why we are not using the blink mojom types, in order to avoid confusion for readers of the code.

Yes, we need some documentation about that. Thanks for raising it :)

[Yutaka Hirano]

As an intermediate state, there are a couple of options in my mind.
1. Restrict usages of std types as small as possible, like only in ResourceLoader (and ThrottlingURLLoader, which will be added during the onion soup work). Convert std types to WTF types as earlier as possible, at mostly the same places where currently we are doing (e.g. DidReceiveResponse).

Option 1 would change smaller number of codes, and we will continue to use blink::ResourceRequest and WTF types elsewhere.

I think we need more to deprecate WebURLRequest.

For example, FrameFetchContext::PrepareRequest (called from ResourceFetcher::PrepareRequest) takes a ResourceRequest and it passes the request to content::RenderFrameImpl::WillSendRequest through LocalFrameClient. If we want to deprecate WebURLRequest and avoid copying, I think we need to rewrite most if not all ResourceRequest occurrences in ResourceFetcher. ResourceFetcher interacts with many classes, and I want to understand the impact before we begin.

[Makoto Shimazu]

Thanks, that sounds reasonable concern.

I think currently there are two steps for the onion soup for loaders.

Yesterday I updated the design doc and actually I felt that the design around RenderFrameImpl still needs a bit more investigation.

As far as I'm currently understanding, there are a few ways to do that when removing WebURLRequest:

1. Exposing parameters RenderFrameImpl needs only.

2. Exposing network::mojom::URLRequest. This needs more investigation to know how much network::mojom::URLRequest will spread around.

2019年8月29日(木) 11:17 Yutaka Hirano <yhi...@chromium.org>:

[Makoto Shimazu]

Since my last response in this thread, I talked with kinuko@, yhirano@ about the design and also spent some amount of time to understand the current design and make the design of Loader's Onion Soup clearer.

Let me share some updates here.

Now I'm planning to split tasks into three steps as follows. 

  (1) Use network::mojom::URLLoader in Blink

  (2) Move most of the code for FetchContext in content to Blink

  (3) Replace WebURLRequest with network::mojom::URLRequest.

Note that step 1 and 2 are blocking the rest of service worker's onion soup.

In step 1, we are going to use ThrottlingURLLoader and add a URLLoader which uses network::mojom::URLLoader in Blink. It's placed in a  directory blink/renderer/platform/loader/fetch/url_loader, which is different from other parts of loader code (blink/renderer/platform/loader/fetch) to clarify they are use non-Blink types.

In step 3, I'm planning to expand the usage of network::mojom::URLRequest as a struct to represent network request exposed to outside of Blink. However, the usage will be limited because the network::mojom::URLRequest is basically going to be copied to blink::ResourceRequest immediately after going into Blink, and the only usage of network::mojom::URLRequest will be kept in the directory (b/r/p/l/f/url_loader). 

For reference, let me put the design docs for Loader's Onion Soup and eliminating WebURLRequest.

Onion Soup: Loaders: https://docs.google.com/document/d/18TVbhZ4MnGTCHYqeQtqz59SdTHP7CFqVyHWYbcD_d5A/edit

Planning: Eliminate WebURLRequest: https://docs.google.com/document/d/1C8S2e20imwIFxEIKgvqcgDGCErOIWCTB34ABAF07QAc/edit#

In addition to that, Minggang created a prototype CL: https://chromium-review.googlesource.com/c/chromium/src/+/1783716.

Feel free to add your comments/thoughts/concerns if you have. 

Thanks!

Makoto

2019年8月29日(木) 14:40 Makoto Shimazu <shi...@chromium.org>:

[Kentaro Hara]

LGTM from my side!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHg_a%2Bv6Sz%3DR2Wj8XNSPfhuKhb8urktQfUivz-Rdv9GjDEUKwg%40mail.gmail.com.