-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sandbox offscreen poc #9352
Sandbox offscreen poc #9352
Conversation
Thoughts on this approach @fregante ? Do you see any gotchas with moving the sandbox execution to the off screen page? This is still missing better type checking, error handling/retries and tests, but the main flow works for nunjucks in this poc |
51273f4
to
7144016
Compare
Playwright test resultsDetails Open report ↗︎ Failed testschrome-setup › setup/affiliated.setup.ts › authenticate with affiliated user Skipped testschrome › tests/bricks/sidebarEffects.spec.ts › sidebar effect bricks › toggle sidebar brick |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9352 +/- ##
==========================================
+ Coverage 74.24% 75.20% +0.96%
==========================================
Files 1332 1373 +41
Lines 40817 41859 +1042
Branches 7634 7778 +144
==========================================
+ Hits 30306 31482 +1176
+ Misses 10511 10377 -134 ☔ View full report in Codecov by Sentry. |
You should probably be able to skip the internal sandboxed iframe since offscreen documents have limited API access as well.
|
@@ -88,3 +89,4 @@ initModUpdater(); | |||
initWalkthroughModalTrigger(); | |||
void initRestrictUnauthenticatedUrlAccess(); | |||
initStateControllerListeners(); | |||
void ensureOffscreenDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to create a new document every time the service worker is restarted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a quick hack to just show that it can be done. As you note, we should make sure that this is only done once
We should double-check with the Chrome DevRel team to see whether that's acceptable under their remote code policy. This page on remote-code doesn't mention off-screen documents: https://developer.chrome.com/docs/extensions/develop/migrate/remote-hosted-code I think we might need to use the sandboxed iframe to support concurrency with 1/ error reporting code, and 2/ our voice streaming code that's also in the offscreen document
Some considerations (not blockers):
|
What does this PR do?
Discussion
Demo
Future Work
Checklist
Leave all that are relevant and check off as completed
For more information on our expectations for the PR process, see the
code review principles doc