Split trusted and untrusted webui registries [chromium/src : main]
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 3 comments![]( "Open in Gerrit")
// PopulateChromeWebUIFrameInterfaceBrokers() registers BrowserInterfaceBrokers
// for each WebUI, these brokers are used to handle that WebUI's JavaScript
// Mojo.bindInterface calls.
void PopulateChromeWebUIFrameInterfaceBrokers(
content::WebUIBrowserInterfaceBrokerRegistry& registry);We can remove this one now, right?
// When possible, please one one of the Parts functions above and avoid making
// this function longer.nit: let's copy this comment below as well.
std::unique_ptr<PerWebUIBrowserInterfaceBroker> CreateInterfaceBroker(We could also override this method in UntrustedWebUIController. Have this one only check the trusted broker and have the UntrustedWebUIController override check the untrusted broker.
Related details
- Fred Shih
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jay Harris (Gerrit)
Jay Harris added 2 comments![]( "Open in Gerrit")
Hey, so this approach seems pretty sensible to me and should be a great improvement.
One thing is that now we're adding support for multiple registries its probably worth thinking about how we'd extend this to support a per-profile registries, as @dpapad needed in this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7113879/comment/641afd01_debac3fd/ (which I don't think we'll be able to do using base::NoDestructor).
I think once we have that piece in place, we'll have all the use cases of the old `RegisterWebUIControllerInterfaceBinder` in `WebUIBrowserInterfaceBrokerRegistry`
That said, lgtm, and I think its worth landing this as-is (but I'm not an owner, so can't +1 sorry).
// When possible, please one one of the Parts functions above and avoid making
// this function longer.nit: let's copy this comment below as well.
nit: "please one one of" --> "please use one of"
Related details
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 5 comments![]( "Open in Gerrit")
Hey, so this approach seems pretty sensible to me and should be a great improvement.
One thing is that now we're adding support for multiple registries its probably worth thinking about how we'd extend this to support a per-profile registries, as @dpapad needed in this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7113879/comment/641afd01_debac3fd/ (which I don't think we'll be able to do using base::NoDestructor).I think once we have that piece in place, we'll have all the use cases of the old `RegisterWebUIControllerInterfaceBinder` in `WebUIBrowserInterfaceBrokerRegistry`
That said, lgtm, and I think its worth landing this as-is (but I'm not an owner, so can't +1 sorry).
Interesting, good that we're breaking apart the registries. Per-profile registries doesn't seem too hard, although we'd need to consider several things:
1. What happens if the same webui is registered in multiple registries.
2. Where we'd put the registry. IIUC, the NoDestructor is really meant as an optimization
We'd probably be able to construct it each time if necessary, for a per-profile registry, this might not be too bad? I'd imagine we'd just need tighter rules around resolution if we're going to have multiple registries.
Thanks for taking a look! Always good to get more eyes on a change.
+dpapad@ who's apparently working in this area too!
// PopulateChromeWebUIFrameInterfaceBrokers() registers BrowserInterfaceBrokers
// for each WebUI, these brokers are used to handle that WebUI's JavaScript
// Mojo.bindInterface calls.
void PopulateChromeWebUIFrameInterfaceBrokers(
content::WebUIBrowserInterfaceBrokerRegistry& registry);We can remove this one now, right?
yep! forgot to remove this after copy and pasting D:
// When possible, please one one of the Parts functions above and avoid making
// this function longer.nit: let's copy this comment below as well.
Done
std::unique_ptr<PerWebUIBrowserInterfaceBroker> CreateInterfaceBroker(Fred ShihWe could also override this method in UntrustedWebUIController. Have this one only check the trusted broker and have the UntrustedWebUIController override check the untrusted broker.
oh! I didn't realize this existed. I do think it's kind of weird to have UntrustedWebUIController inherit from the (implicitly trusted) WebUIController. Would it be reasonable to make WebUIController an ABC with a TrustedWebUIController and UntrustedWebUIController as implementations (in a followup CL)?
Made the change.
Related details
- Giovanni Ortuno Urquidi
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 3 comments![]( "Open in Gerrit")
Fred ShihHey, so this approach seems pretty sensible to me and should be a great improvement.
One thing is that now we're adding support for multiple registries its probably worth thinking about how we'd extend this to support a per-profile registries, as @dpapad needed in this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7113879/comment/641afd01_debac3fd/ (which I don't think we'll be able to do using base::NoDestructor).I think once we have that piece in place, we'll have all the use cases of the old `RegisterWebUIControllerInterfaceBinder` in `WebUIBrowserInterfaceBrokerRegistry`
That said, lgtm, and I think its worth landing this as-is (but I'm not an owner, so can't +1 sorry).
Interesting, good that we're breaking apart the registries. Per-profile registries doesn't seem too hard, although we'd need to consider several things:
1. What happens if the same webui is registered in multiple registries.
2. Where we'd put the registry. IIUC, the NoDestructor is really meant as an optimizationWe'd probably be able to construct it each time if necessary, for a per-profile registry, this might not be too bad? I'd imagine we'd just need tighter rules around resolution if we're going to have multiple registries.
Thanks for taking a look! Always good to get more eyes on a change.
I'm not sure why we ended up with a global registry. It might have been an oversight during the implementation or maybe just an optimization to avoid creating the registry on each navigation. Maybe qjw remembers. Original design doc: https://docs.google.com/document/d/1hvyYFBcobMzG6-HZHLuzaKuvirAPSdkBp5ThDR3Z4Ns/edit?usp=sharing&resourcekey=0-LBl1HzbJ2ZN3WfwcUNCOgQ
It would be pretty straightforward to make this into a browser context keyed service, but maybe it would be better to make this a per-render-frame-host (like the other binder map) or even per-webui-controller and keep it more scoped.
If we do make it more scoped, we could change ForWebUI and Add() to be no ops to reduce the overhead of creating the map for every navigation, basically:
1. Add an optional current_webui_type to the WebUIBrowserInterfaceBrokerRegistry constructor.
2. When ForWebUI<ControllerType>() is called to register binders, the registry will check if ControllerType matches current_webui_type.
* Match: It proceeds as normal, registering the binders.
* No Match: It returns a "dummy" helper that ignores all subsequent .Add<Interface>() calls.
3. In WebUIController::WebUIReadyToCommitNavigation, instead of using a global or shared registry, create a local registry specifically interested in this->GetType()
registry.AddGlobal(base::BindLambdaForTesting(Nice!
std::unique_ptr<PerWebUIBrowserInterfaceBroker> CreateInterfaceBroker(Fred ShihWe could also override this method in UntrustedWebUIController. Have this one only check the trusted broker and have the UntrustedWebUIController override check the untrusted broker.
oh! I didn't realize this existed. I do think it's kind of weird to have UntrustedWebUIController inherit from the (implicitly trusted) WebUIController. Would it be reasonable to make WebUIController an ABC with a TrustedWebUIController and UntrustedWebUIController as implementations (in a followup CL)?
Made the change.
I'm still paging back my WebUI knowledge here, but that seems reasonable.
I would go with BaseWebUIController and WebUIController/UntrustedWebUIController. Historically, we've used the absence of "Untrusted" to mean trusted e.g. chrome:// and chrome-untrusted://; it just keeps things less verbose, but not a super hard rule.
Related details
- Fred Shih
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Fred Shih
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
Fred ShihHey, so this approach seems pretty sensible to me and should be a great improvement.
One thing is that now we're adding support for multiple registries its probably worth thinking about how we'd extend this to support a per-profile registries, as @dpapad needed in this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7113879/comment/641afd01_debac3fd/ (which I don't think we'll be able to do using base::NoDestructor).I think once we have that piece in place, we'll have all the use cases of the old `RegisterWebUIControllerInterfaceBinder` in `WebUIBrowserInterfaceBrokerRegistry`
That said, lgtm, and I think its worth landing this as-is (but I'm not an owner, so can't +1 sorry).
Giovanni Ortuno UrquidiInteresting, good that we're breaking apart the registries. Per-profile registries doesn't seem too hard, although we'd need to consider several things:
1. What happens if the same webui is registered in multiple registries.
2. Where we'd put the registry. IIUC, the NoDestructor is really meant as an optimizationWe'd probably be able to construct it each time if necessary, for a per-profile registry, this might not be too bad? I'd imagine we'd just need tighter rules around resolution if we're going to have multiple registries.
Thanks for taking a look! Always good to get more eyes on a change.
I'm not sure why we ended up with a global registry. It might have been an oversight during the implementation or maybe just an optimization to avoid creating the registry on each navigation. Maybe qjw remembers. Original design doc: https://docs.google.com/document/d/1hvyYFBcobMzG6-HZHLuzaKuvirAPSdkBp5ThDR3Z4Ns/edit?usp=sharing&resourcekey=0-LBl1HzbJ2ZN3WfwcUNCOgQ
It would be pretty straightforward to make this into a browser context keyed service, but maybe it would be better to make this a per-render-frame-host (like the other binder map) or even per-webui-controller and keep it more scoped.
If we do make it more scoped, we could change ForWebUI and Add() to be no ops to reduce the overhead of creating the map for every navigation, basically:
1. Add an optional current_webui_type to the WebUIBrowserInterfaceBrokerRegistry constructor.
2. When ForWebUI<ControllerType>() is called to register binders, the registry will check if ControllerType matches current_webui_type.
* Match: It proceeds as normal, registering the binders.
* No Match: It returns a "dummy" helper that ignores all subsequent .Add<Interface>() calls.
3. In WebUIController::WebUIReadyToCommitNavigation, instead of using a global or shared registry, create a local registry specifically interested in this->GetType()
Another way to achieve what that CL chain is trying to do that would require a less fundamental change would be to add an optional "should_bind" `base::RepeatingCallback<bool(WebUIController)>` to `Add()`. We would call it before adding the binder to the internal map, so it only gets registered when it returns true.
Fred Shih (Gerrit)
Fred Shih voted and added 2 comments![]( "Open in Gerrit")
Votes added by Fred Shih
| Commit-Queue | +2 |
Fred ShihHey, so this approach seems pretty sensible to me and should be a great improvement.
One thing is that now we're adding support for multiple registries its probably worth thinking about how we'd extend this to support a per-profile registries, as @dpapad needed in this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7113879/comment/641afd01_debac3fd/ (which I don't think we'll be able to do using base::NoDestructor).I think once we have that piece in place, we'll have all the use cases of the old `RegisterWebUIControllerInterfaceBinder` in `WebUIBrowserInterfaceBrokerRegistry`
That said, lgtm, and I think its worth landing this as-is (but I'm not an owner, so can't +1 sorry).
Giovanni Ortuno UrquidiInteresting, good that we're breaking apart the registries. Per-profile registries doesn't seem too hard, although we'd need to consider several things:
1. What happens if the same webui is registered in multiple registries.
2. Where we'd put the registry. IIUC, the NoDestructor is really meant as an optimizationWe'd probably be able to construct it each time if necessary, for a per-profile registry, this might not be too bad? I'd imagine we'd just need tighter rules around resolution if we're going to have multiple registries.
Thanks for taking a look! Always good to get more eyes on a change.
Giovanni Ortuno UrquidiI'm not sure why we ended up with a global registry. It might have been an oversight during the implementation or maybe just an optimization to avoid creating the registry on each navigation. Maybe qjw remembers. Original design doc: https://docs.google.com/document/d/1hvyYFBcobMzG6-HZHLuzaKuvirAPSdkBp5ThDR3Z4Ns/edit?usp=sharing&resourcekey=0-LBl1HzbJ2ZN3WfwcUNCOgQ
It would be pretty straightforward to make this into a browser context keyed service, but maybe it would be better to make this a per-render-frame-host (like the other binder map) or even per-webui-controller and keep it more scoped.
If we do make it more scoped, we could change ForWebUI and Add() to be no ops to reduce the overhead of creating the map for every navigation, basically:
1. Add an optional current_webui_type to the WebUIBrowserInterfaceBrokerRegistry constructor.
2. When ForWebUI<ControllerType>() is called to register binders, the registry will check if ControllerType matches current_webui_type.
* Match: It proceeds as normal, registering the binders.
* No Match: It returns a "dummy" helper that ignores all subsequent .Add<Interface>() calls.
3. In WebUIController::WebUIReadyToCommitNavigation, instead of using a global or shared registry, create a local registry specifically interested in this->GetType()
Another way to achieve what that CL chain is trying to do that would require a less fundamental change would be to add an optional "should_bind" `base::RepeatingCallback<bool(WebUIController)>` to `Add()`. We would call it before adding the binder to the internal map, so it only gets registered when it returns true.
Ack -- I'll throw this in a crbug.com/464033838 for followup work. Thanks for the suggestions!
std::unique_ptr<PerWebUIBrowserInterfaceBroker> CreateInterfaceBroker(Fred ShihWe could also override this method in UntrustedWebUIController. Have this one only check the trusted broker and have the UntrustedWebUIController override check the untrusted broker.
oh! I didn't realize this existed. I do think it's kind of weird to have UntrustedWebUIController inherit from the (implicitly trusted) WebUIController. Would it be reasonable to make WebUIController an ABC with a TrustedWebUIController and UntrustedWebUIController as implementations (in a followup CL)?
Giovanni Ortuno UrquidiMade the change.
I'm still paging back my WebUI knowledge here, but that seems reasonable.
I would go with BaseWebUIController and WebUIController/UntrustedWebUIController. Historically, we've used the absence of "Untrusted" to mean trusted e.g. chrome:// and chrome-untrusted://; it just keeps things less verbose, but not a super hard rule.
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fred Shih
| Commit-Queue | +2 |
1 comment
registry.AddGlobal(base::BindLambdaForTesting(- Fred Shih
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih voted Commit-Queue+2![]( "Open in Gerrit")
- Ashley Prasad
- Chromium UI WebUI Reviews
- Dmitry Gozman
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 1 comment![]( "Open in Gerrit")
moving dpapad@ to reviewer for ui/webui/...
Related details
- Ashley Prasad
- Demetrios Papadopoulos
- Dmitry Gozman
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 1 comment![]( "Open in Gerrit")
- Demetrios Papadopoulos
- Dmitry Gozman
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted and added 3 comments![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Code-Review | +1 |
3 comments
LGTM although I am not too knowledgeable about these parts.
work is here: crbug.com/463472166Let's put this is the `Bug` line too?
Should this be linked to a bug? Or is there no bug tracking the general problem being fixed?
Or if this is helping with https://issues.chromium.org/issues/452983498 as a side-effect should we at least link to that?
Related details
- Dmitry Gozman
- Fred Shih
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih voted and added 2 comments![]( "Open in Gerrit")
Votes added by Fred Shih
| Commit-Queue | +1 |
2 comments
Let's put this is the `Bug` line too?
Done
Should this be linked to a bug? Or is there no bug tracking the general problem being fixed?
Or if this is helping with https://issues.chromium.org/issues/452983498 as a side-effect should we at least link to that?
I linked it to the original bug which introduced AddGlobal to the registry because it's a followup work which fixed globals being added to both trusted and untrusted.
Related details
- Dmitry Gozman
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitry Gozman (Gerrit)
Dmitry Gozman added 7 comments![]( "Open in Gerrit")
virtual void RegisterUntrustedWebUIInterfaceBrokers(This method is never called in `content`, so it should not be here.
To quote [content API guidelines](https://source.chromium.org/chromium/chromium/src/+/main:content/public/README.md):
```
Methods in the API should be there because either content is calling out to its embedder, or the embedder is calling to content. There shouldn't be any methods which are used to call from the embedder to the embedder.
```
WebUIBrowserInterfaceBrokerRegistry(Why do we now need to copy the registry? That sounds counter-intuitive to me. I'd assume it's moved around if needed, not copied.
ContentClient* content_client);This should be `ContentBrowserClient`, not `ContentClient`, because we are talking about `content/public/browser` classes that are only used in the browser.
virtual std::unique_ptr<PerWebUIBrowserInterfaceBroker> CreateInterfaceBroker(It seems like there are [no protected methods in `content/public`](https://source.chromium.org/search?q=%22protected:%22%20f:content%2Fpublic%20-f:content%2Fpublic%2Ftest&sq=), except for constructors/destructors of pure abstract interfaces. Can we keep this public?
virtual std::unique_ptr<PerWebUIBrowserInterfaceBroker> CreateInterfaceBroker(I wonder why do we have two ways for content to ask embedder to create interface brokers - one here, and another in `ContentBrowserClient::RegisterTrustedWebUIInterfaceBrokers`? Having just one would seem to be enough? That would certainly make things easier to understand.
return GetTrustedWebUIBrowserInterfaceBrokerRegistry(content_client)Having non-pure interfaces with some partial implementation is against the [content API guidelines](https://source.chromium.org/chromium/chromium/src/+/main:content/public/README.md):
```
//content/public should contain only interfaces, enums, structs and (rarely) static functions
```
I'd recommend to rename this into `CreateUntrustedInterfaceBroker()` or something like that, leave it no-op in `WebUIController`, and call it alongside the trusted registry somewhere in content impl.
void WebUIController::WebUIReadyToCommitNavigation(To be honest, I think this method violates content API guidelines as well. I'd prefer to store `broker_` somewhere in `content::WebUIImpl` and update it there directly from `WebUIMainFrameObserver::ReadyToCommitNavigation`. This way, interface broker business stays in content impl, and we get the (almost) pure interface `WebUIController` for embedders to implement.
Related details
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 10 comments![]( "Open in Gerrit")
@Dmitry, sorry it took so long to respond. It took me some time to understand //content guidelines better and untangle some of the violations. I appreciate the feedback!
auto* broker1 =
web_contents->GetWebUI()->GetController()->broker_for_testing();I don't think these tests are interesting. We can already see that a new broker is created for each new web ui, because it is referenced by a unique_ptr. I don't think it makes a lot of sense to verify that a new instance of WebUIController does actually create a new broker.
virtual void RegisterUntrustedWebUIInterfaceBrokers(This method is never called in `content`, so it should not be here.
To quote [content API guidelines](https://source.chromium.org/chromium/chromium/src/+/main:content/public/README.md):
```
Methods in the API should be there because either content is calling out to its embedder, or the embedder is calling to content. There shouldn't be any methods which are used to call from the embedder to the embedder.
```
Done
WebUIBrowserInterfaceBrokerRegistry(Why do we now need to copy the registry? That sounds counter-intuitive to me. I'd assume it's moved around if needed, not copied.
it's mainly because of the static (global) init, which I'd like to get rid of too. I changed it to a unique_ptr and got rid of this copy construction.
BTW -- just to pick your brain, doesn't this class violates the API guideline for //content/public because it's a concrete impl and not an interface?
virtual TrustPolicy GetTrustPolicy();@Dmitry: I know that this still violates the content guideline. I would like to fix this, but that would trigger a lot more changes, because the browser directly instantiates WebUI. What I'd like to do is:
1. Make this class pure virtual.
2. Create a TrustedWebUIController and UntrustedWebUIController subclasses.
3. Swap over all the existing instantiation sites in the browser to either of the two classes.
Unfortunately that will touch a lot more files, so I'd like to leave it to a followup CL. I did get rid of the WebUIReadyToCommitNavigation method though :)
This should be `ContentBrowserClient`, not `ContentClient`, because we are talking about `content/public/browser` classes that are only used in the browser.
No longer needed.
virtual std::unique_ptr<PerWebUIBrowserInterfaceBroker> CreateInterfaceBroker(I wonder why do we have two ways for content to ask embedder to create interface brokers - one here, and another in `ContentBrowserClient::RegisterTrustedWebUIInterfaceBrokers`? Having just one would seem to be enough? That would certainly make things easier to understand.
No longer needed.
virtual std::unique_ptr<PerWebUIBrowserInterfaceBroker> CreateInterfaceBroker(It seems like there are [no protected methods in `content/public`](https://source.chromium.org/search?q=%22protected:%22%20f:content%2Fpublic%20-f:content%2Fpublic%2Ftest&sq=), except for constructors/destructors of pure abstract interfaces. Can we keep this public?
No longer needed.
return GetTrustedWebUIBrowserInterfaceBrokerRegistry(content_client)Having non-pure interfaces with some partial implementation is against the [content API guidelines](https://source.chromium.org/chromium/chromium/src/+/main:content/public/README.md):
```
//content/public should contain only interfaces, enums, structs and (rarely) static functions
```I'd recommend to rename this into `CreateUntrustedInterfaceBroker()` or something like that, leave it no-op in `WebUIController`, and call it alongside the trusted registry somewhere in content impl.
I moved the registry stuff to the contents impl. I think that makes a lot more sense, because it is a bizzare that web contents controller is creating a broker for itself.
To be honest, I think this method violates content API guidelines as well. I'd prefer to store `broker_` somewhere in `content::WebUIImpl` and update it there directly from `WebUIMainFrameObserver::ReadyToCommitNavigation`. This way, interface broker business stays in content impl, and we get the (almost) pure interface `WebUIController` for embedders to implement.
I think that makes a lot of sense! Moved this over to web ui. Now the web ui is responsible for wiring up the controller with the content.
Related details
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jay Harris (Gerrit)
Jay Harris voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jay Harris
| Code-Review | +1 |
2 comments
// TODO(crbug.com/464033838): we should not allow subclasses to overrideCould we make this bug visible to people outside Google (can't view it with my chromium.org account)
virtual TrustPolicy GetTrustPolicy();@Dmitry: I know that this still violates the content guideline. I would like to fix this, but that would trigger a lot more changes, because the browser directly instantiates WebUI. What I'd like to do is:
1. Make this class pure virtual.
2. Create a TrustedWebUIController and UntrustedWebUIController subclasses.
3. Swap over all the existing instantiation sites in the browser to either of the two classes.Unfortunately that will touch a lot more files, so I'd like to leave it to a followup CL. I did get rid of the WebUIReadyToCommitNavigation method though :)
This would be great! Its always confused me that `UntrustedWebUIController` extends the (presumably trusted) `WebUIController`
Related details
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 2 comments![]( "Open in Gerrit")
broker_.reset();
controller_.reset();
remote_.reset();
receiver_.reset();Hmm I wonder why we manually delete these instead of changing the declaration order to destroy things in the right order.
virtual TrustPolicy GetTrustPolicy();Jay Harris@Dmitry: I know that this still violates the content guideline. I would like to fix this, but that would trigger a lot more changes, because the browser directly instantiates WebUI. What I'd like to do is:
1. Make this class pure virtual.
2. Create a TrustedWebUIController and UntrustedWebUIController subclasses.
3. Swap over all the existing instantiation sites in the browser to either of the two classes.Unfortunately that will touch a lot more files, so I'd like to leave it to a followup CL. I did get rid of the WebUIReadyToCommitNavigation method though :)
This would be great! Its always confused me that `UntrustedWebUIController` extends the (presumably trusted) `WebUIController`
So is the plan to eventually get rid of this method? How would WebUIImpl::GetRegistryFor() work in that world? Or is the plan to have TrustedWebUIController and UntrustedWebUIController override this method? Wouldn't that violate content guidelines?
Related details
- Dmitry Gozman
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitry Gozman (Gerrit)
Dmitry Gozman added 5 comments![]( "Open in Gerrit")
Thank you for following through, this looks almost ideal now!
Splitting into TrustedWebUIController and UntrustedWebUIController subclasses works as well. As I understand, that would be contained in chrome/ or ui/ entirely, and won't touch content at all, so I have no official say in that 😊
virtual void SetUpMojoInterfaceBroker() = 0;This method is not used outside of content (rightfully so), so declare it in `WebUIImpl` right away.
WebUIBrowserInterfaceBrokerRegistry(Fred ShihWhy do we now need to copy the registry? That sounds counter-intuitive to me. I'd assume it's moved around if needed, not copied.
it's mainly because of the static (global) init, which I'd like to get rid of too. I changed it to a unique_ptr and got rid of this copy construction.
BTW -- just to pick your brain, doesn't this class violates the API guideline for //content/public because it's a concrete impl and not an interface?
Yes, this one also violates the guidelines. However, sometime we have to put template methods in the header, and if those need a private field, the interface stops being a pure one. Perhaps it's possible to untangle this one, but I'm not sure.
friend class content::WebUIBrowserInterfaceBrokerRegistryPerhaps this friend is not needed anymore?
virtual TrustPolicy GetTrustPolicy();Jay Harris@Dmitry: I know that this still violates the content guideline. I would like to fix this, but that would trigger a lot more changes, because the browser directly instantiates WebUI. What I'd like to do is:
1. Make this class pure virtual.
2. Create a TrustedWebUIController and UntrustedWebUIController subclasses.
3. Swap over all the existing instantiation sites in the browser to either of the two classes.Unfortunately that will touch a lot more files, so I'd like to leave it to a followup CL. I did get rid of the WebUIReadyToCommitNavigation method though :)
Giovanni Ortuno UrquidiThis would be great! Its always confused me that `UntrustedWebUIController` extends the (presumably trusted) `WebUIController`
So is the plan to eventually get rid of this method? How would WebUIImpl::GetRegistryFor() work in that world? Or is the plan to have TrustedWebUIController and UntrustedWebUIController override this method? Wouldn't that violate content guidelines?
I think what we have in this CL is alright. We do sometimes allow minimal base implementation in content for embedder-implemented interface that provide common getters. This `web_ui()` getter and field are like that.
Another common example would be `WebContentsObserver` and other observer interfaces. Perhaps we can move constructor/destructor of `WebUIController` into `protected` section to align with observers pattern?
Related details
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 7 comments![]( "Open in Gerrit")
adding keren for the webui_browser changes.
// TODO(crbug.com/464033838): we should not allow subclasses to overrideCould we make this bug visible to people outside Google (can't view it with my chromium.org account)
Done
broker_.reset();
controller_.reset();
remote_.reset();
receiver_.reset();Hmm I wonder why we manually delete these instead of changing the declaration order to destroy things in the right order.
I don't know either, but it was a really nasty surprise :\ My guess is that the ownership model is screwed up, especially since WebUIs can be reused (which was in itself, another nasty surprise...)
virtual void SetUpMojoInterfaceBroker() = 0;This method is not used outside of content (rightfully so), so declare it in `WebUIImpl` right away.
Done
WebUIBrowserInterfaceBrokerRegistry(Fred ShihWhy do we now need to copy the registry? That sounds counter-intuitive to me. I'd assume it's moved around if needed, not copied.
Dmitry Gozmanit's mainly because of the static (global) init, which I'd like to get rid of too. I changed it to a unique_ptr and got rid of this copy construction.
BTW -- just to pick your brain, doesn't this class violates the API guideline for //content/public because it's a concrete impl and not an interface?
Yes, this one also violates the guidelines. However, sometime we have to put template methods in the header, and if those need a private field, the interface stops being a pure one. Perhaps it's possible to untangle this one, but I'm not sure.
Ah I see, just wanted to verify my understanding. Thank you!
friend class content::WebUIBrowserInterfaceBrokerRegistryPerhaps this friend is not needed anymore?
Unfortunately not, because this depends where the macro is invoked (yuck...). If this was in the private section, then the registry would be doing a private access. I definitely want to get rid of the macro here and below..
virtual TrustPolicy GetTrustPolicy();Jay Harris@Dmitry: I know that this still violates the content guideline. I would like to fix this, but that would trigger a lot more changes, because the browser directly instantiates WebUI. What I'd like to do is:
1. Make this class pure virtual.
2. Create a TrustedWebUIController and UntrustedWebUIController subclasses.
3. Swap over all the existing instantiation sites in the browser to either of the two classes.Unfortunately that will touch a lot more files, so I'd like to leave it to a followup CL. I did get rid of the WebUIReadyToCommitNavigation method though :)
Giovanni Ortuno UrquidiThis would be great! Its always confused me that `UntrustedWebUIController` extends the (presumably trusted) `WebUIController`
Dmitry GozmanSo is the plan to eventually get rid of this method? How would WebUIImpl::GetRegistryFor() work in that world? Or is the plan to have TrustedWebUIController and UntrustedWebUIController override this method? Wouldn't that violate content guidelines?
I think what we have in this CL is alright. We do sometimes allow minimal base implementation in content for embedder-implemented interface that provide common getters. This `web_ui()` getter and field are like that.
Another common example would be `WebContentsObserver` and other observer interfaces. Perhaps we can move constructor/destructor of `WebUIController` into `protected` section to align with observers pattern?
My plan is to make this method pure virtual and have the new subclasses override it. If I understand the intent of this class correctly, it's really meant to interface webui implementers with the WebUI class.
Eventually I'd like to combine off the trust type and all the binding stuff into its own configuration. It doesn't really make sense for client code to be able to configure all these settings. For example, we really shouldn't allow kWebUI (which enables chrome.send) when the trust policy is kUntrusted.
Related details
- Demetrios Papadopoulos
- Dmitry Gozman
- Giovanni Ortuno Urquidi
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 2 comments![]( "Open in Gerrit")
#include "content/public/browser/web_ui.h"I don't seem to find where this header is used.
return content::WebUIController::TrustPolicy::kUntrusted;This should be trusted - WebUIBrowserUI runs `chrome://webui-browser`.
Related details
- Demetrios Papadopoulos
- Dmitry Gozman
- Fred Shih
- Giovanni Ortuno Urquidi
- Jay Harris
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |