Add browser controls mojo interface [chromium/src : main]
Tianyang Xu (Gerrit)
Tianyang Xu added 6 comments![]( "Open in Gerrit")
Add browser controls mojom interface - Rebacing from Renaming CLTianyang Xu```suggestion
Add browser controls mojo interface
```
There's no need to mention in a commit description that it's rebased, as it won't be once it's landed.
Gotcha, I was adding this to distinguish from the first CL. Removed
Create a new mojom interface to allow us to combine back, forward,Tianyang Xu```suggestion
Create a new mojo interface to allow us to combine back, forward,
```
Done
reload, and some other components in WebUI together, and replaceTianyang XuInstead of saying "to allow us to combine..." we should probably say "to provide basic browser controls (e.g. back, forward, reload etc)."
Done
the webui_toolbar.mojom.Tianyang XuInstead of saying "and replace the..." we should probably say "and seed it with...".
Done
Tianyang XuWe should mention that this CL also transitions the webui_toolbar to use this new interface.
Done
// The event flags for the click that might affect disposition of the reloadTianyang Xu```suggestion
// The event flags for the click that might affect disposition of a
```
Done
Related details
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
Removing Rakina due to duplicated reviewers and she is OOO
Related details
- Mingyu Lei
- Nasko Oskov
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 3 comments![]( "Open in Gerrit")
import "mojo/public/mojom/base/error.mojom";I don't see use of the above 3 imports. Can you help me understand why they are necessary?
interface BrowserControlsObserver {nit: This is a bit of unconventional naming, as it is an API the browser uses to invoke state changes in the renderer process. If it does that because implementation-wise it observes some changes, that's fine, but it is an implementation detail that is leaked in the name.
Related details
- Mingyu Lei
- Paul Jensen
- Tianyang Xu
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. |
Tianyang Xu (Gerrit)
Tianyang Xu added 3 comments![]( "Open in Gerrit")
I don't see use of the above 3 imports. Can you help me understand why they are necessary?
Good catch! Removed
interface BrowserControlsObserver {nit: This is a bit of unconventional naming, as it is an API the browser uses to invoke state changes in the renderer process. If it does that because implementation-wise it observes some changes, that's fine, but it is an implementation detail that is leaked in the name.
The "Observer" in the name of this interface, which is implemented by the renderer, refers to the renderer observing browser events, rather than referring to a possible implementation detail where the browser observes and forwards these events.
Related details
- Mingyu Lei
- Nasko Oskov
- Paul Jensen
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. |
Nasko Oskov (Gerrit)
Nasko Oskov added 1 comment![]( "Open in Gerrit")
interface BrowserControlsObserver {Tianyang Xunit: This is a bit of unconventional naming, as it is an API the browser uses to invoke state changes in the renderer process. If it does that because implementation-wise it observes some changes, that's fine, but it is an implementation detail that is leaked in the name.
The "Observer" in the name of this interface, which is implemented by the renderer, refers to the renderer observing browser events, rather than referring to a possible implementation detail where the browser observes and forwards these events.
But the renderer is not actually observing the events in the browser. It doesn't subscribe to any observation that I can see. The browser process pushes the updates to the renderer and it responds to those.
Related details
- Mingyu Lei
- Paul Jensen
- Tianyang Xu
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. |
Paul Jensen (Gerrit)
Paul Jensen added 1 comment![]( "Open in Gerrit")
interface BrowserControlsObserver {Tianyang Xunit: This is a bit of unconventional naming, as it is an API the browser uses to invoke state changes in the renderer process. If it does that because implementation-wise it observes some changes, that's fine, but it is an implementation detail that is leaked in the name.
Nasko OskovThe "Observer" in the name of this interface, which is implemented by the renderer, refers to the renderer observing browser events, rather than referring to a possible implementation detail where the browser observes and forwards these events.
But the renderer is not actually observing the events in the browser. It doesn't subscribe to any observation that I can see. The browser process pushes the updates to the renderer and it responds to those.
Hmm, I felt like Observer was a good name for something that received pushed updates/events. Can you advise us on an alternate name suggestion?
In the deleted webui_toolbar.mojom they used to be called Page & PageHandler, but we received feedback that we should transition them to Observer & Service, especially because in their new shared location they will be used by multiple WebUI pages so the reference to a singular "Page" wasn't appropriate.
Related details
- Mingyu Lei
- Tianyang Xu
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. |
Nasko Oskov (Gerrit)
Nasko Oskov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nasko Oskov
| Code-Review | +1 |
2 comments
interface BrowserControlsObserver {Tianyang Xunit: This is a bit of unconventional naming, as it is an API the browser uses to invoke state changes in the renderer process. If it does that because implementation-wise it observes some changes, that's fine, but it is an implementation detail that is leaked in the name.
Nasko OskovThe "Observer" in the name of this interface, which is implemented by the renderer, refers to the renderer observing browser events, rather than referring to a possible implementation detail where the browser observes and forwards these events.
Paul JensenBut the renderer is not actually observing the events in the browser. It doesn't subscribe to any observation that I can see. The browser process pushes the updates to the renderer and it responds to those.
Hmm, I felt like Observer was a good name for something that received pushed updates/events. Can you advise us on an alternate name suggestion?
In the deleted webui_toolbar.mojom they used to be called Page & PageHandler, but we received feedback that we should transition them to Observer & Service, especially because in their new shared location they will be used by multiple WebUI pages so the reference to a singular "Page" wasn't appropriate.
Observer interfaces usually require an object interested in observing events to register and unregister and usually it involves `ObserverList` or similar. This is why this throws it off for me as it has none of that. It is an interface to communicate state changes (and I would assume in the future other things) to the actual UI seen by the user. I think this is why `page` was used more universally, but I also am not a WebUI dev, so don't know what the current best practice is.
You all own the code, so I would defer to you, but it just confuses me that an observer interface does not observe anything :). Naming is hard 😜.
Related details
- Mingyu Lei
- Tianyang Xu
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. |
Theresa Sullivan (Gerrit)
Theresa Sullivan added 1 comment![]( "Open in Gerrit")
Hi Tianyang, is there someone in //chrome/browser/ui/waap/OWNERS in the owners set to approve?
The gerrit UI is indicating I'm on here (as //chrome/OWNERS) for approval, which if true, would ask that a closer OWNERS takes point on most of the review
Related details
- Daniel Cheng
- David Yeung
- Mingyu Lei
- Tianyang Xu
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. |
David Yeung (Gerrit)
David Yeung added 6 comments![]( "Open in Gerrit")
class WebUIToolbarPageHandler
: public browser_controls_api::mojom::BrowserControlsService {The idea of a service is that we can reuse it across multiple UIs, instead of strictly tied to the WebUIToolbarPageHandler. It should be able to stand up by itself and stateless to handle multiple clients. Take a look at [TabStripService](https://crsrc.org/c/chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_mojo_handler.cc;drc=07ba8b6a0f60aa5d68af93ef447b3979a4eb09f9;l=13) as an example. It exists in the browser process and we accept any clients (WebUIs) that bind to it.
Happy to discuss details elsewhere.
// BrowserControlsFactory is used by the WebUI page to bootstrap communication
// to/from the browser process.
interface BrowserControlsFactory {
// The WebUI calls this method when the page is first initialized.
// It binds a pending_remote for the observer and a pending_receiver for the
// service.
CreateBrowserControls(
pending_remote<BrowserControlsObserver> observer,
pending_receiver<BrowserControlsService> service);
};If we move to the service pattern, we do not need this factory anymore.
interface BrowserControlsObserver {Tianyang Xunit: This is a bit of unconventional naming, as it is an API the browser uses to invoke state changes in the renderer process. If it does that because implementation-wise it observes some changes, that's fine, but it is an implementation detail that is leaked in the name.
Nasko OskovThe "Observer" in the name of this interface, which is implemented by the renderer, refers to the renderer observing browser events, rather than referring to a possible implementation detail where the browser observes and forwards these events.
Paul JensenBut the renderer is not actually observing the events in the browser. It doesn't subscribe to any observation that I can see. The browser process pushes the updates to the renderer and it responds to those.
Nasko OskovHmm, I felt like Observer was a good name for something that received pushed updates/events. Can you advise us on an alternate name suggestion?
In the deleted webui_toolbar.mojom they used to be called Page & PageHandler, but we received feedback that we should transition them to Observer & Service, especially because in their new shared location they will be used by multiple WebUI pages so the reference to a singular "Page" wasn't appropriate.
Observer interfaces usually require an object interested in observing events to register and unregister and usually it involves `ObserverList` or similar. This is why this throws it off for me as it has none of that. It is an interface to communicate state changes (and I would assume in the future other things) to the actual UI seen by the user. I think this is why `page` was used more universally, but I also am not a WebUI dev, so don't know what the current best practice is.
You all own the code, so I would defer to you, but it just confuses me that an observer interface does not observe anything :). Naming is hard 😜.
+1 Observer should imply state changes - not UI actions. We should be making these events as stateless as possible.
For example, rename to `OnNavigationChanged(NavigationState state);` where the state can be `Unknown, Loading, NotLoading.` and then leave the actions to the various webui clients to handle. `Set` implies some type of action and falls back to the page/page handler pattern.
SetReloadButtonState(bool is_loading, bool is_menu_enabled);Is the menu tied to the reload button or is it across all browser controls? Can this be its own observer method like OnMenuEnabled?
Reload(bool ignore_cache, array<ClickDispositionFlag> flags);ClickDispositionFlag is more of a client/UI concept. What exactly changes for reload when we click on alt versus middle? Can we make it more specific to how reload will change?
enum ClickDispositionFlag {nit. Consider a kUnknown state.
Related details
- Daniel Cheng
- Mingyu Lei
- Tianyang Xu
Fergal Daly (Gerrit)
Fergal Daly added 3 comments![]( "Open in Gerrit")
import {BrowserControlsFactory, BrowserControlsObserverCallbackRouter, BrowserControlsServiceRemote} from './browser_controls_api.mojom-webui.js';Minor thing but will the formatter let you put these 1-per-line with a trailing ","? That way all future diffs are adding/removing lines.
class WebUIToolbarPageHandler
: public browser_controls_api::mojom::BrowserControlsService {The idea of a service is that we can reuse it across multiple UIs, instead of strictly tied to the WebUIToolbarPageHandler. It should be able to stand up by itself and stateless to handle multiple clients. Take a look at [TabStripService](https://crsrc.org/c/chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_mojo_handler.cc;drc=07ba8b6a0f60aa5d68af93ef447b3979a4eb09f9;l=13) as an example. It exists in the browser process and we accept any clients (WebUIs) that bind to it.
Happy to discuss details elsewhere.
It's not happening in this CL, so it's not blocking but is there a design doc somewhere that describes this full plan? "accept any clients (WebUIs)" sounds like it has security implications, e.g. maybe we should only accept clients that are specifically known to be THE toolbar renderer.
enum ClickDispositionFlag {nit. Consider a kUnknown state.
Is there a known use case for an unknown state? I would suggest not adding it until you have something that wants to send the value since handling it might be tricky.
Mingyu Lei (Gerrit)
Mingyu Lei added 3 comments![]( "Open in Gerrit")
module browser_controls_api.mojom;TBH I think the `xxx_api.mojom` is a bit awkward (also the `tabs_api`), generally speaking everything defined in the mojom can be called "API" right? To me both the tabs APIs and the browser controls APIs are just similar to the other interfaces between different components, and we don't usually put "api" in the mojom namespace.
I feel that it makes more sense calling the whole thing `//components/browser_controls`, and we have `tab_strip`, `reload`, ... under it. Maybe we should group the reload and back/forward together and call it `navigation`.
interface BrowserControlsObserver {Tianyang Xunit: This is a bit of unconventional naming, as it is an API the browser uses to invoke state changes in the renderer process. If it does that because implementation-wise it observes some changes, that's fine, but it is an implementation detail that is leaked in the name.
Nasko OskovThe "Observer" in the name of this interface, which is implemented by the renderer, refers to the renderer observing browser events, rather than referring to a possible implementation detail where the browser observes and forwards these events.
Paul JensenBut the renderer is not actually observing the events in the browser. It doesn't subscribe to any observation that I can see. The browser process pushes the updates to the renderer and it responds to those.
Nasko OskovHmm, I felt like Observer was a good name for something that received pushed updates/events. Can you advise us on an alternate name suggestion?
In the deleted webui_toolbar.mojom they used to be called Page & PageHandler, but we received feedback that we should transition them to Observer & Service, especially because in their new shared location they will be used by multiple WebUI pages so the reference to a singular "Page" wasn't appropriate.
David YeungObserver interfaces usually require an object interested in observing events to register and unregister and usually it involves `ObserverList` or similar. This is why this throws it off for me as it has none of that. It is an interface to communicate state changes (and I would assume in the future other things) to the actual UI seen by the user. I think this is why `page` was used more universally, but I also am not a WebUI dev, so don't know what the current best practice is.
You all own the code, so I would defer to you, but it just confuses me that an observer interface does not observe anything :). Naming is hard 😜.
+1 Observer should imply state changes - not UI actions. We should be making these events as stateless as possible.
For example, rename to `OnNavigationChanged(NavigationState state);` where the state can be `Unknown, Loading, NotLoading.` and then leave the actions to the various webui clients to handle. `Set` implies some type of action and falls back to the page/page handler pattern.
I think the "observer" sounds more like a proactive role, which registers itself to some source that would produce events, and reacts to those events. The source is simply notifying the observers that something happen, and it's the observer's responsibility to handle it in its own way.
In this case, `SetReloadButtonState` is more like an instruction from the browser side to update the state of the UI, so it feels a bit weird.
By the way, while implementing the first version of the mojom, I was simply following [the explainer](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/webui/webui_explainer.md#mojo-interface-definition), and I thought the PageHandler thing is a common pattern in the web UI implementation. I wonder what is special about this `BrowserControlsObserver`?
especially because in their new shared location they will be used by multiple WebUI pages
Do we have a design or plan of how this will look like in the future? Maybe it's good to link that in the description.
Reload(bool ignore_cache, array<ClickDispositionFlag> flags);ClickDispositionFlag is more of a client/UI concept. What exactly changes for reload when we click on alt versus middle? Can we make it more specific to how reload will change?
I think there are also modifier keys information. The handling logic for those are implemented in the browser side (e.g. ctrl+shift+click=hard reload), so it's better to just pass the flags as is.
Related details
- Daniel Cheng
- Tianyang Xu
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
// BrowserControlsFactory is used by the WebUI page to bootstrap communication
// to/from the browser process.
interface BrowserControlsFactory {
// The WebUI calls this method when the page is first initialized.
// It binds a pending_remote for the observer and a pending_receiver for the
// service.
CreateBrowserControls(
pending_remote<BrowserControlsObserver> observer,
pending_receiver<BrowserControlsService> service);
};If we move to the service pattern, we do not need this factory anymore.
What's the service pattern?
Paul Jensen (Gerrit)
Paul Jensen added 2 comments![]( "Open in Gerrit")
class WebUIToolbarPageHandler
: public browser_controls_api::mojom::BrowserControlsService {Fergal DalyThe idea of a service is that we can reuse it across multiple UIs, instead of strictly tied to the WebUIToolbarPageHandler. It should be able to stand up by itself and stateless to handle multiple clients. Take a look at [TabStripService](https://crsrc.org/c/chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_mojo_handler.cc;drc=07ba8b6a0f60aa5d68af93ef447b3979a4eb09f9;l=13) as an example. It exists in the browser process and we accept any clients (WebUIs) that bind to it.
Happy to discuss details elsewhere.
It's not happening in this CL, so it's not blocking but is there a design doc somewhere that describes this full plan? "accept any clients (WebUIs)" sounds like it has security implications, e.g. maybe we should only accept clients that are specifically known to be THE toolbar renderer.
David, that sounds like a good idea, perhaps we should rename this class to `BrowserControlsService`?
In the follow-up https://chromium-review.googlesource.com/c/chromium/src/+/7413625 perhaps we could move this file up a couple directories so that it can be used by webui_browser/ stuff too.
SetReloadButtonState(bool is_loading, bool is_menu_enabled);Is the menu tied to the reload button or is it across all browser controls? Can this be its own observer method like OnMenuEnabled?
It's tied to the reload button. Basically it gets enabled when DevTools is connected. Perhaps, going off your prior suggestion, we should rename this to `OnDevToolsStatusChanged(state)` where the state is `Connected, Disconnected`?
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
Hi Tianyang, is there someone in //chrome/browser/ui/waap/OWNERS in the owners set to approve?
The gerrit UI is indicating I'm on here (as //chrome/OWNERS) for approval, which if true, would ask that a closer OWNERS takes point on most of the review
Hi Theresa, sorry I didn't clarify that, I added you just for chrome/browser/DEPS I think.
Related details
- Daniel Cheng
- Theresa Sullivan
Theresa Sullivan (Gerrit)
Theresa Sullivan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Theresa Sullivan
| Code-Review | +1 |
1 comment
Tianyang XuHi Tianyang, is there someone in //chrome/browser/ui/waap/OWNERS in the owners set to approve?
The gerrit UI is indicating I'm on here (as //chrome/OWNERS) for approval, which if true, would ask that a closer OWNERS takes point on most of the review
Hi Theresa, sorry I didn't clarify that, I added you just for chrome/browser/DEPS I think.
chrome/browser/DEPS lgtm!
Related details
- Daniel Cheng
Fred Shih (Gerrit)
Fred Shih added 2 comments![]( "Open in Gerrit")
module browser_controls_api.mojom;TBH I think the `xxx_api.mojom` is a bit awkward (also the `tabs_api`), generally speaking everything defined in the mojom can be called "API" right? To me both the tabs APIs and the browser controls APIs are just similar to the other interfaces between different components, and we don't usually put "api" in the mojom namespace.
I feel that it makes more sense calling the whole thing `//components/browser_controls`, and we have `tab_strip`, `reload`, ... under it. Maybe we should group the reload and back/forward together and call it `navigation`.
For context: mojom interfaces were traditionally more bespoke IPCs between very specific clients (usually 1:1). The API in the name signals that this interface is intended to be used by any client, without any assumption in how the client is implemented. I would've preferred the name "service," but that term is way too overloaded in chrome...
the *_api.mojom is probably a bit of a stutter though. Maybe module browser_apis.navigation?
SetReloadButtonState(bool is_loading, bool is_menu_enabled);Paul JensenIs the menu tied to the reload button or is it across all browser controls? Can this be its own observer method like OnMenuEnabled?
It's tied to the reload button. Basically it gets enabled when DevTools is connected. Perhaps, going off your prior suggestion, we should rename this to `OnDevToolsStatusChanged(state)` where the state is `Connected, Disconnected`?
+1 to having a state to avoid a world where we have 10 different bools in this method :)
Related details
- Daniel Cheng
- Tianyang Xu
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. |
David Yeung (Gerrit)
David Yeung added 4 comments![]( "Open in Gerrit")
class WebUIToolbarPageHandler
: public browser_controls_api::mojom::BrowserControlsService {Fergal DalyThe idea of a service is that we can reuse it across multiple UIs, instead of strictly tied to the WebUIToolbarPageHandler. It should be able to stand up by itself and stateless to handle multiple clients. Take a look at [TabStripService](https://crsrc.org/c/chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_mojo_handler.cc;drc=07ba8b6a0f60aa5d68af93ef447b3979a4eb09f9;l=13) as an example. It exists in the browser process and we accept any clients (WebUIs) that bind to it.
Happy to discuss details elsewhere.
Paul JensenIt's not happening in this CL, so it's not blocking but is there a design doc somewhere that describes this full plan? "accept any clients (WebUIs)" sounds like it has security implications, e.g. maybe we should only accept clients that are specifically known to be THE toolbar renderer.
David, that sounds like a good idea, perhaps we should rename this class to `BrowserControlsService`?
In the follow-up https://chromium-review.googlesource.com/c/chromium/src/+/7413625 perhaps we could move this file up a couple directories so that it can be used by webui_browser/ stuff too.
Renaming to BrowserControlsService sounds good to me.
accept any clients (WebUIs)" sounds like it has security implications
We've talked with chrome security during TabStripApi and they've allowed it but it was limited to topchrome webuis. I think its a case by case if the service wants to limit what type of clients can be accepted. The main feedback is that it shouldn't be 1:1.
// BrowserControlsFactory is used by the WebUI page to bootstrap communication
// to/from the browser process.
interface BrowserControlsFactory {
// The WebUI calls this method when the page is first initialized.
// It binds a pending_remote for the observer and a pending_receiver for the
// service.
CreateBrowserControls(
pending_remote<BrowserControlsObserver> observer,
pending_receiver<BrowserControlsService> service);
};Giovanni Ortuno UrquidiIf we move to the service pattern, we do not need this factory anymore.
What's the service pattern?
Factories are used when the WebUI client creates their own remote/receivers and then passes it to the factory instance in the browser to bind. It is an empheral 1:1 relationship where the page is tied to a page handler.
We do not need this factory interface for TabStripService. Instead many different clients can bind to a persistent service component in the browser.
SetReloadButtonState(bool is_loading, bool is_menu_enabled);Paul JensenIs the menu tied to the reload button or is it across all browser controls? Can this be its own observer method like OnMenuEnabled?
Fred ShihIt's tied to the reload button. Basically it gets enabled when DevTools is connected. Perhaps, going off your prior suggestion, we should rename this to `OnDevToolsStatusChanged(state)` where the state is `Connected, Disconnected`?
+1 to having a state to avoid a world where we have 10 different bools in this method :)
Yup, I think that's a good suggestion.
Reload(bool ignore_cache, array<ClickDispositionFlag> flags);Mingyu LeiClickDispositionFlag is more of a client/UI concept. What exactly changes for reload when we click on alt versus middle? Can we make it more specific to how reload will change?
I think there are also modifier keys information. The handling logic for those are implemented in the browser side (e.g. ctrl+shift+click=hard reload), so it's better to just pass the flags as is.
If its hard reload, we could have flags like `ReloadType{ kNoCache, etc }` instead of click disposition. This abstracts the intent from the input type which makes the service api cleaner and input agnostic.
The service should really be focused on execution and then it doesn't need to worry about all the other triggers
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
There's already a lot of reviewers, so please don't block on me, but one comment about the factory thing.
// BrowserControlsFactory is used by the WebUI page to bootstrap communication
// to/from the browser process.
interface BrowserControlsFactory {
// The WebUI calls this method when the page is first initialized.
// It binds a pending_remote for the observer and a pending_receiver for the
// service.
CreateBrowserControls(
pending_remote<BrowserControlsObserver> observer,
pending_receiver<BrowserControlsService> service);
};Giovanni Ortuno UrquidiIf we move to the service pattern, we do not need this factory anymore.
David YeungWhat's the service pattern?
Factories are used when the WebUI client creates their own remote/receivers and then passes it to the factory instance in the browser to bind. It is an empheral 1:1 relationship where the page is tied to a page handler.
We do not need this factory interface for TabStripService. Instead many different clients can bind to a persistent service component in the browser.
How would that change this interface? Do you mean that the WebUI would bind the service interface directly?
While this does add a bit of indirection/boilerplate, there's value to "this remote/receiver pair should always be bound together". Switching to a singleton services shouldn't affect whether or not this is a useful invariant to maintain.
Related details
- Tianyang Xu
David Yeung (Gerrit)
David Yeung added 1 comment![]( "Open in Gerrit")
// BrowserControlsFactory is used by the WebUI page to bootstrap communication
// to/from the browser process.
interface BrowserControlsFactory {
// The WebUI calls this method when the page is first initialized.
// It binds a pending_remote for the observer and a pending_receiver for the
// service.
CreateBrowserControls(
pending_remote<BrowserControlsObserver> observer,
pending_receiver<BrowserControlsService> service);
};Giovanni Ortuno UrquidiIf we move to the service pattern, we do not need this factory anymore.
David YeungWhat's the service pattern?
Daniel ChengFactories are used when the WebUI client creates their own remote/receivers and then passes it to the factory instance in the browser to bind. It is an empheral 1:1 relationship where the page is tied to a page handler.
We do not need this factory interface for TabStripService. Instead many different clients can bind to a persistent service component in the browser.
How would that change this interface? Do you mean that the WebUI would bind the service interface directly?
While this does add a bit of indirection/boilerplate, there's value to "this remote/receiver pair should always be bound together". Switching to a singleton services shouldn't affect whether or not this is a useful invariant to maintain.
Yes, the WebUI would bind the service directly. You wouldn't need the factory interface. IMO the factory pattern is confusing. I think there is potential for a pattern that has less boilerplate - where clients do not need to know about remote/receivers/binding, and instead it is all taken care of by the service.
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
// BrowserControlsFactory is used by the WebUI page to bootstrap communication
// to/from the browser process.
interface BrowserControlsFactory {
// The WebUI calls this method when the page is first initialized.
// It binds a pending_remote for the observer and a pending_receiver for the
// service.
CreateBrowserControls(
pending_remote<BrowserControlsObserver> observer,
pending_receiver<BrowserControlsService> service);
};Giovanni Ortuno UrquidiIf we move to the service pattern, we do not need this factory anymore.
David YeungWhat's the service pattern?
Daniel ChengFactories are used when the WebUI client creates their own remote/receivers and then passes it to the factory instance in the browser to bind. It is an empheral 1:1 relationship where the page is tied to a page handler.
We do not need this factory interface for TabStripService. Instead many different clients can bind to a persistent service component in the browser.
David YeungHow would that change this interface? Do you mean that the WebUI would bind the service interface directly?
While this does add a bit of indirection/boilerplate, there's value to "this remote/receiver pair should always be bound together". Switching to a singleton services shouldn't affect whether or not this is a useful invariant to maintain.
Yes, the WebUI would bind the service directly. You wouldn't need the factory interface. IMO the factory pattern is confusing. I think there is potential for a pattern that has less boilerplate - where clients do not need to know about remote/receivers/binding, and instead it is all taken care of by the service.
I think there is potential for a pattern that has less boilerplate - where clients do not need to know about remote/receivers/binding, and instead it is all taken care of by the service.
Can you elaborate on what this looks like in practice and how it maintains the invariant dcheng mentioned?
I agree the factory pattern adds a lot of boilerplate but without it callers end up with interface implementations like this[[1]](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/searchbox/webui_omnibox_handler.cc;l=338-356;drc=4864b1e90893cbda6e50d8e155898acf62758e52) and crashes like this[[2]](https://issues.chromium.org/u/1/issues/468943376?pli=1).
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
Tianyang XuHi Tianyang, is there someone in //chrome/browser/ui/waap/OWNERS in the owners set to approve?
The gerrit UI is indicating I'm on here (as //chrome/OWNERS) for approval, which if true, would ask that a closer OWNERS takes point on most of the review
Theresa SullivanHi Theresa, sorry I didn't clarify that, I added you just for chrome/browser/DEPS I think.
chrome/browser/DEPS lgtm!
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
Move Daniel to CC list due to duplicated mojo reviewers
Related details
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. |
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
import {BrowserControlsFactory, BrowserControlsObserverCallbackRouter, BrowserControlsServiceRemote} from './browser_controls_api.mojom-webui.js';Minor thing but will the formatter let you put these 1-per-line with a trailing ","? That way all future diffs are adding/removing lines.
Yeah, this was corrected by the command:
`git cl format --js --no-python chrome/browser/resources`
in presubmit warning suggestion. Now I changed it back to three lines.
Related details
- Fergal Daly
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
import {BrowserControlsFactory, BrowserControlsObserverCallbackRouter, BrowserControlsServiceRemote} from './browser_controls_api.mojom-webui.js';Tianyang XuMinor thing but will the formatter let you put these 1-per-line with a trailing ","? That way all future diffs are adding/removing lines.
Yeah, this was corrected by the command:
`git cl format --js --no-python chrome/browser/resources`
in presubmit warning suggestion. Now I changed it back to three lines.
Done
Related details
- Fergal Daly
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 2 comments![]( "Open in Gerrit")
// BrowserControlsFactory is used by the WebUI page to bootstrap communication
// to/from the browser process.
interface BrowserControlsFactory {
// The WebUI calls this method when the page is first initialized.
// It binds a pending_remote for the observer and a pending_receiver for the
// service.
CreateBrowserControls(
pending_remote<BrowserControlsObserver> observer,
pending_receiver<BrowserControlsService> service);
};Giovanni Ortuno UrquidiIf we move to the service pattern, we do not need this factory anymore.
David YeungWhat's the service pattern?
Daniel ChengFactories are used when the WebUI client creates their own remote/receivers and then passes it to the factory instance in the browser to bind. It is an empheral 1:1 relationship where the page is tied to a page handler.
We do not need this factory interface for TabStripService. Instead many different clients can bind to a persistent service component in the browser.
David YeungHow would that change this interface? Do you mean that the WebUI would bind the service interface directly?
While this does add a bit of indirection/boilerplate, there's value to "this remote/receiver pair should always be bound together". Switching to a singleton services shouldn't affect whether or not this is a useful invariant to maintain.
Giovanni Ortuno UrquidiYes, the WebUI would bind the service directly. You wouldn't need the factory interface. IMO the factory pattern is confusing. I think there is potential for a pattern that has less boilerplate - where clients do not need to know about remote/receivers/binding, and instead it is all taken care of by the service.
I think there is potential for a pattern that has less boilerplate - where clients do not need to know about remote/receivers/binding, and instead it is all taken care of by the service.
Can you elaborate on what this looks like in practice and how it maintains the invariant dcheng mentioned?
I agree the factory pattern adds a lot of boilerplate but without it callers end up with interface implementations like this[[1]](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/searchbox/webui_omnibox_handler.cc;l=338-356;drc=4864b1e90893cbda6e50d8e155898acf62758e52) and crashes like this[[2]](https://issues.chromium.org/u/1/issues/468943376?pli=1).
here is an example of such a usage:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/webui_browser/tab_strip_controller.ts;l=79;bpv=0
In the above example, the browser side is completely unaware of the webui client and how it's implemented. There isn't any assumption about the client or browser state on bind, so we wouldn't run into the same issue as the omnibox.
There are cases where we don't necessary want or need the invariance that a remote/receiver pair is bound at the same time. For example, if there is a client that is completely uninterested in receiving updates, it might not want to pass over a receiver at all.
enum ClickDispositionFlag {Fergal Dalynit. Consider a kUnknown state.
Is there a known use case for an unknown state? I would suggest not adding it until you have something that wants to send the value since handling it might be tricky.
the risk with having a value assigned to default (0) init is that it's very easy to accidentally express something you didn't intend. For example, if I had something like:
struct SomeThing {
ClickDispositionFlag flag;
};And we did something like this: `SomeThing thing = new SomeThing()` and neglect to set `thing.flag`, we'd end up in an undefined state. It is reasonable to use a NOT_REACHED for the kUnknown state on the client side. That might trigger a failure, but at least we don't end up in an undefined state. Maybe kUnspecified is a better name?
FWIW, this was the recommendation for lacros (which was extensible, but similar idea):
https://docs.google.com/document/d/17ClARPa5eHH5rhae_N1efZ_ytAEL72Z2QxOaeRo-ZhY/edit?tab=t.0#bookmark=id.wsmctrmzkl5r
It's also a best practice for protos:
go/protodosdonts#unspecified-enum
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
class WebUIToolbarPageHandler
: public browser_controls_api::mojom::BrowserControlsService {Fergal DalyThe idea of a service is that we can reuse it across multiple UIs, instead of strictly tied to the WebUIToolbarPageHandler. It should be able to stand up by itself and stateless to handle multiple clients. Take a look at [TabStripService](https://crsrc.org/c/chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_mojo_handler.cc;drc=07ba8b6a0f60aa5d68af93ef447b3979a4eb09f9;l=13) as an example. It exists in the browser process and we accept any clients (WebUIs) that bind to it.
Happy to discuss details elsewhere.
Paul JensenIt's not happening in this CL, so it's not blocking but is there a design doc somewhere that describes this full plan? "accept any clients (WebUIs)" sounds like it has security implications, e.g. maybe we should only accept clients that are specifically known to be THE toolbar renderer.
David YeungDavid, that sounds like a good idea, perhaps we should rename this class to `BrowserControlsService`?
In the follow-up https://chromium-review.googlesource.com/c/chromium/src/+/7413625 perhaps we could move this file up a couple directories so that it can be used by webui_browser/ stuff too.
Renaming to BrowserControlsService sounds good to me.
accept any clients (WebUIs)" sounds like it has security implications
We've talked with chrome security during TabStripApi and they've allowed it but it was limited to topchrome webuis. I think its a case by case if the service wants to limit what type of clients can be accepted. The main feedback is that it shouldn't be 1:1.
- David Yeung
- Fergal Daly
- Nasko Oskov
- Paul Jensen
- Theresa Sullivan
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. |
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
class WebUIToolbarPageHandler
: public browser_controls_api::mojom::BrowserControlsService {Fergal DalyThe idea of a service is that we can reuse it across multiple UIs, instead of strictly tied to the WebUIToolbarPageHandler. It should be able to stand up by itself and stateless to handle multiple clients. Take a look at [TabStripService](https://crsrc.org/c/chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_mojo_handler.cc;drc=07ba8b6a0f60aa5d68af93ef447b3979a4eb09f9;l=13) as an example. It exists in the browser process and we accept any clients (WebUIs) that bind to it.
Happy to discuss details elsewhere.
Paul JensenIt's not happening in this CL, so it's not blocking but is there a design doc somewhere that describes this full plan? "accept any clients (WebUIs)" sounds like it has security implications, e.g. maybe we should only accept clients that are specifically known to be THE toolbar renderer.
David YeungDavid, that sounds like a good idea, perhaps we should rename this class to `BrowserControlsService`?
In the follow-up https://chromium-review.googlesource.com/c/chromium/src/+/7413625 perhaps we could move this file up a couple directories so that it can be used by webui_browser/ stuff too.
Tianyang XuRenaming to BrowserControlsService sounds good to me.
accept any clients (WebUIs)" sounds like it has security implications
We've talked with chrome security during TabStripApi and they've allowed it but it was limited to topchrome webuis. I think its a case by case if the service wants to limit what type of clients can be accepted. The main feedback is that it shouldn't be 1:1.
Done
@day...@chromium.org Can you still find the discussion regarding to the unlimited service connection with the chrome security team?
Paul Jensen (Gerrit)
Paul Jensen added 1 comment![]( "Open in Gerrit")
enum ClickDispositionFlag {Fergal Dalynit. Consider a kUnknown state.
Fred ShihIs there a known use case for an unknown state? I would suggest not adding it until you have something that wants to send the value since handling it might be tricky.
the risk with having a value assigned to default (0) init is that it's very easy to accidentally express something you didn't intend. For example, if I had something like:
struct SomeThing {
ClickDispositionFlag flag;
};And we did something like this: `SomeThing thing = new SomeThing()` and neglect to set `thing.flag`, we'd end up in an undefined state. It is reasonable to use a NOT_REACHED for the kUnknown state on the client side. That might trigger a failure, but at least we don't end up in an undefined state. Maybe kUnspecified is a better name?
FWIW, this was the recommendation for lacros (which was extensible, but similar idea):
https://docs.google.com/document/d/17ClARPa5eHH5rhae_N1efZ_ytAEL72Z2QxOaeRo-ZhY/edit?tab=t.0#bookmark=id.wsmctrmzkl5rIt's also a best practice for protos:
go/protodosdonts#unspecified-enum
I suggest we follow advice from https://chromium-review.googlesource.com/c/chromium/src/+/7405167/10/components/browser_apis/browser_controls/browser_controls_api.mojom#42 and consider getting rid of this enum entirely, and switch to two more explicit APIs without enums:
Reload(bool bypass_cache); (instead of when shift or control key is used)
DuplicateCurrentTab(); (instead of when middle click is used)
Related details
- David Yeung
- Fergal Daly
- Nasko Oskov
- Theresa Sullivan
- Tianyang Xu
Paul Jensen (Gerrit)
Paul Jensen added 2 comments![]( "Open in Gerrit")
// Stops the current page from reloading. If the page is not in a loadingThis stops any load, not just reloads, so proposing a reword:
```suggestion
// Stops the current page from loading. If the page is not in a loading
```
StopReload();This stops any load, not just reloads, so proposing a rename:
```suggestion
StopLoad();
```
Related details
- David Yeung
- Fergal Daly
- Nasko Oskov
- Theresa Sullivan
- Tianyang Xu
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. |
David Yeung (Gerrit)
David Yeung added 1 comment![]( "Open in Gerrit")
class WebUIToolbarPageHandler
: public browser_controls_api::mojom::BrowserControlsService {Fergal DalyThe idea of a service is that we can reuse it across multiple UIs, instead of strictly tied to the WebUIToolbarPageHandler. It should be able to stand up by itself and stateless to handle multiple clients. Take a look at [TabStripService](https://crsrc.org/c/chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_mojo_handler.cc;drc=07ba8b6a0f60aa5d68af93ef447b3979a4eb09f9;l=13) as an example. It exists in the browser process and we accept any clients (WebUIs) that bind to it.
Happy to discuss details elsewhere.
Paul JensenIt's not happening in this CL, so it's not blocking but is there a design doc somewhere that describes this full plan? "accept any clients (WebUIs)" sounds like it has security implications, e.g. maybe we should only accept clients that are specifically known to be THE toolbar renderer.
David YeungDavid, that sounds like a good idea, perhaps we should rename this class to `BrowserControlsService`?
In the follow-up https://chromium-review.googlesource.com/c/chromium/src/+/7413625 perhaps we could move this file up a couple directories so that it can be used by webui_browser/ stuff too.
Tianyang XuRenaming to BrowserControlsService sounds good to me.
accept any clients (WebUIs)" sounds like it has security implications
We've talked with chrome security during TabStripApi and they've allowed it but it was limited to topchrome webuis. I think its a case by case if the service wants to limit what type of clients can be accepted. The main feedback is that it shouldn't be 1:1.
Tianyang XuDone
@day...@chromium.org Can you still find the discussion regarding to the unlimited service connection with the chrome security team?
It was in a CL thread when we made tabstripservice but I don't recall which one.
If this is a blocker, we can start new discussion with chrome security folks about what it would take to allow 1 to many. I understand there are security implications for untrusted urls on other WebUIs, but ideally webium should be a 1st party citizen. We should make sure it follows all of the safety protocols while being able to do all of the actions necessary for a top chrome webui page.
Related details
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
interface BrowserControlsObserver {Tianyang Xunit: This is a bit of unconventional naming, as it is an API the browser uses to invoke state changes in the renderer process. If it does that because implementation-wise it observes some changes, that's fine, but it is an implementation detail that is leaked in the name.
Nasko OskovThe "Observer" in the name of this interface, which is implemented by the renderer, refers to the renderer observing browser events, rather than referring to a possible implementation detail where the browser observes and forwards these events.
Paul JensenBut the renderer is not actually observing the events in the browser. It doesn't subscribe to any observation that I can see. The browser process pushes the updates to the renderer and it responds to those.
Nasko OskovHmm, I felt like Observer was a good name for something that received pushed updates/events. Can you advise us on an alternate name suggestion?
In the deleted webui_toolbar.mojom they used to be called Page & PageHandler, but we received feedback that we should transition them to Observer & Service, especially because in their new shared location they will be used by multiple WebUI pages so the reference to a singular "Page" wasn't appropriate.
David YeungObserver interfaces usually require an object interested in observing events to register and unregister and usually it involves `ObserverList` or similar. This is why this throws it off for me as it has none of that. It is an interface to communicate state changes (and I would assume in the future other things) to the actual UI seen by the user. I think this is why `page` was used more universally, but I also am not a WebUI dev, so don't know what the current best practice is.
You all own the code, so I would defer to you, but it just confuses me that an observer interface does not observe anything :). Naming is hard 😜.
Mingyu Lei+1 Observer should imply state changes - not UI actions. We should be making these events as stateless as possible.
For example, rename to `OnNavigationChanged(NavigationState state);` where the state can be `Unknown, Loading, NotLoading.` and then leave the actions to the various webui clients to handle. `Set` implies some type of action and falls back to the page/page handler pattern.
I think the "observer" sounds more like a proactive role, which registers itself to some source that would produce events, and reacts to those events. The source is simply notifying the observers that something happen, and it's the observer's responsibility to handle it in its own way.
In this case, `SetReloadButtonState` is more like an instruction from the browser side to update the state of the UI, so it feels a bit weird.
By the way, while implementing the first version of the mojom, I was simply following [the explainer](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/webui/webui_explainer.md#mojo-interface-definition), and I thought the PageHandler thing is a common pattern in the web UI implementation. I wonder what is special about this `BrowserControlsObserver`?
especially because in their new shared location they will be used by multiple WebUI pages
Do we have a design or plan of how this will look like in the future? Maybe it's good to link that in the description.
@day...@chromium.org Here is a quick proposal for this CL. I think it is better to modify the new service and observer interfaces and methods in this CL, and put the factory mode refactor stuff in a following one. What do you think?
https://docs.google.com/document/d/16n018AQk-rj7LWy2fyIaaWMdivBbG9EKij1TFin8BmE/edit?usp=sharing
Related details
- Daniel Cheng
- David Yeung
- Fergal Daly
- Fred Shih
- Giovanni Ortuno Urquidi
- Mingyu Lei
- Nasko Oskov
- Paul Jensen
- Theresa Sullivan
David Yeung (Gerrit)
David Yeung added 1 comment![]( "Open in Gerrit")
Direction looks good to me! Left some comments. Thanks for bringing up the security concern.
Related details
- Daniel Cheng
- Fergal Daly
- Fred Shih
- Giovanni Ortuno Urquidi
- Mingyu Lei
- Nasko Oskov
- Paul Jensen
- Theresa Sullivan
- Tianyang Xu