-
-
Notifications
You must be signed in to change notification settings - Fork 881
web: Fix the config negotation between self-hosted and the extension #17346
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
base: master
Are you sure you want to change the base?
web: Fix the config negotation between self-hosted and the extension #17346
Conversation
688444a
to
47c2cff
Compare
: {}), | ||
...(window.RufflePlayer?.config ?? {}), | ||
// This will make extension config take precedence, if I move this one line higher self-hosted config will take precedence | ||
// Let the discussions begin |
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.
Comment is here to facilitate discussion. Start discussing below. This has come up before. Site owners have come out against it, mainly arguing against showSwfDownload
being able to be changed by end-users. However, it is true that end users can usually just check the network tab, making this just a convenience method.
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.
The extension config taking precedence is fine with me 👍
47c2cff
to
a43b737
Compare
this.loadedConfig?.backgroundColor && | ||
this.loadedConfig?.wmode !== WindowMode.Transparent |
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.
See the new getter.
ec4789a
to
4fb452b
Compare
unmuteOverlay: "hidden", | ||
splashScreen: false, | ||
} | ||
: {}; |
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.
The idea here, much like explicitOptions
, is to only set these options if autostart is not the default (false), so if a self-hosted config sets autoplay to "on", for example, it's not ignored when the extension is used.
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 is not important if the self-hosted config takes precedence over the extension toggles as is currently the case.
@@ -858,6 +849,32 @@ export class InnerPlayer { | |||
} | |||
} | |||
|
|||
// The effective config loaded upon `load`, possibly changing if `window.RufflePlayer` changes later. |
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.
loadedConfig
needs to be able to change after load
in the event that load
is run either before the extension loads or before a self-hosted script that sets window.RufflePlayer.config
runs.
@@ -23,6 +23,7 @@ declare global { | |||
*/ | |||
export interface PublicAPILike { | |||
config?: DataLoadOptions | URLLoadOptions | object; | |||
extensionConfig?: DataLoadOptions | URLLoadOptions | object; |
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.
Two separate configs, one for the extension and the other for self-hosted, ensures the values of one can not override the values of the other. We can ensure the values in the extension don't override the values of self-hosted without a separate config, as we control the way the values in the extension config are set. However, we can not do the same in reverse, as we can't dictate how self-hosted config values are set.
if (window.RufflePlayer.config === undefined) { | ||
window.RufflePlayer.config = {}; | ||
} | ||
window.RufflePlayer.config = { |
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.
window.RufflePlayer.config
is now explicitly the self-hosted config.
I've thought about this before, and never added it because it just didn't feel right. Today looking at it in code, I'm still a little hesitant. I think I'd prefer something like We can then in the extension have some things that don't override the websites wishes (insert with low prio), and some that do (insert with high prio). Log level absolutely should change the log level, but "download swf" is the only debatable one afaik. Websites like the wayback machine can then also use this to better give their preferences without a website replacing their wishes. Assuming, of course, a website doesn't just blindly go |
4fb452b
to
95e0e04
Compare
The browser is still the user agent. You host it publicly, I can access it. If I can access it to play it, my browser will store it anyway in it's cache, so if I want to, I will keep it to myself. No need/point to make it unnecessary complicated by me having to open the network tab in the developer tools or whatever... |
95e0e04
to
8d15306
Compare
8d15306
to
4ac0e4b
Compare
4ac0e4b
to
6217e83
Compare
No description provided.