Package AIM eligibility WebUI in a component extension [chromium/src : main]
Moe Ahmadi (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Colin Blundell
- Demetrios Papadopoulos
- Devlin Cronin
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks! Moe, what kind of feedback are you looking for here at this point? e.g., where on the spectrum is this from (a) POC that you're looking to get folks' insight on the high-level approach and how to remove any hacks to (z) CL that you want to land in the codebase and want folks to review as such?
Related details
- Demetrios Papadopoulos
- Devlin Cronin
- Moe Ahmadi
- Nasko Oskov
Moe Ahmadi (Gerrit)
Moe Ahmadi added 1 comment![]( "Open in Gerrit")
Thanks! Moe, what kind of feedback are you looking for here at this point? e.g., where on the spectrum is this from (a) POC that you're looking to get folks' insight on the high-level approach and how to remove any hacks to (z) CL that you want to land in the codebase and want folks to review as such?
Hi Colin, I'd like to land it in the codebase with the understanding that we will continue iterating on it. I added information about the goals in the bug.
Related details
- Colin Blundell
- Demetrios Papadopoulos
- Devlin Cronin
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Moe AhmadiThanks! Moe, what kind of feedback are you looking for here at this point? e.g., where on the spectrum is this from (a) POC that you're looking to get folks' insight on the high-level approach and how to remove any hacks to (z) CL that you want to land in the codebase and want folks to review as such?
Hi Colin, I'd like to land it in the codebase with the understanding that we will continue iterating on it. I added information about the goals in the bug.
Thanks! I defer to the other reviewers on the technical review (let me know if there was anything that you wanted me to look at on the technical side!).
Related details
- Demetrios Papadopoulos
- Devlin Cronin
- Moe Ahmadi
- Nasko Oskov
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 added 1 comment![]( "Open in Gerrit")
<include name="IDR_AIM_ELIGIBILITY_EXTENSION_HTML" file="omnibox/aim_eligibility/aim_eligibility.html" resource_path="aim_eligibility_extension/aim_eligibility.html" type="BINDATA" />
<include name="IDR_AIM_ELIGIBILITY_EXTENSION_APP_JS" file="${root_gen_dir}/chrome/browser/resources/omnibox_aim_eligibility_extension/app.rollup.js" use_base_dir="false" resource_path="aim_eligibility_extension/app.js" type="BINDATA" />We have moved away from adding WebuI files (or component extension files) in checked-in grd files. Can these not be included with the modern ways, via properly hooking up the output of build_webui() to the build?
See example from the PDF Viewer extension, https://source.chromium.org/search?q=%22chrome%2Fbrowser%2Fresources%2Fpdf:resources%22%20-file:out%2F%20-file:go%2F%20-file:luci%2F%20-file:third_party%2F%20-file:appengine%2F%20-file:v8%2F&ss=chromium
Related details
- Devlin Cronin
- Moe Ahmadi
- Nasko Oskov
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. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 3 comments![]( "Open in Gerrit")
Package AIM eligibility WebUI in a component extensionIIUC this CL simply adds a dummy expty extension, it does not actually package the existing WebUI as an extension, no? Can you clarify? If this is the first part of a CL that follows, maybe suffix with `part 1` ?
```suggestion
Package AIM eligibility WebUI in a component extension, part 1.
```
tooling to bundle the extension's JS resouces. This automaticallyPlease fix this WARNING reported by Spellchecker: "resouces" is a possible misspelling of "resources".
To bypass Spellchecker, ad...
"resouces" is a possible misspelling of "resources".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Additionally, the build process now uses the standard bundle_js Rollup
tooling to bundle the extension's JS resouces. This automaticallyI don't see these changes anymore in this CL. Can you clarify? Were these moved to a follow up CL?
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
bundle_js("bundle_aim_eligibility_extension") {
host = "chrome-extension://kgjeljgkbckpoekmgjfplammhcggiiaf"
input = rebase_path(
"$root_gen_dir/chrome/browser/resources/omnibox/aim_eligibility/tsc",
root_build_dir)
js_module_in_files = [ "app.js" ]
out_folder = "$root_gen_dir/chrome/browser/resources/omnibox_aim_eligibility_extension"
excludes = [ "chrome://resources/mojo/mojo/public/js/bindings.js" ]
deps = [ "//chrome/browser/resources/omnibox/aim_eligibility:build_grdp" ]
}
copy("copy_aim_eligibility_extension_app_js") {
sources = [ "$root_gen_dir/chrome/browser/resources/omnibox_aim_eligibility_extension/app.rollup.js" ]
outputs = [ "$root_out_dir/aim_eligibility_extension/app.js" ]
deps = [ ":bundle_aim_eligibility_extension" ]
}Can this code be moved to chrome/browser/resources/omnibox/aim_eligibility_extension/BUILD.gn instead? It seems to belong there better, no? Doing this here seems a bit hacky.
For example, is using the bundled code for both optimize_webui false/true correct? This does not match what the corresponding WebUI does.
See other similar comment in the .grd file about properly using `build_webui()`, in chrome/browser/resources/omnibox/aim_eligibility_extension/BUILD.gn to achieve a more common build pipeline that works as expected for both optimize_webui cases.
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
if (site_instance_->GetSiteInfo().site_url().SchemeIs("chrome-extension")) {This is a layering violation and should not be done here. Whether this is extension or not is something to be delegated to the embedder.
BrowserContext* browser_context);The //content/ layer should not deal and know about extensions. They are a higher level abstraction. Any reason why the API is not about any URL?
Related details
- Devlin Cronin
- Moe Ahmadi
Moe Ahmadi (Gerrit)
Moe Ahmadi added 7 comments![]( "Open in Gerrit")
Package AIM eligibility WebUI in a component extensionIIUC this CL simply adds a dummy expty extension, it does not actually package the existing WebUI as an extension, no? Can you clarify? If this is the first part of a CL that follows, maybe suffix with `part 1` ?
```suggestion
Package AIM eligibility WebUI in a component extension, part 1.
```
this CL does package the existing WebUI into a private component extension which works by navigating to its url.
tooling to bundle the extension's JS resouces. This automaticallyPlease fix this WARNING reported by Spellchecker: "resouces" is a possible misspelling of "resources".
To bypass Spellchecker, ad...
"resouces" is a possible misspelling of "resources".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
Additionally, the build process now uses the standard bundle_js Rollup
tooling to bundle the extension's JS resouces. This automaticallyI don't see these changes anymore in this CL. Can you clarify? Were these moved to a follow up CL?
`bundle_js` changes are in this CL.
bundle_js("bundle_aim_eligibility_extension") {
host = "chrome-extension://kgjeljgkbckpoekmgjfplammhcggiiaf"
input = rebase_path(
"$root_gen_dir/chrome/browser/resources/omnibox/aim_eligibility/tsc",
root_build_dir)
js_module_in_files = [ "app.js" ]
out_folder = "$root_gen_dir/chrome/browser/resources/omnibox_aim_eligibility_extension"
excludes = [ "chrome://resources/mojo/mojo/public/js/bindings.js" ]
deps = [ "//chrome/browser/resources/omnibox/aim_eligibility:build_grdp" ]
}
copy("copy_aim_eligibility_extension_app_js") {
sources = [ "$root_gen_dir/chrome/browser/resources/omnibox_aim_eligibility_extension/app.rollup.js" ]
outputs = [ "$root_out_dir/aim_eligibility_extension/app.js" ]
deps = [ ":bundle_aim_eligibility_extension" ]
}Can this code be moved to chrome/browser/resources/omnibox/aim_eligibility_extension/BUILD.gn instead? It seems to belong there better, no? Doing this here seems a bit hacky.
For example, is using the bundled code for both optimize_webui false/true correct? This does not match what the corresponding WebUI does.
See other similar comment in the .grd file about properly using `build_webui()`, in chrome/browser/resources/omnibox/aim_eligibility_extension/BUILD.gn to achieve a more common build pipeline that works as expected for both optimize_webui cases.
Done. Moved the grit resource target to `aim_eligibility_extension/BUILD.gn`. I had difficulty using `build_webui()` with compiling resources from a sibling directory and used lower-level webui_ts_library and optimize_webui targets directly.
bundling the code regardless of `optimize_webui false/true` was intended to avoid having to list all the JS resources. Obviously if we can get to use build_webui() and conditionally bundle or not it would be ideal.
<include name="IDR_AIM_ELIGIBILITY_EXTENSION_HTML" file="omnibox/aim_eligibility/aim_eligibility.html" resource_path="aim_eligibility_extension/aim_eligibility.html" type="BINDATA" />
<include name="IDR_AIM_ELIGIBILITY_EXTENSION_APP_JS" file="${root_gen_dir}/chrome/browser/resources/omnibox_aim_eligibility_extension/app.rollup.js" use_base_dir="false" resource_path="aim_eligibility_extension/app.js" type="BINDATA" />We have moved away from adding WebuI files (or component extension files) in checked-in grd files. Can these not be included with the modern ways, via properly hooking up the output of build_webui() to the build?
See example from the PDF Viewer extension, https://source.chromium.org/search?q=%22chrome%2Fbrowser%2Fresources%2Fpdf:resources%22%20-file:out%2F%20-file:go%2F%20-file:luci%2F%20-file:third_party%2F%20-file:appengine%2F%20-file:v8%2F&ss=chromium
Done, thanks for the example.
if (site_instance_->GetSiteInfo().site_url().SchemeIs("chrome-extension")) {This is a layering violation and should not be done here. Whether this is extension or not is something to be delegated to the embedder.
Done. renamed the function to a generic `ShouldAllowMojoJsBindingsForURL()`
The //content/ layer should not deal and know about extensions. They are a higher level abstraction. Any reason why the API is not about any URL?
- Demetrios Papadopoulos
- Devlin Cronin
- Nasko Oskov
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. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 6 comments![]( "Open in Gerrit")
Package AIM eligibility WebUI in a component extensionMoe AhmadiIIUC this CL simply adds a dummy expty extension, it does not actually package the existing WebUI as an extension, no? Can you clarify? If this is the first part of a CL that follows, maybe suffix with `part 1` ?
```suggestion
Package AIM eligibility WebUI in a component extension, part 1.
```
this CL does package the existing WebUI into a private component extension which works by navigating to its url.
Ack. I wasn't expecting the build setup for this to be in chorme/browser/resources/BUILD.gn so I missed it when I made that comment, realized it later.
Additionally, the build process now uses the standard bundle_js Rollup
tooling to bundle the extension's JS resouces. This automaticallyMoe AhmadiI don't see these changes anymore in this CL. Can you clarify? Were these moved to a follow up CL?
`bundle_js` changes are in this CL.
Acknowledged
bundle_js("bundle_aim_eligibility_extension") {
host = "chrome-extension://kgjeljgkbckpoekmgjfplammhcggiiaf"
input = rebase_path(
"$root_gen_dir/chrome/browser/resources/omnibox/aim_eligibility/tsc",
root_build_dir)
js_module_in_files = [ "app.js" ]
out_folder = "$root_gen_dir/chrome/browser/resources/omnibox_aim_eligibility_extension"
excludes = [ "chrome://resources/mojo/mojo/public/js/bindings.js" ]
deps = [ "//chrome/browser/resources/omnibox/aim_eligibility:build_grdp" ]
}
copy("copy_aim_eligibility_extension_app_js") {
sources = [ "$root_gen_dir/chrome/browser/resources/omnibox_aim_eligibility_extension/app.rollup.js" ]
outputs = [ "$root_out_dir/aim_eligibility_extension/app.js" ]
deps = [ ":bundle_aim_eligibility_extension" ]
}Moe AhmadiCan this code be moved to chrome/browser/resources/omnibox/aim_eligibility_extension/BUILD.gn instead? It seems to belong there better, no? Doing this here seems a bit hacky.
For example, is using the bundled code for both optimize_webui false/true correct? This does not match what the corresponding WebUI does.
See other similar comment in the .grd file about properly using `build_webui()`, in chrome/browser/resources/omnibox/aim_eligibility_extension/BUILD.gn to achieve a more common build pipeline that works as expected for both optimize_webui cases.
Done. Moved the grit resource target to `aim_eligibility_extension/BUILD.gn`. I had difficulty using `build_webui()` with compiling resources from a sibling directory and used lower-level webui_ts_library and optimize_webui targets directly.
bundling the code regardless of `optimize_webui false/true` was intended to avoid having to list all the JS resources. Obviously if we can get to use build_webui() and conditionally bundle or not it would be ideal.
I had difficulty using build_webui() with compiling resources from a sibling directory and used lower-level webui_ts_library and optimize_webui targets directly.
Curious what did you run into. Indeed using build_webui() to point to files that belong to non-child folders is not common, so some assumptions might be broken.
Having said that, I think we should follow-up with necessary changes to make this work properly because what is done here is a one-off build codepath and seems is a bit hacky (on top of not respecting optimize_webui, possibly not minifying files at all)
Let's also add `visibility` rules for all targets below to make sure only targets that are supposed to be exposed outside of this file are actually exposed publicly? (this is taken care by build_webui() normally).
bundle_js("bundle_js") {Note that bundling does not do any minifying, removing comments etc. So this custom one-off build setup, IIUC will package completely unminified code into the build. Can you double check? And if so, is this OK?
Do you need to also invoke minify_js() below?
out_folder = "$target_gen_dir/bundled"Can this point to the final folder directly? See follow-up comment as well
```suggestion
out_folder = "$target_gen_dir/resources"
```
Related details
- Devlin Cronin
- Moe Ahmadi
- Nasko Oskov
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
AddAimEligibilityExtension();I am a bit concerned that the experimenal component extension that is added in this CL is not guarded by any feature flag. Is this intentional? Can you highlight this in the CL description?
Or alternatively, should this be placed behind a feature flag?
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
import("//ui/webui/resources/tools/generate_grd.gni")Posting a possible (simpler) alternative to package the existing WebUI as an extension. Would the following work?
Leverage the already produced `out/gchrome/gen/chrome/browser/resources/omnibox/aim_eligibility/resources.grdp` file, and simply pass it to a new `generate_grd()` target in this file to produce `out/gchrome/gen/chrome/browser/resources/omnibox/aim_eligibility_extension/resources.grd` file.
Then you can remove `bundle_js()` from here, and you would still get bundled+minified resources when optimize_webui=true, and non-bundled, non-minified when optimize_webui=false. The registration of the resources in chrome/browser/extensions/chrome_component_extension_resource_manager.cc would work seamlessly for both cases.
WDYT?
Moe Ahmadi (Gerrit)
Moe Ahmadi added 6 comments![]( "Open in Gerrit")
I am a bit concerned that the experimenal component extension that is added in this CL is not guarded by any feature flag. Is this intentional? Can you highlight this in the CL description?
Or alternatively, should this be placed behind a feature flag?
Good callout, done.
you're 100% right we should follow-up and fix this properly. that's essentially the goal of this proof of concept. I filed b/524396928 describing issues I've been experiencing. If that's OK with you, let's track the problem there for a follow-up change.
import("//ui/webui/resources/tools/generate_grd.gni")Posting a possible (simpler) alternative to package the existing WebUI as an extension. Would the following work?
Leverage the already produced `out/gchrome/gen/chrome/browser/resources/omnibox/aim_eligibility/resources.grdp` file, and simply pass it to a new `generate_grd()` target in this file to produce `out/gchrome/gen/chrome/browser/resources/omnibox/aim_eligibility_extension/resources.grd` file.
Then you can remove `bundle_js()` from here, and you would still get bundled+minified resources when optimize_webui=true, and non-bundled, non-minified when optimize_webui=false. The registration of the resources in chrome/browser/extensions/chrome_component_extension_resource_manager.cc would work seamlessly for both cases.
WDYT?
Thanks for the suggestion! Unfortunately reusing the sibling's resources.grdp didn't not work because the extension loader expects all resources in the PAK to be prefixed with the extension's directory name (aim_eligibility_extension/manifest.json, etc). The sibling WebUI's resources.grdp defines unprefixed paths (aim_eligibility.html, etc) to serve them at the root of chrome://omnibox/.
If we include the sibling WebUI's resources.grdp directly, GRIT compiles them unprefixed. We cannot prefix them in the sibling's build target either without breaking the chrome://omnibox/ WebUI.
I could be holding this wrong, so please share your thoughts and ideas in b/524396928 to follow this change.
Let's also add `visibility` rules for all targets below to make sure only targets that are supposed to be exposed outside of this file are actually exposed publicly? (this is taken care by build_webui() normally).
Done
bundle_js("bundle_js") {Note that bundling does not do any minifying, removing comments etc. So this custom one-off build setup, IIUC will package completely unminified code into the build. Can you double check? And if so, is this OK?
Do you need to also invoke minify_js() below?
you're 100% correct, minify_js is never invoked. neither this or having to bundle JS in dev builds is OK. however I'm not sure if it's worth spending time replicating everything build_webui() does or invest in make build_webui() work in a follow-up (b/524396928). WDYT?
out_folder = "$target_gen_dir/bundled"Can this point to the final folder directly? See follow-up comment as well
```suggestion
out_folder = "$target_gen_dir/resources"
```
we would still need a copy target to rename app.rollup.js to app.js inside that directory, no?
Related details
- Demetrios Papadopoulos
- Devlin Cronin
- Nasko Oskov
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. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 4 comments![]( "Open in Gerrit")
bundle_js("bundle_aim_eligibility_extension") {
host = "chrome-extension://kgjeljgkbckpoekmgjfplammhcggiiaf"
input = rebase_path(
"$root_gen_dir/chrome/browser/resources/omnibox/aim_eligibility/tsc",
root_build_dir)
js_module_in_files = [ "app.js" ]
out_folder = "$root_gen_dir/chrome/browser/resources/omnibox_aim_eligibility_extension"
excludes = [ "chrome://resources/mojo/mojo/public/js/bindings.js" ]
deps = [ "//chrome/browser/resources/omnibox/aim_eligibility:build_grdp" ]
}
copy("copy_aim_eligibility_extension_app_js") {
sources = [ "$root_gen_dir/chrome/browser/resources/omnibox_aim_eligibility_extension/app.rollup.js" ]
outputs = [ "$root_out_dir/aim_eligibility_extension/app.js" ]
deps = [ ":bundle_aim_eligibility_extension" ]
}Moe AhmadiCan this code be moved to chrome/browser/resources/omnibox/aim_eligibility_extension/BUILD.gn instead? It seems to belong there better, no? Doing this here seems a bit hacky.
For example, is using the bundled code for both optimize_webui false/true correct? This does not match what the corresponding WebUI does.
See other similar comment in the .grd file about properly using `build_webui()`, in chrome/browser/resources/omnibox/aim_eligibility_extension/BUILD.gn to achieve a more common build pipeline that works as expected for both optimize_webui cases.
Demetrios PapadopoulosDone. Moved the grit resource target to `aim_eligibility_extension/BUILD.gn`. I had difficulty using `build_webui()` with compiling resources from a sibling directory and used lower-level webui_ts_library and optimize_webui targets directly.
bundling the code regardless of `optimize_webui false/true` was intended to avoid having to list all the JS resources. Obviously if we can get to use build_webui() and conditionally bundle or not it would be ideal.
Moe AhmadiI had difficulty using build_webui() with compiling resources from a sibling directory and used lower-level webui_ts_library and optimize_webui targets directly.
Curious what did you run into. Indeed using build_webui() to point to files that belong to non-child folders is not common, so some assumptions might be broken.
Having said that, I think we should follow-up with necessary changes to make this work properly because what is done here is a one-off build codepath and seems is a bit hacky (on top of not respecting optimize_webui, possibly not minifying files at all)
you're 100% right we should follow-up and fix this properly. that's essentially the goal of this proof of concept. I filed b/524396928 describing issues I've been experiencing. If that's OK with you, let's track the problem there for a follow-up change.
SG. As long as you are aware that further modifications to this extension code might be hindered by using a custom one-off build configuration until/if properly fixed.
Thanks for filing the bug.
import("//ui/webui/resources/tools/generate_grd.gni")Moe AhmadiPosting a possible (simpler) alternative to package the existing WebUI as an extension. Would the following work?
Leverage the already produced `out/gchrome/gen/chrome/browser/resources/omnibox/aim_eligibility/resources.grdp` file, and simply pass it to a new `generate_grd()` target in this file to produce `out/gchrome/gen/chrome/browser/resources/omnibox/aim_eligibility_extension/resources.grd` file.
Then you can remove `bundle_js()` from here, and you would still get bundled+minified resources when optimize_webui=true, and non-bundled, non-minified when optimize_webui=false. The registration of the resources in chrome/browser/extensions/chrome_component_extension_resource_manager.cc would work seamlessly for both cases.
WDYT?
Thanks for the suggestion! Unfortunately reusing the sibling's resources.grdp didn't not work because the extension loader expects all resources in the PAK to be prefixed with the extension's directory name (aim_eligibility_extension/manifest.json, etc). The sibling WebUI's resources.grdp defines unprefixed paths (aim_eligibility.html, etc) to serve them at the root of chrome://omnibox/.
If we include the sibling WebUI's resources.grdp directly, GRIT compiles them unprefixed. We cannot prefix them in the sibling's build target either without breaking the chrome://omnibox/ WebUI.
I could be holding this wrong, so please share your thoughts and ideas in b/524396928 to follow this change.
because the extension loader expects all resources in the PAK to be prefixed with the extension's directory name (aim_eligibility_extension/manifest.json, etc)
generate_grd() exposes a `resource_path_rewrites` property which can be usesd to modify the path under which a file is going to be served. Perhaps you could leverage this to satisfy the extension loader's expectation. Having said that, off the top of my head, I don't remember if `resource_path_rewrites` is applied to resources residing in files passed via `grdp_files` (probably not).
I could be holding this wrong, so please share your thoughts and ideas in b/524396928 to follow this change.
SG. Lets' continue the discussion there.
bundle_js("bundle_js") {Moe AhmadiNote that bundling does not do any minifying, removing comments etc. So this custom one-off build setup, IIUC will package completely unminified code into the build. Can you double check? And if so, is this OK?
Do you need to also invoke minify_js() below?
you're 100% correct, minify_js is never invoked. neither this or having to bundle JS in dev builds is OK. however I'm not sure if it's worth spending time replicating everything build_webui() does or invest in make build_webui() work in a follow-up (b/524396928). WDYT?
Evaluating this work as a follow-up SGTM. Having said that, in order to set the correct expectations, it is likely that supporting this case (aka having build_webui() be fully functional when referring to files residing in a sibling folder) might prove to be too complicated and you might be stuck with this custom build configuration for a while.
Since this request is not common, and would be obsolete if/when the WebUI is fully migrated to the component extension, at which point you could use build_webui() like the PDF viewer already does, we have to be very careful with avoiding unnecessary complexity to address a one-off transient need.
LMK if this sounds reasonable.
out_folder = "$target_gen_dir/bundled"Moe AhmadiCan this point to the final folder directly? See follow-up comment as well
```suggestion
out_folder = "$target_gen_dir/resources"
```
we would still need a copy target to rename app.rollup.js to app.js inside that directory, no?
build_webui() does this by leveraging `resource_path_rewrites` avoiding any extra copies, see [1]. As said earlier, not sure if you can leverage that here though.
Related details
- Devlin Cronin
- Moe Ahmadi
- Nasko Oskov
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
//content/ LGTM once the API is renamed.
GetContentClient()->browser()->ShouldAllowMojoJsBindingsForURL(Since you are passing a site URL, this should probably be "ForSite" as it won't support a full URL and we won't enable/disable bindings based on anything beyond origin or site. It is also the suffix commonly found in the API to the embedder.
Related details
- Devlin Cronin
- Moe Ahmadi
Nasko Oskov (Gerrit)
Nasko Oskov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Devlin Cronin
- Moe Ahmadi
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. |
Moe Ahmadi (Gerrit)
Moe Ahmadi added 3 comments![]( "Open in Gerrit")
bundle_js("bundle_js") {Moe AhmadiNote that bundling does not do any minifying, removing comments etc. So this custom one-off build setup, IIUC will package completely unminified code into the build. Can you double check? And if so, is this OK?
Do you need to also invoke minify_js() below?
Demetrios Papadopoulosyou're 100% correct, minify_js is never invoked. neither this or having to bundle JS in dev builds is OK. however I'm not sure if it's worth spending time replicating everything build_webui() does or invest in make build_webui() work in a follow-up (b/524396928). WDYT?
Evaluating this work as a follow-up SGTM. Having said that, in order to set the correct expectations, it is likely that supporting this case (aka having build_webui() be fully functional when referring to files residing in a sibling folder) might prove to be too complicated and you might be stuck with this custom build configuration for a while.
Since this request is not common, and would be obsolete if/when the WebUI is fully migrated to the component extension, at which point you could use build_webui() like the PDF viewer already does, we have to be very careful with avoiding unnecessary complexity to address a one-off transient need.
LMK if this sounds reasonable.
That makes perfect sense. I agree that we shouldn't add unnecessary complexity to build_webui() for a one-off transient setup if it proves difficult. As long as we resolve minification in `optimize_webui = true` and resource resolution in `optimize_webui = false` it's totally fine to stick to this custom configuration for now.
out_folder = "$target_gen_dir/bundled"Moe AhmadiCan this point to the final folder directly? See follow-up comment as well
```suggestion
out_folder = "$target_gen_dir/resources"
```
Demetrios Papadopouloswe would still need a copy target to rename app.rollup.js to app.js inside that directory, no?
build_webui() does this by leveraging `resource_path_rewrites` avoiding any extra copies, see [1]. As said earlier, not sure if you can leverage that here though.
thanks for clarifying! Done.
GetContentClient()->browser()->ShouldAllowMojoJsBindingsForURL(Since you are passing a site URL, this should probably be "ForSite" as it won't support a full URL and we won't enable/disable bindings based on anything beyond origin or site. It is also the suffix commonly found in the API to the embedder.
- Demetrios Papadopoulos
- Devlin Cronin
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 Code-Review+1![]( "Open in Gerrit")
- Devlin Cronin
- Moe Ahmadi
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
LGTM with a question. What is the test story for the extension version? Do you plan to port the WebUI tests to be run in the context of the extension in a follow-up? Or will the tests be copied? (Given that this is quite an unusual setup, running the tests in two different contexts sounds complicated, probably harder than making the prod code work)
Related details
- Devlin Cronin
- Moe Ahmadi
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 9 comments![]( "Open in Gerrit")
Thanks, Moe!
I didn't look in detail at the aim_elegibility_private implementation -- would you like me to review that, too, or is someone else going to handle that one?
if (site_url.SchemeIs("chrome-extension")) {kExtensionScheme
"//chrome/browser/extensions/api/aim_eligibility_private",non-api code should not depend on API code if we can help it. Can we restructure this so there aren't dependencies from //c/b/extensions -> //c/b/extensions/api?
# APIs supported on Win/Mac/Linux but NOT desktop Android go here.is the intention for this to never be supported on desktop android? If so, put this in the !android block below (line 114ish).
# Copyright 2026 The Chromium AuthorsCan we add an OWNERS file to this directory?
// extensions via direct Mojo binding, completely bypassing JSON extension APIs.JS extension APIs?
Also, it doesn't bypass them -- they're still there. It just allows direct communication in addition to APIs.
render_frame_host->EnableMojoJsBindings(nullptr);nit: document anonymous param
extension_misc::kGlicExtensionId,please also add the extension ID here
bool IsMojoJsEnabledForExtension(std::string_view extension_id);nit: prefer const ExtensionId&
Related details
- Moe Ahmadi
Moe Ahmadi (Gerrit)
Moe Ahmadi added 10 comments![]( "Open in Gerrit")
LGTM with a question. What is the test story for the extension version? Do you plan to port the WebUI tests to be run in the context of the extension in a follow-up? Or will the tests be copied? (Given that this is quite an unusual setup, running the tests in two different contexts sounds complicated, probably harder than making the prod code work)
Thanks Demetrios! that is a great question. Ideally we would want to avoid duplicating the test logic. I imagine we'd have a single WebUI Mocha test suite and run it in both contexts during the transition. I trust you when you say that this is more complicated that it sounds :) I filed b/525170703 to track this exploration.
Thanks, Moe!
I didn't look in detail at the aim_elegibility_private implementation -- would you like me to review that, too, or is someone else going to handle that one?
Thanks Devlin! please do!
if (site_url.SchemeIs("chrome-extension")) {Moe AhmadikExtensionScheme
Done
"//chrome/browser/extensions/api/aim_eligibility_private",non-api code should not depend on API code if we can help it. Can we restructure this so there aren't dependencies from //c/b/extensions -> //c/b/extensions/api?
that's a great callout. I thought about this a bit and the solution I had in mind might add too much additional scope to this CL. If you don't mind, I filed b/525150940 to followup on this.
# APIs supported on Win/Mac/Linux but NOT desktop Android go here.is the intention for this to never be supported on desktop android? If so, put this in the !android block below (line 114ish).
Excluding from Desktop Android isn't intended. Done.
Can we add an OWNERS file to this directory?
Done
// extensions via direct Mojo binding, completely bypassing JSON extension APIs.JS extension APIs?
Also, it doesn't bypass them -- they're still there. It just allows direct communication in addition to APIs.
Removed. This was supposed to mean we can avoid adding new extension APIs in JSON/IDL files but it was missing parts and weirdly phrased. Thanks for catching it.
render_frame_host->EnableMojoJsBindings(nullptr);Moe Ahmadinit: document anonymous param
Done
please also add the extension ID here
Done
bool IsMojoJsEnabledForExtension(std::string_view extension_id);Moe Ahmadinit: prefer const ExtensionId&
Done
Related details
- Demetrios Papadopoulos
- Devlin Cronin
- Nasko Oskov
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. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted Code-Review+1![]( "Open in Gerrit")
- Devlin Cronin
- Moe Ahmadi
- Nasko Oskov
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 1 comment![]( "Open in Gerrit")
Related details
- Devlin Cronin
- Moe Ahmadi
Devlin Cronin (Gerrit)
Devlin Cronin added 9 comments![]( "Open in Gerrit")
Thanks, Moe!
if (!is_android) {Comment [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/7921547/comment/ab2b38ec_838d6e48/) says excluding on desktop android *wasn't* intended, but this omits it from Android.
Can you clarify the intention?
const raw_ptr<Profile> profile_;This can't be null -- raw_ref?
void AimEligibilityExtensionBridge::CreatePageHandler(Can you provide more details on the future plans for this? Is this "API" and this bridge going to do more in the near future?
As it is now, I'm not sure putting this in an API directory is really the right place. Since all this is doing is binding a receiver for a particular extension, and the receiver isn't really API code, it seems like it could instead live near the PageHandler itself (chrome/browser/ui/webui/omnibox/aim_eligibility), in other aim code (maybe just //chrome/browser/omnibox?), or maybe in //chrome/browser/extensions.
raw_handler->set_disconnect_handler(base::BindOnce(
[](AimEligibilityExtensionBridge* bridge,
AimEligibilityPageHandler* handler_to_erase) {
std::erase_if(bridge->page_handlers_,
[handler_to_erase](
const std::unique_ptr<AimEligibilityPageHandler>& p) {
return p.get() == handler_to_erase;
});
},
base::Unretained(this), raw_handler));This deletes the PageHandler directly within the set_disconnect_handler callback -- I assume that's safe? (i.e., the mojo calling code knows that the handler may be deleted at that point and won't use it?)
if (!profile->IsOffTheRecord() && !profile->IsSystemProfile() &&
!profile->IsGuestSession()) {The Bridge service is created for incognito and guest profiles. Should it match these restrictions?
#if BUILDFLAG(ENABLE_EXTENSIONS)Tying in with the comment [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/7921547/comment/ab2b38ec_838d6e48/), these aren't included in desktop android.
In general, new code should not use ENABLE_EXTENSIONS (or enable_extensions in GN files):
- Supported on desktop android? Use ENABLE_EXTENSIONS_CORE (or enable_extensions_core)
- Not supported on desktop android? Use !IS_ANDROID (or !is_android)
return extension_id == extension_misc::kAimEligibilityExtensionId;Ideally, we'd also confirm that the extension was the component extension (and not an unpacked extension that the user sideloaded).
Can we retrieve the extension from the ExtensionRegistry (fetched from the profile) and pass it in here to check that?
"aimEligibilityPrivate"this permission doesn't exist and doesn't do anything. Is the intention to add an API in the future? If so, we could add the permission then.
Related details
- Moe Ahmadi
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. |
Moe Ahmadi (Gerrit)
Moe Ahmadi added 9 comments![]( "Open in Gerrit")
"//chrome/browser/extensions/api/aim_eligibility_private",Moe Ahmadinon-api code should not depend on API code if we can help it. Can we restructure this so there aren't dependencies from //c/b/extensions -> //c/b/extensions/api?
that's a great callout. I thought about this a bit and the solution I had in mind might add too much additional scope to this CL. If you don't mind, I filed b/525150940 to followup on this.
Resolving to follow-up on the bug.
Comment [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/7921547/comment/ab2b38ec_838d6e48/) says excluding on desktop android *wasn't* intended, but this omits it from Android.
Can you clarify the intention?
that's correct. I don't intent to exclude this from desktop android intentionally. I'm not entirely sure why I moved the dependency here. thanks for catching that.
This can't be null -- raw_ref?
Good callout. Done.
Can you provide more details on the future plans for this? Is this "API" and this bridge going to do more in the near future?
As it is now, I'm not sure putting this in an API directory is really the right place. Since all this is doing is binding a receiver for a particular extension, and the receiver isn't really API code, it seems like it could instead live near the PageHandler itself (chrome/browser/ui/webui/omnibox/aim_eligibility), in other aim code (maybe just //chrome/browser/omnibox?), or maybe in //chrome/browser/extensions.
I agree putting this in the api/ directory is not the right place since it's not a real extension API. I moved the bridge files to `c/b/ui/webui/omnibox/aim_eligibility/extension/` where they logically belong. Though, we are still left with a dependency from `c/b/extensions` and `c/b/extensions/api` to `c/b/ui/`.
I'm planning to follow up in b/525150940 by introducing an interface binder registry such that AIM eligibility extension bridge can register Mojo binder callbacks and eliminate the dependency that way. Since this is temporary I am using `// nogncheck` on the includes not to pollute the build graph. Feel free to reopen or share feedback on the bug.
raw_handler->set_disconnect_handler(base::BindOnce(
[](AimEligibilityExtensionBridge* bridge,
AimEligibilityPageHandler* handler_to_erase) {
std::erase_if(bridge->page_handlers_,
[handler_to_erase](
const std::unique_ptr<AimEligibilityPageHandler>& p) {
return p.get() == handler_to_erase;
});
},
base::Unretained(this), raw_handler));This deletes the PageHandler directly within the set_disconnect_handler callback -- I assume that's safe? (i.e., the mojo calling code knows that the handler may be deleted at that point and won't use it?)
Thanks for checking. this should be safe since the disconnect handler should be run after the pipe is closed. Deleting mojo::Receiver at any time should be safe too as it also closes the pipe immediately.
if (!profile->IsOffTheRecord() && !profile->IsSystemProfile() &&
!profile->IsGuestSession()) {The Bridge service is created for incognito and guest profiles. Should it match these restrictions?
Done. Cleaned up the test harness to retrieve the mock service from the active profile directly.
Tying in with the comment [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/7921547/comment/ab2b38ec_838d6e48/), these aren't included in desktop android.
In general, new code should not use ENABLE_EXTENSIONS (or enable_extensions in GN files):
- Supported on desktop android? Use ENABLE_EXTENSIONS_CORE (or enable_extensions_core)
- Not supported on desktop android? Use !IS_ANDROID (or !is_android)
Thanks for explaining, the macros and the build flags make sense to me now. I admit I could've read the comments more closely.
return extension_id == extension_misc::kAimEligibilityExtensionId;Ideally, we'd also confirm that the extension was the component extension (and not an unpacked extension that the user sideloaded).
Can we retrieve the extension from the ExtensionRegistry (fetched from the profile) and pass it in here to check that?
Done.
this permission doesn't exist and doesn't do anything. Is the intention to add an API in the future? If so, we could add the permission then.
- Demetrios Papadopoulos
- Devlin Cronin
- Nasko Oskov
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. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
1 comment
LGTM; thanks, Moe!
As long as you're planning on cleaning up the deps issues soon, I'm okay with the (temporary) tech debt.
I'm a bit nervous about landing the impl + default-enabled feature in one CL (I'd lean towards landing it disabled and enabling in a follow up, for quick-and-easy reverting if necessary), but I'll trust your judgment on that one.
Related details
- Demetrios Papadopoulos
- Moe Ahmadi
- Nasko Oskov
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. |
Moe Ahmadi (Gerrit)
Moe Ahmadi added 2 comments![]( "Open in Gerrit")
Thanks, Moe!
I didn't look in detail at the aim_elegibility_private implementation -- would you like me to review that, too, or is someone else going to handle that one?
Thanks Devlin! please do!
Done
LGTM; thanks, Moe!
As long as you're planning on cleaning up the deps issues soon, I'm okay with the (temporary) tech debt.
I'm a bit nervous about landing the impl + default-enabled feature in one CL (I'd lean towards landing it disabled and enabling in a follow up, for quick-and-easy reverting if necessary), but I'll trust your judgment on that one.
Thanks! your feedback here has been really helpful in finding out what's needed for a sustainable solution. I'll write a short doc to align on the dependency and layering with you. agreed keeping the feature disabled is the right thing to do here, given this is meant as a proof of concept.
Related details
- Demetrios Papadopoulos
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |