MV3: Move translation from sandboxed content iframe to offscreen #527
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
Offscreen page doesn't support eval, so we are actually embedding a sandbox iframe there which then supports eval so we can run the translate architecture there. Message passing is technically complicated: Content Scripts <-chrome.runtime.sendMessage-> Background Service Worker (background-worker.js) <-MessagePort-> Offscreen Sandbox Iframe (offscreenSandbox.html) The MessagePort is created by Offscreen Page (offscreen.html) and the two ports are passed respectively to to Offscreen Sandbox Iframe and Background page after which offscreen.html only performs the function of embedding the Offscreen Sandbox and reinitializing the MessagePort in case the Background Service Worker gets restarted by the browser. We are moving translate to offscreen sandbox because the sandbox iframe on content pages are causing issues on some websites where their pages refuse to load due to loading code finding unexpected elements. Also: Added a wrapper to Zotero.Translate.Web which makes translate only be concerned with scrapping the webpage (i.e. the stuff we need to be able to run eval for). Scrapped item saving to Zotero and UI notifications are now handled independently. This was done because 1. The Zotero Translate architecture has no business in doing that stuff anyway 2. If ItemSaver runs in the Offscreen Translate page it then needs to be able to independently send progress callback messages to the tab where translation occurs, but we have no way to pass the tab information with which to communicate to ItemSaver if it is managed by Zotero.Translate, without changing the translate repo. Passing around that information via Zotero.Translate is dumb and creates even more insane and difficult to track interdependencies in translate architecture. As part of this some refactoring to Zotero.Inject also occurred as that namespace has troubled me for a while as more and more random unrelated hard-to-place functions got crammed there. All page saving related functionality has been moved to pageSaving.js
adomasven
force-pushed
the
mv3-offscreen-page-translate
branch
from
c48fee3 to
315239f
Compare
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.
Closes #522, Closes #483
We are moving translate to offscreen sandbox because the sandbox iframe on content pages are causing issues on some websites where their pages refuse to load due to loading code finding unexpected elements.
Translation occurs in a sandboxed iframe embedded in the offscreen page. The offscreen page itself doesn't allow running evals.
Message passing is technically complicated:
Content Scripts <-chrome.runtime.sendMessage->
Background Service Worker (background-worker.js) <-MessagePort->
Offscreen Sandbox Iframe (offscreenSandbox.html)
In practice however it's handled gracefully and very robustly by the generic message passing framework (messagePassing.js)
The MessagePort is created by Offscreen Page (offscreen.html) and the two ports are passed respectively to to Offscreen Sandbox Iframe and Background page after which offscreen.html only performs the function of embedding the Offscreen Sandbox and reinitializing the MessagePort in case the Background Service Worker gets restarted by the browser.
Also:
Added a wrapper to Zotero.Translate.Web which makes Translate only be concerned with scrapping the webpage (i.e. the stuff we need to be able to run eval for). Scrapped item saving to Zotero and UI notifications are now handled independently after scrapping.
This was done because
As part of this some refactoring to Zotero.Inject also occurred as that namespace has troubled me for a while as more and more random unrelated hard-to-place functions got crammed there. All page saving related functionality has been moved to pageSaving.js
Tested translation on Chrome, Firefox and Safari (since the refactoring technically affects all of them). Safari needs a complementary commit (zotero/safari-app-extension@7b98af6).
Also tested for any performance impacts/downgrades. Spending around 20ms to run detection for a pageload that takes around 1s, which is good. I didn't check performance for the previous approach but given that we do not need to create an additional iframe for every page this will overall be faster.