✨ Assist.js: Add a shared FrameService to assistjs extension #32784
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…sistant-assistjs. It consists of one shared service and multiple custom elements.
runtime dependency in package.json
- Add appropriate validator rules for the new extension - Add more detailed description about what this extension does
… Also includes some minor refactoring. The service provides config to other components and the element take prespecified config json.
- Ensure config json retrieval - Move widget iframe url creation into one single place. - Add assertion to ensure there's only one config element on the page Minor fixes: - Replace await usage with then to avoid extra dependencies. - Update validator rules for the invisible config element
…rvice.js Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
…aseElement). Also add a config service mock to the unit test.
- Parse config in config element to save a DOM query - Delete assistjs-frame-service.js. Add it in next PR with real usage.
…from underlying iframes. Actual implementation of two endpoints: OpenMic and SendTextQuery is empty now and will be implemented in upcoming PRs.
samouri
reviewed
Feb 22, 2021
|
|
||
| /** @private */ | ||
| createAssistantIframe_() { | ||
| document.addEventListener('DOMContentLoaded', () => { |
There was a problem hiding this comment.
@jridgewell: is it important to use ampdoc.whenReady() ==> Promise instead of DOMContentLoaded?
There was a problem hiding this comment.
Changed to whenReady() because it seems like it is necessary. I double checked and realized that without it the iframe never gets created. I guess DOMContentLoaded might somehow got intercepted.
| createAssistantIframe_() { | ||
| document.addEventListener('DOMContentLoaded', () => { | ||
| this.configService_ = Services.assistjsConfigServiceForDoc(this.ampDoc_); | ||
| const iframe = this.win.document.createElement('iframe'); |
There was a problem hiding this comment.
can we add sandbox to the iframe? And only give permissions it needs
Add sandbox attributes to all iframes.
samouri
approved these changes
Feb 22, 2021
jridgewell
approved these changes
Feb 23, 2021
extensions/amp-google-assistant-assistjs/0.1/assistjs-frame-service.js
Outdated
Show resolved
Hide resolved
jridgewell
requested changes
Feb 23, 2021
extensions/amp-google-assistant-assistjs/0.1/assistjs-frame-service.js
Outdated
Show resolved
Hide resolved
…rvice.js Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
…rvice.js Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
jridgewell
approved these changes
Feb 24, 2021
|
Seems like I don't have the permission to merge even all checks are passed. @samouri @jridgewell could you help me merge this? Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The new service enables all custom elements to handle requests from underlying iframes by talking to the Assistant iframe. The implementation of two endpoints(OpenMic and SendTextQuery) is empty now and would be implemented in the upcoming PRs(after the message channel between main document and iframes are constructed).
Assist.js AMP integration I2I issue: #30631
@samouri
/cc @ianba