content: Add URLLoaderFactory able to serve resources in-process [chromium/src : main]
Alex Yang (Gerrit)
Alex Yang added 1 comment![]( "Open in Gerrit")
Hi Daniel, I've moved the resource loader implementation from Blink to //content/renderer as we discussed. Please take a look. Thanks!
Related details
- Daniel Cheng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 1 comment![]( "Open in Gerrit")
Alex YangI know I originally gave the recommendation of just implementing it directly in Blink, but the implementation looks kind of awkward to me. Are there other URL loader factories like this that are already implemented in Blink? Or are most of them implemented in //content today?
Alex YangThere are a bunch at //content/browser/loader [1], those are somewhat different from this one because they run in the browser process. I don't know of any that are implemented in Blink.
Alex YangAn alternate approach I was considering before I reached out to you and others on the Blink team for guidance was to put the resource loader implementation in //content/renderer/. It was more work so I dropped it when I got the ok to leave it in Blink. I can revisit it but I may need additional guidance on how to update the public Blink API to accommodate something like that.
Moved loader out of Blink as discussed offline
Related details
- Daniel Cheng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 12 comments![]( "Open in Gerrit")
I've been thinking about the integration overall. I don't love the current way it's written because we add a special case to ChildURLLoaderFactoryBundle just for `chrome` even though we already have `scheme_specific_factories_`.
This means that there are two different ways to gate requests to the `chrome` scheme now, and I think that is worse.
1. The original gating is based off of `scheme_specific_factories_`; if a factory for the `chrome` scheme isn't present in the map, then we just can't satisfy requests for the `chrome` scheme.
2. But now we also have `local_resource_loader_factory_` which is populated using a different IPC and gated on a different set of checks.
It's hard to make sure that these stay in sync.
Have you considered if there's any way to reuse `scheme_specific_factories_` here? It would require some changes from the original CLs to make it work, but if we plumb it across with `URLLoaderFactoryBundle`, I feel like that would better ensure everything always stays in sync: https://source.chromium.org/search?q=subresource_loader_factories%20f:mojom$&ss=chromium
The tricky part here is that the map owns a `mojo::Remote`. We can:
- either have `mojo::Remote` be in-process and just have that be an additional async call or
- add some indirection so that there could be an in-process facade using `URLLoaderFactory*` directly instead of `mojo::Remote<URLLoaderFactory>`.
However, I think the first option would be better unless we find out that even an in-process async call is bad for performance.
return ranges[0];Just curious, but is this a cleanroom implementation or taken from something else? Wondering if this snippet should be shared somehow (perhaps in a separate CL)
std::map<const std::string, std::string> replacement_strings)It's unusual to specify `const` in a map key, as it's implicitly const. Is this needed?
std::map<url::Origin, std::unique_ptr<Source>> sources)i.e. this one doesn't have `const` on the key type.
url::Origin::CreateFromNormalizedTuple("chrome", source->name, 0);I am not very comfortable with doing this. Can we just pass an origin directly over IPC? Is there any place where the WebUI would already have a tuple? Does WebUI have any helper for this sort of thing?
void LocalResourceURLLoaderFactoryImpl::CreateLoaderAndStart(Is there any world where it'd make sense to combine CreateLoaderAndStart() and CanServe(), i.e. `MaybeCreateLoaderAndStart()` and have it returning a PendingReceiver?
mojo::PendingRemote<network::mojom::URLLoaderClient> client) const {Worth adding `DCHECK(CanServce(request))` here?
base::span<uint8_t> to_buffer;
MojoResult result = pipe_producer_handle->BeginWriteData(
output_size, MOJO_WRITE_DATA_FLAG_NONE, to_buffer);
CHECK_EQ(result, MOJO_RESULT_OK);
CHECK_GE(to_buffer.size(), output_size);
CHECK_LE(output_offset + output_size, bytes.size());
to_buffer.copy_prefix_from(base::as_bytes(
base::make_span(bytes).subspan(output_offset, output_size)));
result = pipe_producer_handle->EndWriteData(output_size);
CHECK_EQ(result, MOJO_RESULT_OK);
resource_response->content_length = output_size;Would it make sense to use `BlockingCopyFromString()` from https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/cpp/system/data_pipe_utils.h;l=21;drc=d387f092f963cdceb9568d3a589686f7cf0e5986?
// Reply to loader client.
mojo::Remote<network::mojom::URLLoaderClient> client_remote(
std::move(client));
client_remote->OnReceiveResponse(std::move(resource_response),
std::move(pipe_consumer_handle),
std::nullopt);Would it make sense to reply to the client before doing the write?
void AddReplacementString(std::string key, std::string value) {Nit: either use const ref or use std::move() below. Same comment elsewhere this shows up
result = client.response_body().ReadData(`BlockingCopyFromString()`?
class LocalResourceURLLoaderFactoryClasses and public/protected methods in the Blink public API should have some documentation comments.
Related details
- Alex Yang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 5 comments![]( "Open in Gerrit")
I've been thinking about the integration overall. I don't love the current way it's written because we add a special case to ChildURLLoaderFactoryBundle just for `chrome` even though we already have `scheme_specific_factories_`.
This means that there are two different ways to gate requests to the `chrome` scheme now, and I think that is worse.
1. The original gating is based off of `scheme_specific_factories_`; if a factory for the `chrome` scheme isn't present in the map, then we just can't satisfy requests for the `chrome` scheme.
2. But now we also have `local_resource_loader_factory_` which is populated using a different IPC and gated on a different set of checks.It's hard to make sure that these stay in sync.
Have you considered if there's any way to reuse `scheme_specific_factories_` here? It would require some changes from the original CLs to make it work, but if we plumb it across with `URLLoaderFactoryBundle`, I feel like that would better ensure everything always stays in sync: https://source.chromium.org/search?q=subresource_loader_factories%20f:mojom$&ss=chromium
The tricky part here is that the map owns a `mojo::Remote`. We can:
- either have `mojo::Remote` be in-process and just have that be an additional async call or
- add some indirection so that there could be an in-process facade using `URLLoaderFactory*` directly instead of `mojo::Remote<URLLoaderFactory>`.
However, I think the first option would be better unless we find out that even an in-process async call is bad for performance.
Thanks for calling out `scheme_specific_factories_`, I wasn't aware of it. At a glance, it looks like it would make more sense to integrate there. I can look into it when I get back from PTO next week.
Responded to the larger comments, will address the rest later. TL;DR will probably spin off at least 2 CLs and eventually rebase this on top. Will pick this back up when I'm back from PTO (11/14)
return ranges[0];Just curious, but is this a cleanroom implementation or taken from something else? Wondering if this snippet should be shared somehow (perhaps in a separate CL)
It's approximately copied from here: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/web_ui_url_loader_factory.cc;l=213-228;drc=13d96e931eb929b66d26ead4e98a2243a4e5d2c6
I agree, sharing this somehow would be ideal. I'll explore that.
std::map<const std::string, std::string> replacement_strings)It's unusual to specify `const` in a map key, as it's implicitly const. Is this needed?
It's because ui::ReplaceTemplateExpressions takes map<const string, string>
url::Origin::CreateFromNormalizedTuple("chrome", source->name, 0);I am not very comfortable with doing this. Can we just pass an origin directly over IPC? Is there any place where the WebUI would already have a tuple? Does WebUI have any helper for this sort of thing?
We could create the origin on the browser side [1] and send that instead of the source name. I feel like that's also hacky for the same reason. But no, there is no place where the data source would already have a tuple. This is because URLDataManagerBackend (ab)uses the fact that a data source with no scheme in its name implicitly serves chrome://-scheme URLs [2].
I don't know of a helper for this. Speaking of helpers though, maybe WebUIDataSource should have an API to return the origin it serves; that way URLDataManagerBackend would not have to rely on the format of the source name. We would have to create that API and maybe refactor the normal browser-side codepath to use it.
[1] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/web_ui_impl.cc;l=85;drc=8f98cea7f548d3bb3e37f7773a52584ecfe857e3
[2] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/url_data_manager_backend.cc;l=145;drc=fc7ddc185b7f027dd7f4981ba8b2334c69a2e79c
Related details
- Daniel Cheng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 1 comment![]( "Open in Gerrit")
Alex YangI've been thinking about the integration overall. I don't love the current way it's written because we add a special case to ChildURLLoaderFactoryBundle just for `chrome` even though we already have `scheme_specific_factories_`.
This means that there are two different ways to gate requests to the `chrome` scheme now, and I think that is worse.
1. The original gating is based off of `scheme_specific_factories_`; if a factory for the `chrome` scheme isn't present in the map, then we just can't satisfy requests for the `chrome` scheme.
2. But now we also have `local_resource_loader_factory_` which is populated using a different IPC and gated on a different set of checks.It's hard to make sure that these stay in sync.
Have you considered if there's any way to reuse `scheme_specific_factories_` here? It would require some changes from the original CLs to make it work, but if we plumb it across with `URLLoaderFactoryBundle`, I feel like that would better ensure everything always stays in sync: https://source.chromium.org/search?q=subresource_loader_factories%20f:mojom$&ss=chromium
The tricky part here is that the map owns a `mojo::Remote`. We can:
- either have `mojo::Remote` be in-process and just have that be an additional async call or
- add some indirection so that there could be an in-process facade using `URLLoaderFactory*` directly instead of `mojo::Remote<URLLoaderFactory>`.
However, I think the first option would be better unless we find out that even an in-process async call is bad for performance.
Thanks for calling out `scheme_specific_factories_`, I wasn't aware of it. At a glance, it looks like it would make more sense to integrate there. I can look into it when I get back from PTO next week.
I'm in the process of understanding how this would work. I have two questions so far:
1. I think I see how we could plumb the LocalResourceLoaderConfig across in a URLLoaderFactoryBundle via UpdateSubresourceLoaderFactories. We would pass the LocalResourceLoaderConfig in the PendingURLLoaderFactoryBundle, and then in RenderFrameImpl we would construct the actual LocalResourceURLLoaderFactory and own it, and construct a mojo::Remote<URLLoaderFactory> from it and move it into scheme_specific_factories_. This would involve adding code to URLLoaderFactoryBundle::Update [1]. Is that sort of what you had in mind?
2. Re-using scheme_specific_factories_ would mean making the LocalResourceURLLoaderFactory implement the URLLoaderFactory interface. This is almost possible, but one thing that is stopping me is that LocalResourceURLLoaderFactory has an extra method, CanServe, which is used to preemptively check for URLs it can't serve and fallback to the default handling if it can't serve them. One way I can think of to accommodate this with scheme_specific_factories_ would be to add CanServe to the URLLoaderFactory interface, with a default implementation of always returning true, and override this in LocalResourceURLLoaderFactory with the real implementation. WDYT about this approach?
Alex Yang (Gerrit)
Alex Yang added 9 comments![]( "Open in Gerrit")
Alex YangJust curious, but is this a cleanroom implementation or taken from something else? Wondering if this snippet should be shared somehow (perhaps in a separate CL)
It's approximately copied from here: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/web_ui_url_loader_factory.cc;l=213-228;drc=13d96e931eb929b66d26ead4e98a2243a4e5d2c6
I agree, sharing this somehow would be ideal. I'll explore that.
This is now shared with WebUIURLLoaderFactory
url::Origin::CreateFromNormalizedTuple("chrome", source->name, 0);Alex YangI am not very comfortable with doing this. Can we just pass an origin directly over IPC? Is there any place where the WebUI would already have a tuple? Does WebUI have any helper for this sort of thing?
We could create the origin on the browser side [1] and send that instead of the source name. I feel like that's also hacky for the same reason. But no, there is no place where the data source would already have a tuple. This is because URLDataManagerBackend (ab)uses the fact that a data source with no scheme in its name implicitly serves chrome://-scheme URLs [2].
I don't know of a helper for this. Speaking of helpers though, maybe WebUIDataSource should have an API to return the origin it serves; that way URLDataManagerBackend would not have to rely on the format of the source name. We would have to create that API and maybe refactor the normal browser-side codepath to use it.
[1] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/web_ui_impl.cc;l=85;drc=8f98cea7f548d3bb3e37f7773a52584ecfe857e3
[2] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/url_data_manager_backend.cc;l=145;drc=fc7ddc185b7f027dd7f4981ba8b2334c69a2e79c
The origin is now created on the browser side and passed to the renderer process via IPC.
void LocalResourceURLLoaderFactoryImpl::CreateLoaderAndStart(Is there any world where it'd make sense to combine CreateLoaderAndStart() and CanServe(), i.e. `MaybeCreateLoaderAndStart()` and have it returning a PendingReceiver?
How would this work if I wanted to implement the URLLoaderFactory interface?
mojo::PendingRemote<network::mojom::URLLoaderClient> client) const {Alex YangWorth adding `DCHECK(CanServce(request))` here?
Done
base::span<uint8_t> to_buffer;
MojoResult result = pipe_producer_handle->BeginWriteData(
output_size, MOJO_WRITE_DATA_FLAG_NONE, to_buffer);
CHECK_EQ(result, MOJO_RESULT_OK);
CHECK_GE(to_buffer.size(), output_size);
CHECK_LE(output_offset + output_size, bytes.size());
to_buffer.copy_prefix_from(base::as_bytes(
base::make_span(bytes).subspan(output_offset, output_size)));
result = pipe_producer_handle->EndWriteData(output_size);
CHECK_EQ(result, MOJO_RESULT_OK);
resource_response->content_length = output_size;Would it make sense to use `BlockingCopyFromString()` from https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/cpp/system/data_pipe_utils.h;l=21;drc=d387f092f963cdceb9568d3a589686f7cf0e5986?
This code is now shared with WebUIURLLoaderFactory so I would say this is out of scope of this CL now.
// Reply to loader client.
mojo::Remote<network::mojom::URLLoaderClient> client_remote(
std::move(client));
client_remote->OnReceiveResponse(std::move(resource_response),
std::move(pipe_consumer_handle),
std::nullopt);Would it make sense to reply to the client before doing the write?
This code is now shared with WebUIURLLoaderFactory so I would say this is out of scope of this CL now.
void AddReplacementString(std::string key, std::string value) {Nit: either use const ref or use std::move() below. Same comment elsewhere this shows up
Done
result = client.response_body().ReadData(Alex Yang`BlockingCopyFromString()`?
Done
Classes and public/protected methods in the Blink public API should have some documentation comments.
Done
Related details
- Daniel Cheng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
Alex YangI've been thinking about the integration overall. I don't love the current way it's written because we add a special case to ChildURLLoaderFactoryBundle just for `chrome` even though we already have `scheme_specific_factories_`.
This means that there are two different ways to gate requests to the `chrome` scheme now, and I think that is worse.
1. The original gating is based off of `scheme_specific_factories_`; if a factory for the `chrome` scheme isn't present in the map, then we just can't satisfy requests for the `chrome` scheme.
2. But now we also have `local_resource_loader_factory_` which is populated using a different IPC and gated on a different set of checks.It's hard to make sure that these stay in sync.
Have you considered if there's any way to reuse `scheme_specific_factories_` here? It would require some changes from the original CLs to make it work, but if we plumb it across with `URLLoaderFactoryBundle`, I feel like that would better ensure everything always stays in sync: https://source.chromium.org/search?q=subresource_loader_factories%20f:mojom$&ss=chromium
The tricky part here is that the map owns a `mojo::Remote`. We can:
- either have `mojo::Remote` be in-process and just have that be an additional async call or
- add some indirection so that there could be an in-process facade using `URLLoaderFactory*` directly instead of `mojo::Remote<URLLoaderFactory>`.
However, I think the first option would be better unless we find out that even an in-process async call is bad for performance.
Alex YangThanks for calling out `scheme_specific_factories_`, I wasn't aware of it. At a glance, it looks like it would make more sense to integrate there. I can look into it when I get back from PTO next week.
I'm in the process of understanding how this would work. I have two questions so far:
1. I think I see how we could plumb the LocalResourceLoaderConfig across in a URLLoaderFactoryBundle via UpdateSubresourceLoaderFactories. We would pass the LocalResourceLoaderConfig in the PendingURLLoaderFactoryBundle, and then in RenderFrameImpl we would construct the actual LocalResourceURLLoaderFactory and own it, and construct a mojo::Remote<URLLoaderFactory> from it and move it into scheme_specific_factories_. This would involve adding code to URLLoaderFactoryBundle::Update [1]. Is that sort of what you had in mind?
2. Re-using scheme_specific_factories_ would mean making the LocalResourceURLLoaderFactory implement the URLLoaderFactory interface. This is almost possible, but one thing that is stopping me is that LocalResourceURLLoaderFactory has an extra method, CanServe, which is used to preemptively check for URLs it can't serve and fallback to the default handling if it can't serve them. One way I can think of to accommodate this with scheme_specific_factories_ would be to add CanServe to the URLLoaderFactory interface, with a default implementation of always returning true, and override this in LocalResourceURLLoaderFactory with the real implementation. WDYT about this approach?
1. Yes, I think something like that.
2. See the other part about falling back—ideally the fallback would be internal and not an external `CanServe()` method.
void LocalResourceURLLoaderFactoryImpl::CreateLoaderAndStart(Alex YangIs there any world where it'd make sense to combine CreateLoaderAndStart() and CanServe(), i.e. `MaybeCreateLoaderAndStart()` and have it returning a PendingReceiver?
How would this work if I wanted to implement the URLLoaderFactory interface?
In an optimally ideal world, you wouldn't need the delegation at all (i.e. the 'fallback that happens to make browser tests work' wouldn't be needed at all).
In a slightly ideal world, you'd be able to delegate internally to the original URLLoaderFactory for chrome:
Related details
- Alex Yang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
class LocalResourceURLLoaderFactoryImplWhat is the desire to implement this in content/renderer and not inside blink directly?
It is unclear having the implementation be inside content brings us.
Related details
- Alex Yang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 1 comment![]( "Open in Gerrit")
class LocalResourceURLLoaderFactoryImplWhat is the desire to implement this in content/renderer and not inside blink directly?
It is unclear having the implementation be inside content brings us.
An earlier iteration of this CL had the implementation in Blink: https://chromium-review.googlesource.com/c/chromium/src/+/5955670/7
It felt weird because we were making a lot of exceptions for it in Blink's type allowlist and there was no precedent for implementing a URLLoaderFactory in Blink, so I moved the implementation out of Blink. See this doc for context on this decision: https://docs.google.com/document/d/1fN0Z5OcKY5kLiMBFaOSC3a5__wRJhohVuiEC5tkNIMI/edit?usp=sharing&resourcekey=0-1tB7UQVP2vg2rO_YsGQyvA
Related details
- Dave Tapuska
Alex Yang (Gerrit)
Alex Yang added 2 comments![]( "Open in Gerrit")
Alex YangI've been thinking about the integration overall. I don't love the current way it's written because we add a special case to ChildURLLoaderFactoryBundle just for `chrome` even though we already have `scheme_specific_factories_`.
This means that there are two different ways to gate requests to the `chrome` scheme now, and I think that is worse.
1. The original gating is based off of `scheme_specific_factories_`; if a factory for the `chrome` scheme isn't present in the map, then we just can't satisfy requests for the `chrome` scheme.
2. But now we also have `local_resource_loader_factory_` which is populated using a different IPC and gated on a different set of checks.It's hard to make sure that these stay in sync.
Have you considered if there's any way to reuse `scheme_specific_factories_` here? It would require some changes from the original CLs to make it work, but if we plumb it across with `URLLoaderFactoryBundle`, I feel like that would better ensure everything always stays in sync: https://source.chromium.org/search?q=subresource_loader_factories%20f:mojom$&ss=chromium
The tricky part here is that the map owns a `mojo::Remote`. We can:
- either have `mojo::Remote` be in-process and just have that be an additional async call or
- add some indirection so that there could be an in-process facade using `URLLoaderFactory*` directly instead of `mojo::Remote<URLLoaderFactory>`.
However, I think the first option would be better unless we find out that even an in-process async call is bad for performance.
Alex YangThanks for calling out `scheme_specific_factories_`, I wasn't aware of it. At a glance, it looks like it would make more sense to integrate there. I can look into it when I get back from PTO next week.
Daniel ChengI'm in the process of understanding how this would work. I have two questions so far:
1. I think I see how we could plumb the LocalResourceLoaderConfig across in a URLLoaderFactoryBundle via UpdateSubresourceLoaderFactories. We would pass the LocalResourceLoaderConfig in the PendingURLLoaderFactoryBundle, and then in RenderFrameImpl we would construct the actual LocalResourceURLLoaderFactory and own it, and construct a mojo::Remote<URLLoaderFactory> from it and move it into scheme_specific_factories_. This would involve adding code to URLLoaderFactoryBundle::Update [1]. Is that sort of what you had in mind?
2. Re-using scheme_specific_factories_ would mean making the LocalResourceURLLoaderFactory implement the URLLoaderFactory interface. This is almost possible, but one thing that is stopping me is that LocalResourceURLLoaderFactory has an extra method, CanServe, which is used to preemptively check for URLs it can't serve and fallback to the default handling if it can't serve them. One way I can think of to accommodate this with scheme_specific_factories_ would be to add CanServe to the URLLoaderFactory interface, with a default implementation of always returning true, and override this in LocalResourceURLLoaderFactory with the real implementation. WDYT about this approach?
1. Yes, I think something like that.
2. See the other part about falling back—ideally the fallback would be internal and not an external `CanServe()` method.
I wrote down what it would take to implement your suggestion here: https://docs.google.com/document/d/1rIDkAXZEmHPX4OKR7yqD5tQBPPti75Y1dbIcwDtgWzY/edit?resourcekey=0-HFAgyAgrwbDxBOGLbJl3-g&tab=t.0#heading=h.fldpq3jw2h8a
Determine where the pipe to the WebUIURLLoaderFactory is. It will be in one of 3 places: (1) already set as the fallback for an existing in-process URLLoaderFactory, (2) in the chrome:// scheme entry in scheme_specific_factories_, or (3) it is the default_factory_. Set the existing pipe as the fallback in the in-process URLLoaderFactory, then replace the existing pipe with a pipe to the in-process URLLoaderFactory via assignment.
Unfortunately it's complicated by two facts:
1. The ability for the in-process loader to fall back to the browser process is mandatory
2. The WebUIURLLoaderFactory can actually be in one of two places: scheme_specific_factories_ (rare) or default_factory_ (common case)
If and when WebUIURLLoaderFactory is consolidated into one place, it will be easier to set it as the fallback for the in-process loader. Until then, it will be messy to do what you're suggesting.
void LocalResourceURLLoaderFactoryImpl::CreateLoaderAndStart(Alex YangIs there any world where it'd make sense to combine CreateLoaderAndStart() and CanServe(), i.e. `MaybeCreateLoaderAndStart()` and have it returning a PendingReceiver?
Daniel ChengHow would this work if I wanted to implement the URLLoaderFactory interface?
In an optimally ideal world, you wouldn't need the delegation at all (i.e. the 'fallback that happens to make browser tests work' wouldn't be needed at all).
In a slightly ideal world, you'd be able to delegate internally to the original URLLoaderFactory for chrome:
As I said, the ability for the in-process loader to fall back to the browser process is mandatory. This is because
There are certain resources which the renderer process is not capable of serving in-process:
> 1. Dynamic resources that are generated on-the-fly in the browser process (e.g. strings.m.js, chrome://theme/colors.css)
2. Resources that reside in a pak file that is not memory-mapped in the renderer process (e.g. browser_tests.pak)
Related details
- Daniel Cheng
- Dave Tapuska
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
Have we explored special casing say a like data URLs are? (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_loader.cc;l=332;drc=5108636c70c0b08fdbeb57de2640a22e138f6685;bpv=1;bpt=1)
Going through a whole URL loader might be so much more machinery.. We already load stylesheets in blink from resource bundles... https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/data_resource_helper.cc;drc=5108636c70c0b08fdbeb57de2640a22e138f6685;bpv=1;bpt=1;l=18
Related details
- Alex Yang
- Daniel Cheng
Alex Yang (Gerrit)
Alex Yang added 1 comment![]( "Open in Gerrit")
Have we explored special casing say a like data URLs are? (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_loader.cc;l=332;drc=5108636c70c0b08fdbeb57de2640a22e138f6685;bpv=1;bpt=1)
Going through a whole URL loader might be so much more machinery.. We already load stylesheets in blink from resource bundles... https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/data_resource_helper.cc;drc=5108636c70c0b08fdbeb57de2640a22e138f6685;bpv=1;bpt=1;l=18
We did not explore that. Thank you for the suggestion. I took a brief look just now and my sense is that the special casing for handling data URLs in-process in ResourceLoader is scattered (there are 4 conditional branches that check if the protocol is data://) so adding similar special casing for a subset of chrome:// URLs would get messy I think.
Related details
- Daniel Cheng
- Dave Tapuska
Alex Yang (Gerrit)
Alex Yang added 3 comments![]( "Open in Gerrit")
New plumbing is done. See next CL: https://chromium-review.googlesource.com/c/chromium/src/+/5955234
Hi Daniel, I've completed the changes you requested. Please take another look. Thanks!
void LocalResourceURLLoaderFactoryImpl::CreateLoaderAndStart(Alex YangIs there any world where it'd make sense to combine CreateLoaderAndStart() and CanServe(), i.e. `MaybeCreateLoaderAndStart()` and have it returning a PendingReceiver?
Daniel ChengHow would this work if I wanted to implement the URLLoaderFactory interface?
Alex YangIn an optimally ideal world, you wouldn't need the delegation at all (i.e. the 'fallback that happens to make browser tests work' wouldn't be needed at all).
In a slightly ideal world, you'd be able to delegate internally to the original URLLoaderFactory for chrome:
As I said, the ability for the in-process loader to fall back to the browser process is mandatory. This is because
There are certain resources which the renderer process is not capable of serving in-process:
> 1. Dynamic resources that are generated on-the-fly in the browser process (e.g. strings.m.js, chrome://theme/colors.css)
2. Resources that reside in a pak file that is not memory-mapped in the renderer process (e.g. browser_tests.pak)
In patch set 15 I've internalized the fallback, which has made it possible for LocalResourceURLLoaderFactory to implement network::mojom::URLLoaderFactory.
Related details
- Daniel Cheng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 6 comments![]( "Open in Gerrit")
const std::map<url::Origin, std::unique_ptr<Source>> sources_;I'm a bit unclear on why this is a unique_ptr; Source could be movable, right?
std::map<const std::string, std::string> replacement_strings);Nit: no need for `const` here, as map keys are implicitly const always.
for (const auto& source : config->sources) {Can you help me understand why we need this instead of just moving `config` or `config->sources` as-is into a field? Aren't we already saving `config->sources`?
Edit: OK, I guess this is because the ui helpers don't take the right map type.
... if you want to land this as-is for now, that's fine, but I think there should be a comment/TODO + a followup bug to fix //ui's signature.
sources[origin] = std::make_unique<LocalResourceURLLoaderFactory::Source>(Do we expect multiple entries for the same origin? We don't, right, so we could just use insert({origin, ...})
std::make_unique<mojo::Receiver<network::mojom::URLLoaderFactory>>(this);receiver_ doesn't need to be a unique_ptr here; can just reset and then rebind if this is what we want.
base::ThreadPool::CreateSequencedTaskRunner(One followup idea is to just have `this` live on a non-main sequence so we don't need this thread hop at all. But that can wait.
Related details
- Alex Yang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 7 comments![]( "Open in Gerrit")
const std::map<url::Origin, std::unique_ptr<Source>> sources_;I'm a bit unclear on why this is a unique_ptr; Source could be movable, right?
I made it unique_ptr because I couldn't figure out why I was getting compiler errors when I tried to insert a value of this type into the map (where the type was std::map<url::Origin, Source>). I think that was because I didn't define a move constructor for it. So I've done that now and I made the type std::map<url::Origin, Source>
std::map<const std::string, std::string> replacement_strings);Nit: no need for `const` here, as map keys are implicitly const always.
Will fix in follow-up CL. Tracked in https://issues.chromium.org/384765582
Can you help me understand why we need this instead of just moving `config` or `config->sources` as-is into a field? Aren't we already saving `config->sources`?
Edit: OK, I guess this is because the ui helpers don't take the right map type.
... if you want to land this as-is for now, that's fine, but I think there should be a comment/TODO + a followup bug to fix //ui's signature.
Created a bug to fix //ui's signature: https://issues.chromium.org/384765582
Added a TODO comment here with link to bug
sources[origin] = std::make_unique<LocalResourceURLLoaderFactory::Source>(Do we expect multiple entries for the same origin? We don't, right, so we could just use insert({origin, ...})
Done
std::make_unique<mojo::Receiver<network::mojom::URLLoaderFactory>>(this);receiver_ doesn't need to be a unique_ptr here; can just reset and then rebind if this is what we want.
Done
base::ThreadPool::CreateSequencedTaskRunner(One followup idea is to just have `this` live on a non-main sequence so we don't need this thread hop at all. But that can wait.
I don't fully understand how that would work. Don't we still need to post the task in order to parallelize the task? Could you explain a bit more, or perhaps you could create a tracking bug that explains how it would work?
std::map<const std::string, std::string> replacement_strings)Alex YangIt's unusual to specify `const` in a map key, as it's implicitly const. Is this needed?
It's because ui::ReplaceTemplateExpressions takes map<const string, string>
Created bug to track this: https://issues.chromium.org/384765582
and I will submit a follow-up CL.
Related details
- Daniel Cheng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
2 comments
Source(Source&& other);Very minor nit: usually it's good practice to define the assignment operator too (even if it's not needed)
base::ThreadPool::CreateSequencedTaskRunner(Alex YangOne followup idea is to just have `this` live on a non-main sequence so we don't need this thread hop at all. But that can wait.
I don't fully understand how that would work. Don't we still need to post the task in order to parallelize the task? Could you explain a bit more, or perhaps you could create a tracking bug that explains how it would work?
When you bind a receiver (BindNewPipeAndPassRemote in this case), Mojo dispatches messages to the receiver on the sequence that BindNewPipeAndPassRemote was called on.
Usually I would say we can just own it with a `base::SequenceBound`, but it gets a bit trickier here because you're planning on reusing the fallback factory in certain cases.
It's fine to save this for a followup because of this complexity.
Related details
- Alex Yang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 4 comments![]( "Open in Gerrit")
Alex YangHave we explored special casing say a like data URLs are? (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_loader.cc;l=332;drc=5108636c70c0b08fdbeb57de2640a22e138f6685;bpv=1;bpt=1)
Going through a whole URL loader might be so much more machinery.. We already load stylesheets in blink from resource bundles... https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/data_resource_helper.cc;drc=5108636c70c0b08fdbeb57de2640a22e138f6685;bpv=1;bpt=1;l=18
We did not explore that. Thank you for the suggestion. I took a brief look just now and my sense is that the special casing for handling data URLs in-process in ResourceLoader is scattered (there are 4 conditional branches that check if the protocol is data://) so adding similar special casing for a subset of chrome:// URLs would get messy I think.
I followed up with Dave offline. We discussed that having this loader implementation in //content is the most straightforward and flexible path for right now, though in the future that may change when //content/renderer is eventually pared down. Prioritizing landing this as is since the primary purpose of this code as written is to gather data and not necessarily to land a finalized implementation right now. Marking resolved with no action.
Very minor nit: usually it's good practice to define the assignment operator too (even if it's not needed)
Done
base::ThreadPool::CreateSequencedTaskRunner(Alex YangOne followup idea is to just have `this` live on a non-main sequence so we don't need this thread hop at all. But that can wait.
Daniel ChengI don't fully understand how that would work. Don't we still need to post the task in order to parallelize the task? Could you explain a bit more, or perhaps you could create a tracking bug that explains how it would work?
When you bind a receiver (BindNewPipeAndPassRemote in this case), Mojo dispatches messages to the receiver on the sequence that BindNewPipeAndPassRemote was called on.
Usually I would say we can just own it with a `base::SequenceBound`, but it gets a bit trickier here because you're planning on reusing the fallback factory in certain cases.
It's fine to save this for a followup because of this complexity.
Created a bug to track this: https://issues.chromium.org/385317089
Added TODO comment with link to bug
Alex YangWhat is the desire to implement this in content/renderer and not inside blink directly?
It is unclear having the implementation be inside content brings us.
An earlier iteration of this CL had the implementation in Blink: https://chromium-review.googlesource.com/c/chromium/src/+/5955670/7
It felt weird because we were making a lot of exceptions for it in Blink's type allowlist and there was no precedent for implementing a URLLoaderFactory in Blink, so I moved the implementation out of Blink. See this doc for context on this decision: https://docs.google.com/document/d/1fN0Z5OcKY5kLiMBFaOSC3a5__wRJhohVuiEC5tkNIMI/edit?usp=sharing&resourcekey=0-1tB7UQVP2vg2rO_YsGQyvA
See other comment. Marking resolved with no action
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 1 comment![]( "Open in Gerrit")
Nasko, please take a look at changes to //content/test/*. Thanks!
Related details
- Nasko Oskov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 1 comment![]( "Open in Gerrit")
base::ThreadPool::CreateSequencedTaskRunner(Alex YangOne followup idea is to just have `this` live on a non-main sequence so we don't need this thread hop at all. But that can wait.
Daniel ChengI don't fully understand how that would work. Don't we still need to post the task in order to parallelize the task? Could you explain a bit more, or perhaps you could create a tracking bug that explains how it would work?
Alex YangWhen you bind a receiver (BindNewPipeAndPassRemote in this case), Mojo dispatches messages to the receiver on the sequence that BindNewPipeAndPassRemote was called on.
Usually I would say we can just own it with a `base::SequenceBound`, but it gets a bit trickier here because you're planning on reusing the fallback factory in certain cases.
It's fine to save this for a followup because of this complexity.
Created a bug to track this: https://issues.chromium.org/385317089
Added TODO comment with link to bug
Marking unresolved and working on this now, since I suspect this is causing test failures where sync requests get stuck waiting for a reply from the in-process loader because both are on the same sequence
Related details
- Daniel Cheng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alex Yang
| Commit-Queue | +1 |
Nasko, please take a look at changes to //content/test/*. Thanks!
base::ThreadPool::CreateSequencedTaskRunner(Alex YangOne followup idea is to just have `this` live on a non-main sequence so we don't need this thread hop at all. But that can wait.
Daniel ChengI don't fully understand how that would work. Don't we still need to post the task in order to parallelize the task? Could you explain a bit more, or perhaps you could create a tracking bug that explains how it would work?
Alex YangWhen you bind a receiver (BindNewPipeAndPassRemote in this case), Mojo dispatches messages to the receiver on the sequence that BindNewPipeAndPassRemote was called on.
Usually I would say we can just own it with a `base::SequenceBound`, but it gets a bit trickier here because you're planning on reusing the fallback factory in certain cases.
It's fine to save this for a followup because of this complexity.
Alex YangCreated a bug to track this: https://issues.chromium.org/385317089
Added TODO comment with link to bug
Marking unresolved and working on this now, since I suspect this is causing test failures where sync requests get stuck waiting for a reply from the in-process loader because both are on the same sequence
Done in later commit https://chromium-review.googlesource.com/c/chromium/src/+/5955234
and marked bug fixed
Related details
- Daniel Cheng
- Nasko Oskov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 6 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +0 |
6 comments
Just some minor nits.
const url::Origin origin = url::Origin::Create(request.url);
auto it = sources_.find(origin);
// CanServe should have been called before this point, which would have
// confirmed that there exists a source corresponding to the URL origin.
CHECK(it != sources_.end());
const blink::mojom::LocalResourceSourcePtr& source = it->second.source;
const std::map<const std::string, std::string>& replacement_strings =
it->second.replacement_strings;It might be worth refactoring this to eliminate the duplication between here and `CanServe()` in the future. For now it's fine though (and it'll make the parallelization CL easier to leave it as-is).
std::string mime_type;
if (net::GetMimeTypeFromFile(
base::FilePath::FromASCII(request.url.ExtractFileName()),
&mime_type)) {
url_response_head->mime_type = mime_type;
} else {
url_response_head->mime_type = "text/html";
}Should we try to combine this with https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/web_ui_data_source_impl.cc;l=340;drc=7ce981a3f10aa14a57e156fa52bd90953092d085 at some point?
(this is probably better as a followup though)
#include "gmock/gmock.h"I think this should be `testing/gmock/include/gmock/gmock.h`; then we might not need the DEPS change.
return std::nullopt;We don't want this to ever fail in the test right?
I guess ASSERT_TRUE is hard here, due to the non-void return, so we'd have to CHECK to make that work. And it's a bit stylistic, but the way it's written makes it look like maybe it should be allowed (and expected) to fail sometimes.
void UpdateLoaderFactory() {Style nit: fields should come after everything else per https://google.github.io/styleguide/cppguide.html#Declaration_Order
Related details
- Alex Yang
- Nasko Oskov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 5 comments![]( "Open in Gerrit")
const url::Origin origin = url::Origin::Create(request.url);
auto it = sources_.find(origin);
// CanServe should have been called before this point, which would have
// confirmed that there exists a source corresponding to the URL origin.
CHECK(it != sources_.end());
const blink::mojom::LocalResourceSourcePtr& source = it->second.source;
const std::map<const std::string, std::string>& replacement_strings =
it->second.replacement_strings;It might be worth refactoring this to eliminate the duplication between here and `CanServe()` in the future. For now it's fine though (and it'll make the parallelization CL easier to leave it as-is).
Created a bug to track this https://issues.chromium.org/388870472
std::string mime_type;
if (net::GetMimeTypeFromFile(
base::FilePath::FromASCII(request.url.ExtractFileName()),
&mime_type)) {
url_response_head->mime_type = mime_type;
} else {
url_response_head->mime_type = "text/html";
}Should we try to combine this with https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/web_ui_data_source_impl.cc;l=340;drc=7ce981a3f10aa14a57e156fa52bd90953092d085 at some point?
(this is probably better as a followup though)
Created a bug to track this https://issues.chromium.org/388871395
I think this should be `testing/gmock/include/gmock/gmock.h`; then we might not need the DEPS change.
Done
We don't want this to ever fail in the test right?
I guess ASSERT_TRUE is hard here, due to the non-void return, so we'd have to CHECK to make that work. And it's a bit stylistic, but the way it's written makes it look like maybe it should be allowed (and expected) to fail sometimes.
changed to use CHECK
Style nit: fields should come after everything else per https://google.github.io/styleguide/cppguide.html#Declaration_Order
- Daniel Cheng
- Nasko Oskov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
2 comments
base::MakeRefCounted<base::RefCountedString>(replaced_string);Nit: std::move() and avoid a copy here
Related details
- Alex Yang
- Nasko Oskov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 1 comment![]( "Open in Gerrit")
base::MakeRefCounted<base::RefCountedString>(replaced_string);Nit: std::move() and avoid a copy here
- Nasko Oskov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nasko Oskov
| Code-Review | +1 |
3 comments
// (whether the URL-to-ID mapping is present depends on what resourcenit: Unclosed parenthesis.
const url::Origin origin = url::Origin::Create(request.url);Should we put a CHECK here to ensure the scheme of the URL is a WebUI scheme or one supported by this URLLoaderFactory?
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Yang (Gerrit)
Alex Yang added 2 comments![]( "Open in Gerrit")
// (whether the URL-to-ID mapping is present depends on what resourceAlex Yangnit: Unclosed parenthesis.
Thanks, fixed.
const url::Origin origin = url::Origin::Create(request.url);Should we put a CHECK here to ensure the scheme of the URL is a WebUI scheme or one supported by this URLLoaderFactory?
I believe we can assert that the scheme of incoming request URLs is always going to be "chrome". This is only used for WebUIs that are not network-enabled, and also, "chrome-untrusted" goes to a scheme-specific factory I believe; not the default factory in the URLLoaderFactoryBundle, so they shouldn't end up here. Added the CHECK and will wait to see if any browser tests fail in the integration CL, in any case: https://chromium-review.googlesource.com/c/chromium/src/+/5955234
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nasko Oskov
| Code-Review | +1 |
1 comment
const url::Origin origin = url::Origin::Create(request.url);Alex YangShould we put a CHECK here to ensure the scheme of the URL is a WebUI scheme or one supported by this URLLoaderFactory?
I believe we can assert that the scheme of incoming request URLs is always going to be "chrome". This is only used for WebUIs that are not network-enabled, and also, "chrome-untrusted" goes to a scheme-specific factory I believe; not the default factory in the URLLoaderFactoryBundle, so they shouldn't end up here. Added the CHECK and will wait to see if any browser tests fail in the integration CL, in any case: https://chromium-review.googlesource.com/c/chromium/src/+/5955234
- Alex Yang
Alex Yang (Gerrit)
Alex Yang added 1 comment![]( "Open in Gerrit")
const url::Origin origin = url::Origin::Create(request.url);Alex YangShould we put a CHECK here to ensure the scheme of the URL is a WebUI scheme or one supported by this URLLoaderFactory?
Nasko OskovI believe we can assert that the scheme of incoming request URLs is always going to be "chrome". This is only used for WebUIs that are not network-enabled, and also, "chrome-untrusted" goes to a scheme-specific factory I believe; not the default factory in the URLLoaderFactoryBundle, so they shouldn't end up here. Added the CHECK and will wait to see if any browser tests fail in the integration CL, in any case: https://chromium-review.googlesource.com/c/chromium/src/+/5955234
👍
Looks like it failed on these 3 tests:
NetworkServiceBrowserTest.WebUIBindingsNoHttp
WebUIManagedInterfaceBrowserTest.RemoveIframe
WebUIManagedInterfaceBrowserTest.WebUIInIframe
because somehow it ended up handling "http:" and "chrome-untrusted:" -scheme URLs in those tests. Not sure how that's happening, but...
I've put the CHECK after CanServe returns true, because while it might receive requests with those schemes in some edge cases, it definitely should not decide to service them, and this would shut that down if that happens, because that would be very unexpected indeed.
Let me know if that addresses your concerns.
Related details
- Nasko Oskov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nasko Oskov
| Code-Review | +1 |
2 comments
Thanks for the extra details! Still LGTM
const url::Origin origin = url::Origin::Create(request.url);Alex YangShould we put a CHECK here to ensure the scheme of the URL is a WebUI scheme or one supported by this URLLoaderFactory?
Nasko OskovI believe we can assert that the scheme of incoming request URLs is always going to be "chrome". This is only used for WebUIs that are not network-enabled, and also, "chrome-untrusted" goes to a scheme-specific factory I believe; not the default factory in the URLLoaderFactoryBundle, so they shouldn't end up here. Added the CHECK and will wait to see if any browser tests fail in the integration CL, in any case: https://chromium-review.googlesource.com/c/chromium/src/+/5955234
Alex Yang👍
Looks like it failed on these 3 tests:
NetworkServiceBrowserTest.WebUIBindingsNoHttp
WebUIManagedInterfaceBrowserTest.RemoveIframe
WebUIManagedInterfaceBrowserTest.WebUIInIframebecause somehow it ended up handling "http:" and "chrome-untrusted:" -scheme URLs in those tests. Not sure how that's happening, but...
I've put the CHECK after CanServe returns true, because while it might receive requests with those schemes in some edge cases, it definitely should not decide to service them, and this would shut that down if that happens, because that would be very unexpected indeed.
Let me know if that addresses your concerns.
Thanks for investigating and adding the CHECK in CanServe! This is a very useful defense against bugs that can crop up.
Let's proceed with this and we can look independently into why this is getting http cases.
Related details
- Alex Yang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
content: Add URLLoaderFactory able to serve resources in-process
Implement a URLLoaderFactory with the ability to retrieve resources from
the memory-mapped 'resources.pak' file.
In a future commit, this URLLoaderFactory will be used in WebUI renderer
processes to load resources in-process, without having to request them
from the browser process.
- M content/renderer/BUILD.gn
- M content/renderer/DEPS
- A content/renderer/local_resource_url_loader_factory.cc
- A content/renderer/local_resource_url_loader_factory.h
- A content/renderer/local_resource_url_loader_factory_unittest.cc
- M content/test/BUILD.gn
- M content/test/test_content_client.cc
- M content/test/test_content_client.h
Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Nasko Oskov, +1 by Daniel Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |