[UnifiedFRE] Introduce AppRouter to manage Unified FRE and Glic App [chromium/src : main]
Dan Harrington (Gerrit)
Dan Harrington added 15 comments![]( "Open in Gerrit")
bool is_unified_fre_enabled = GlicEnabling::IsUnifiedFreEnabled(nit: i suggest to inline these calls instead of using variables
net::EmbeddedTestServer glic_server_;the base class already sets up a test server to serve glic test data, why isn't that setup enough?
this.browserProxy = new BrowserProxyImpl(this);we shouldn't make more than one BrowserProxyImpl, so we'll need to do some modifications to glic_app_controller
navigateTo(view: AppView): void {nit: the word 'navigate' makes me think of web navigation, which is not happening here. I'd pick a different term.
html,it would be nice to do the work to figure out what's shared between fre.css and glic.css, and make a shared .css file. They probably have similar style requirements. Maybe make a bug to track revisiting this?
const INITIAL_WIDTH = loadTimeData.getInteger('freInitialWidth');nit: was there something wrong with this?
interface PageElementTypes {
webviewContainer: HTMLDivElement;
}
const $: PageElementTypes = new Proxy({}, {
get(_target: any, prop: string) {
return getRequiredElement(prop);
},
});
i don't think this had to change, other than the element name.
const container = document.getElementById('fre-app-container');
assert(container, '#fre-app-container not found in constructor');getRequiredElement
this.webviewContainer =
this.freContainer.querySelector<HTMLElement>('#freWebviewContainer')!;
assert(this.webviewContainer);use getRequiredElement?
?.addEventListener('click', (e) => {this is old code, but since you're changing names, it would be better to assert that the element exists.
this.freContainer.querySelector('#fre-reload')better to use getRequiredElement
this.freContainer.addEventListener('keydown', (ev: Event) => {why can't we use KeyboardEvent here, looks like there's typings for this.
<div id="fre-app-container" hidden>how are we going to ensure this stays up to date with fre.html?
I'd be less concerned if we forked the code, but it seems like it would be easy to update one .html file and the .ts file, but not the other .html file.
Can we use grit to include the fre html file? Looks like it's supported https://source.chromium.org/chromium/chromium/src/+/main:tools/grit/grit/testdata/include_test.html
const appRouter = new AppRouter();
window.appRouter = appRouter;
setupCommonListeners(appRouter);
} else {
getRequiredElement('glic-app-container').hidden = false;
const appController = new GlicAppController();
window.appController = appController;
setupCommonListeners(appController);I think we should just always use the app router to simplify things.
Related details
- Michelle Abreo
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. |
Michelle Abreo (Gerrit)
Michelle Abreo voted and added 16 comments![]( "Open in Gerrit")
Votes added by Michelle Abreo
| Commit-Queue | +1 |
16 comments
Hey Dan,
Thanks for the review! I spent some time trying to unify the shared HTML rather than duplicating it, but I ran into a couple of issues:
We couldn’t use <include src="..."> because it’s discouraged per the style guide
(https://source.chromium.org/chromium/chromium/src/+/main:styleguide/web/web.md;l=710-729?q=%22%3Cinclude%20src%3D%22). The README suggested using a custom JS module instead, which worked for the most part, but it couldn’t resolve the $i18nRaw strings used in the FRE’s admin panel.
I then looked into migrating to CrLitElement, and while that would be the ideal long-term solution, it introduces additional build-system complexity that feels outside the scope of this CL for now.
Given those blockers and the time sensitivity of this change, I’d like to temporarily keep both HTML copies, with a <!-- LINT:IfChange --> comment in both places. I filed a follow-up bug to track migrating to a Lit component and removing the duplicate markup (b/453057582), which I will look into towards the end of the week after this code is checked in.
I wanted to make sure I explored the options before circling back — sorry this took a bit longer than expected! Let me know what you think.
bool is_unified_fre_enabled = GlicEnabling::IsUnifiedFreEnabled(nit: i suggest to inline these calls instead of using variables
Done
the base class already sets up a test server to serve glic test data, why isn't that setup enough?
you're right, updated to use embedded_https_test_server()!
comment on what this does
Done
we shouldn't make more than one BrowserProxyImpl, so we'll need to do some modifications to glic_app_controller
You're right, we should just have one BrowserProxyImpl. The main issue with this was dealing with a new lifecycle timing for the PageHandler. Before the `GlicPageHandler` was created after the user consented in the FRE, so its initial profile ready state was always correct. However now we create the handler before the FRE, which means it can exist in a pre-consent state and needs to know when that state changes. This is what creating a new browser proxy in the `GlicAppController` ctor did, since it just set a new state after consent was given. However, since we want to add it in the `AppRouter` ctor, I've added a subscription in the `GlicPageHandler` constructor, which now correctly listens for the consent update, so it ensures that the `ProfileReadyState` is not stale when we switch to the GLIC app.
This actually also makes the support much easier for multi-instance / multiple FRE panels existing and the user accepting on one. I didn't want to add that in this CL since this is already a large change, but I've created a child CL to update the glic.mojom API to support a boolean for transitioning the view so once the `kGlicCompletedFre` pref is updated on any surface, the panel will automatically transition to the GLIC view.
nit: the word 'navigate' makes me think of web navigation, which is not happening here. I'd pick a different term.
updated to `switchToView`!
it would be nice to do the work to figure out what's shared between fre.css and glic.css, and make a shared .css file. They probably have similar style requirements. Maybe make a bug to track revisiting this?
Good idea, created b/452964239!
const INITIAL_WIDTH = loadTimeData.getInteger('freInitialWidth');nit: was there something wrong with this?
I inlined the loadTimeData.getInteger('freInitialWidth') call into the if (this.shouldSizeForDialog) block where it's used. This way the 'freInitialWidth' value is only fetched when needed (for the standalone FRE app's dialog) and isn't unnecessarily included in the loadTimeData for the sidepanel FRE, where this specific width isn't used.
interface PageElementTypes {
webviewContainer: HTMLDivElement;
}
const $: PageElementTypes = new Proxy({}, {
get(_target: any, prop: string) {
return getRequiredElement(prop);
},
});
i don't think this had to change, other than the element name.
I had replaced usages of `$.webviewContainer` with `this.webviewContainer`, so we no longer need the `$` object.
const container = document.getElementById('fre-app-container');
assert(container, '#fre-app-container not found in constructor');Michelle AbreogetRequiredElement
Done.
this.webviewContainer =
this.freContainer.querySelector<HTMLElement>('#freWebviewContainer')!;
assert(this.webviewContainer);Michelle Abreouse getRequiredElement?
Done.
this is old code, but since you're changing names, it would be better to assert that the element exists.
Done
this.freContainer.querySelector('#fre-reload')Michelle Abreobetter to use getRequiredElement
Done.
this.freContainer.addEventListener('keydown', (ev: Event) => {why can't we use KeyboardEvent here, looks like there's typings for this.
Done
how are we going to ensure this stays up to date with fre.html?
I'd be less concerned if we forked the code, but it seems like it would be easy to update one .html file and the .ts file, but not the other .html file.Can we use grit to include the fre html file? Looks like it's supported https://source.chromium.org/chromium/chromium/src/+/main:tools/grit/grit/testdata/include_test.html
Addressed in top level comment
const appRouter = new AppRouter();
window.appRouter = appRouter;
setupCommonListeners(appRouter);
} else {
getRequiredElement('glic-app-container').hidden = false;
const appController = new GlicAppController();
window.appController = appController;
setupCommonListeners(appController);I think we should just always use the app router to simplify things.
- Dan Harrington
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. |
Dan Harrington (Gerrit)
Dan Harrington added 9 comments![]( "Open in Gerrit")
Hey Dan,
Thanks for the review! I spent some time trying to unify the shared HTML rather than duplicating it, but I ran into a couple of issues:
We couldn’t use <include src="..."> because it’s discouraged per the style guide
(https://source.chromium.org/chromium/chromium/src/+/main:styleguide/web/web.md;l=710-729?q=%22%3Cinclude%20src%3D%22). The README suggested using a custom JS module instead, which worked for the most part, but it couldn’t resolve the $i18nRaw strings used in the FRE’s admin panel.I then looked into migrating to CrLitElement, and while that would be the ideal long-term solution, it introduces additional build-system complexity that feels outside the scope of this CL for now.
Given those blockers and the time sensitivity of this change, I’d like to temporarily keep both HTML copies, with a <!-- LINT:IfChange --> comment in both places. I filed a follow-up bug to track migrating to a Lit component and removing the duplicate markup (b/453057582), which I will look into towards the end of the week after this code is checked in.
I wanted to make sure I explored the options before circling back — sorry this took a bit longer than expected! Let me know what you think.
Thanks for trying, given this is a temporary situation, i think migrating to Lit doesn't really make sense anyway. As long as the two copies are supposed to be identical, the IfChange lint will hopefully be enough of a reminder to copy/paste.
void SetUpCommandLine(base::CommandLine* command_line) override {
GlicUiInteractiveUiTestBase::SetUpCommandLine(command_line);
GURL fre_url = embedded_https_test_server().GetURL(
"glic.test", "/glic/test_client/fre.html");
GURL glic_guest_url = embedded_https_test_server().GetURL(
"glic.test", "/glic/test_client/index.html");
ASSERT_TRUE(fre_url.is_valid()) << "FRE URL is invalid";
command_line->AppendSwitchASCII(switches::kGlicFreURL, fre_url.spec());
ASSERT_TRUE(glic_guest_url.is_valid()) << "Glic Guest URL is invalid";
command_line->AppendSwitchASCII(switches::kGlicGuestURL,
glic_guest_url.spec());
command_line->AppendSwitch(switches::kDisableWebSecurity);https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/test_support/interactive_glic_test.h already sets up the guest URL like this. Can you update that file to also set up the FRE url?
embedded_https_test_server().StartAcceptingConnections();I think you can just call Start() here, and remove the calls to StartAcceptingConnections() and InitializeAndListen()
switchToView(view: AppView): void {nit: return early if the view is already the target view.
case AppView.FRE:assert that the GLIC->FRE transition never happens
this.switchToView(AppView.GLIC);
this.freAppController?.destroyWebview();
this.freAppController = undefined;i suggest doing this in switchToView, so there's no incomplete way to switch views.
if (this.currentView === AppView.GLIC && this.glicController) {i think there's never a need to create the glic controller, unless we're showing the glic view. So this can be
```this.glicController?.reload();```
<!-- LINT.IfChange(fre_container) -->make it clear what should be changed. I hope we can say these two sections must be identical.
setProfileReadyState(state: ProfileReadyState|undefined) {why did this change?
Related details
- Michelle Abreo
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. |
Michelle Abreo (Gerrit)
Michelle Abreo added 9 comments![]( "Open in Gerrit")
void SetUpCommandLine(base::CommandLine* command_line) override {
GlicUiInteractiveUiTestBase::SetUpCommandLine(command_line);
GURL fre_url = embedded_https_test_server().GetURL(
"glic.test", "/glic/test_client/fre.html");
GURL glic_guest_url = embedded_https_test_server().GetURL(
"glic.test", "/glic/test_client/index.html");
ASSERT_TRUE(fre_url.is_valid()) << "FRE URL is invalid";
command_line->AppendSwitchASCII(switches::kGlicFreURL, fre_url.spec());
ASSERT_TRUE(glic_guest_url.is_valid()) << "Glic Guest URL is invalid";
command_line->AppendSwitchASCII(switches::kGlicGuestURL,
glic_guest_url.spec());
command_line->AppendSwitch(switches::kDisableWebSecurity);https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/glic/test_support/interactive_glic_test.h already sets up the guest URL like this. Can you update that file to also set up the FRE url?
good idea, this reduces a lot of the complexity in this class' setup. updated!
embedded_https_test_server().StartAcceptingConnections();I think you can just call Start() here, and remove the calls to StartAcceptingConnections() and InitializeAndListen()
Done
nit: return early if the view is already the target view.
Done
assert that the GLIC->FRE transition never happens
Done
this.switchToView(AppView.GLIC);
this.freAppController?.destroyWebview();
this.freAppController = undefined;i suggest doing this in switchToView, so there's no incomplete way to switch views.
Done
if (this.currentView === AppView.GLIC && this.glicController) {i think there's never a need to create the glic controller, unless we're showing the glic view. So this can be
```this.glicController?.reload();```
Done
make it clear what should be changed. I hope we can say these two sections must be identical.
updated with a comment clarifying in both files, and changed `fre_container` to `fre-app-container` to be match the id of the div.
setProfileReadyState(state: ProfileReadyState|undefined) {Michelle Abreowhy did this change?
whoops, this was a changed that was accidentally added from the child CL and not reverted. removed!
Related details
- Dan Harrington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Harrington (Gerrit)
Dan Harrington added 2 comments![]( "Open in Gerrit")
GetGlicService()->enabling().RegisterAllowedChanged(base::BindRepeating(glic enabling should instead offer a ProfileReadyStateChanged callback, do you mind doing that instead?
const controller = window.appRouter.glicController;Related details
- Michelle Abreo
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. |
Michelle Abreo (Gerrit)
Michelle Abreo voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michelle Abreo
| Commit-Queue | +1 |
2 comments
GetGlicService()->enabling().RegisterAllowedChanged(base::BindRepeating(glic enabling should instead offer a ProfileReadyStateChanged callback, do you mind doing that instead?
Done.
I think this also needs updated: http://shortn/_O4Lf26rXN0
good catch, the dry run didn't catch it earlier because the test was disabled! updated.
Related details
- Dan Harrington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Harrington (Gerrit)
Dan Harrington voted Code-Review+1![]( "Open in Gerrit")
- Michelle Abreo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |