Enable load-time data support for component extensions [chromium/src : main]
Moe Ahmadi (Gerrit)
Moe Ahmadi voted and added 3 comments![]( "Open in Gerrit")
Votes added by Moe Ahmadi
| Commit-Queue | +1 |
3 comments
rdevlin.cronin: //c/b/extensions and extensions/browser
dpapad: //c/b/resources/omnibox and //c/b/ui/webui/omnibox
Moe AhmadiOverall this approach is addressing my previous concern from https://chromium-review.googlesource.com/c/chromium/src/+/8027188/comment/e2862f6a_199652fe/, as it no longer involves WebUIDataSource to serve strings.m.js to component extensions.
Since most of the changes now reside within c/b/extensions/ I'll let Devlin comment on the rest.
(See one open question below)
thanks Demetrios. responded to your comment. Do you mind reviewing the aim_eligibility WebUI and resources so Devlin can focus on the extensions?
- Introduce dynamic /strings.m.js module loading and dynamic $i18n{key}
template replacements for component extensions.Moe AhmadiI am a bit confused about this part. AFAICT $i18n{} template replacements already work in component extensions, or to be precise they are already used in the PDF Viewer component extension. What exactly is being added here that is not already supported with regards to `$i18n{}` template replacements?
You're right that $i18n{} replacements already work for the PDF Viewer. However, the existing mechanism only supports global data computed once at startup. What this CL adds that is not currently supported is (1) profile-scoped data (e.g., account eligibility or enterprise policy) and (2) clean layering that doesn't need direct dependency on feature-specific utilities (e.g., pdf_extension_util::) in //c/b/extensions.
let me know if that makes sense and if you prefer I clarify that in the CL description.
Related details
- Demetrios Papadopoulos
- Devlin Cronin
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")
dict.Set("showFooter", true);Add a comment about this being done for illustration purposes only and not actually needed?
"(() => {"
" const app = document.querySelector('aim-eligibility-app');"
" if (!app) return null;"
" if (typeof app.showFooter_ === 'undefined') return null;"
" return app.showFooter_;"
"})()");Injecting JS code from C++ is not the recommended way of writing tests, definitely not for WebUI, but also not for the PDF Viewer component extension. Could these tests be migrated to tests similar to https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/data/pdf/, which even though they don't use Mocha like WebUI tests yet, they at least use TypeScript for all the test code?
source->AddBoolean("showFooter", true);Either add a comment here as well about this not being needed, or remove it from here, and change the UI code to also call loadTimeData.valueExists(...) first? Having this here is not very helpful (we already know loadTimeData works in WebUI contexts).
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. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
- Introduce dynamic /strings.m.js module loading and dynamic $i18n{key}
template replacements for component extensions.Moe AhmadiI am a bit confused about this part. AFAICT $i18n{} template replacements already work in component extensions, or to be precise they are already used in the PDF Viewer component extension. What exactly is being added here that is not already supported with regards to `$i18n{}` template replacements?
You're right that $i18n{} replacements already work for the PDF Viewer. However, the existing mechanism only supports global data computed once at startup. What this CL adds that is not currently supported is (1) profile-scoped data (e.g., account eligibility or enterprise policy) and (2) clean layering that doesn't need direct dependency on feature-specific utilities (e.g., pdf_extension_util::) in //c/b/extensions.
let me know if that makes sense and if you prefer I clarify that in the CL description.
let me know if that makes sense and if you prefer I clarify that in the CL description.
Thank for the details. Yes clarifying a bit more in the CL description would help.
Just to make sure I uderstand the essence, you are saying that the way $i18n{...} worked previously was not scaleable to be used across multiple component extensions and was very specific to making it work just for PDF Viewer, whereas with the changes here it more scaleable. Is that accurate?
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. |