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.
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
Proposal: Multiple user script worlds #560
Proposal: Multiple user script worlds #560
Changes from 4 commits
6b2f9b8
ad67d8c
f0f6c69
316c349
0ce3b97
c50d727
d087168
85e1235
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Done
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.
Important design question: what are the persistence/lifetime characteristics of worldId + configureWorld? worldId should not persist across extension updates for consistency with the existing userScripts API design (https://developer.chrome.com/docs/extensions/reference/api/userScripts#extension_updates), but there is still an open question of the lifetime of worldId (and consequently, potentially reduce the duration of persistence).
Previously,
configureWorld
would have a limited scope, exactly one. Now, with the introduction ofworldId
, there may be an unbounded number. According to the current proposal, the concept of a specific world (identified by worldId) becomes concrete whenever it is used inRegisteredUserScript
orWorldProperties
. Because the number of worlds is now dynamic, we should clarify when a worldId exists and when it ceases to exist. As currently written, worldIds are instantiated and never discarded.This section seems to have been written with
userScripts.register
in mind. The issue that I'm thinking of would have become much more obvious if the proposal explicitly spelled out the expected behavioral change (or non-change) per method (from the proposal process perspective - I think that it would be a good idea for future proposals to exhaustively enumerate all uses of a type to make sure that the behavior is fully specified where needed):userScripts.register()
- may introduce a worldId. Does not necessarily have to do anything until the script is used in the renderer (unless there is a desire to broadcast all known worldIds?).userScripts.configureWorld()
- may create a persistent configuration for a world. What if the worldId does not exist yet?userScripts.getScripts()
- does this have to "create a separate user script world"?userScripts.unregister()
- when the last use ofworldId
disappears, would that delete the configuration?userScripts.update()
- when theworldId
changes, and the previous one was the last one, does that delete the registered configured worlds?I think that there are roughly two ways to address this issue (not mutually exclusive):
userScripts.registerWorld
+userScripts.unregisterWorld
). Using an unregisteredworldId
in RegisteredUserScript or WorldProperties is an error.userScripts.unregister
oruserScripts.update
).userScripts.configureWorld
beforeuserScripts.register
? If so, thenconfigureWorld
should register a world implicitly, despite there not being any scripts that reference it.configureWorld
unless the worldId has been allocated explicitly byuserScripts.register
. Extensions that really care about ensuring setup of the world before execution can register a dummy script with a worldId, then configure the world, then register the actual script for that world, and then unregister the dummy script. Is this an acceptable pattern?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.
Thanks, @Rob--W !
I was thinking about most of these already (complete with a few TODOs, e.g. here) and was planning on submitting an update to the proposal as we got nearer, but I hadn't considered the update behavior and allowing developers to clear out old registrations -- both of which I agree are valuable.
The way I'm thinking about worlds is that:
As such, there isn't really the "creation" of a world when you register a script -- worlds are created in the document in which they run, and are created according to the configuration provided (if any).
I don't like the idea of either reference counting or requiring a user script be registered for every specified worldId -- that sounds very difficult for both us (the browser) and for developers to keep track of, and seems like it would very likely lead to some unpredictable or unexpected edge cases (e.g., a developer updates a user script world via unregister + register, and doesn't realize that unregistered the user script world). It also leads to a (very small) race where it'd be impossible to ensure a user script injected with given world properties, since you'd have to register the script before the configuring the world, but then the script may inject before that configuration took hold. I'd much rather lean on the developers handling the registration.
I've expanded the proposal to include:
removeWorld()
andgetWorldConfigurations()
.Please take a look and let me know what you think.