Last modified: 2014-02-12 23:38:25 UTC
Activation of mwEmbed at Wikimedia Commons leads to some bugs. Therefore, I call it a regression. Writing this bug report presumely took me more time than fixing the bug but if I don't get dev-access, it's not my problem. Browsers: all. OS: all. Instead of adding one "EmbedPlayer"-object, it adds "EmbedPlayer.Attributes", "EmbedPlayer.AttributionButton" and so on. Cluttering the global scope is considered bad practise ( https://www.google.com/search?q=javascript+clutter+window+scope ) and accessing global scope variables is slow. Let's illustrate this issue in JavaScript: // Currently it does: window['EmbedPlayer.Attributes'] = { /* an object */ }; window['EmbedPlayer.AttributionButton'] = { /* an object */ }; // But it should window.EmbedPlayer = { Attributes: { /* an object */ }, AttributionButton: { /* an object */ } }; // This would allow to "transfer" the variable to a local scope (actually creating a new reference to window.EmbedPlayer) e.g. function abc() { var ep = window.EmbedPlayer; if (ep.Attributes) // fast variable access because it is in local function scope and also allows to use abbrevations }
you can create an account for https://gerrit.wikimedia.org and push changes for review. (http://www.mediawiki.org/wiki/Developer_access) if thats to much you can also clone the git repository and attach a patch here.
as for your bug: Mediawiki currently still registers all config options as global variables, this has been fixed, but since not all code is updated it still defaults to register them as global variables: http://www.mediawiki.org/wiki/Manual:$wgLegacyJavaScriptGlobals this has nothing to do with TimedMediaHandler. TMH accesses those values only through mw.config.get/set.
Created attachment 11488 [details] One page of global variables that should be part of a mwEmbedConfig object Manual hin oder her... Upload Wizard does it right: It puts everything in "window.UploadWizardConfig" and "mw.UploadWizard*" Global mw bad practise is no excuse.
As Jan outlined, this is an issue with core, not TMH. Presently core puts every configuration option into the global name-space because of legacy globals. Core mediaWiki.js should be fixed to no longer do this. Set Component to Resource Loader.
(In reply to comment #0) > Activation of mwEmbed at Wikimedia Commons leads to some bugs. Therefore, I > call it a regression. What bugs? (In reply to comment #2) > as for your bug: > Mediawiki currently still registers all config options as global variables, > this has been fixed, but since not all code is updated it still defaults to > register them as global variables: > http://www.mediawiki.org/wiki/Manual:$wgLegacyJavaScriptGlobals > > this has nothing to do with TimedMediaHandler. > TMH accesses those values only through mw.config.get/set. (In reply to comment #4) > As Jan outlined, this is an issue with core, not TMH. Presently core puts > every configuration option into the global name-space because of legacy globals. Indeed. I'm leaving this bug open pending more information regarding the "lead to some bugs". Otherwise, duplicate of bug 33837.
> What bugs? Was a default text I used while writing bug reports about mwEmbed or TMH. > Mediawiki currently still registers all config options as global variables Yes. But this is not a reason not to put all your config stuff into *a single variable*, an *object* consisting of *properties*. If this is not possible in LocalSettings.php, it seems to be superior design to put it in something like UploadWizard.config.php (so TMH.config.php) If UploadWizard does it (yes it has *one global variable* with _all its config in it_), there must be a way. Why else should you do this? Of course to save transfer volume. If you make variables like these, their name will be always written as full strings into JavaScript. And when accessing them, it is also likely that you use the full string or some strange hack to avoid it. Furthermore, these strings can't be replaced when you e.g. decide to introduce code compression (closure compiler) while when doing something like suggested in #c0, "EmbedPlayer" can be shortened to 1 char.
BTW, please don't "re-target" this again or reinterpret what I asked for. It was about TMH and I don't ask for/ demand/ support turning off the LegacyJavaScriptGlobals without an extensive announcement and some automated script which does its best to assist users/ or admins to keep their site usable and useful. It would make crash 75% of the larger user scripts and 20% of the gadgets >25% of other JS code in MediaWiki-namespace at Commons, I guess.
We have a flat config namespace, because we don't double wrap the "config.get" method, and the mw.config.get method has no way to specify retrieval nested config .. i.e mw.config.get('EmbedPlayer')['MySubConfigOption'] will throw an error if the config does not exist instead of returning null. I don't think its irregular to have a flat config key value pair setup. i.e there are advantages in not having requiring JSON parsing everywhere you can set config. We read the config outside of "EmbedPlayer" interface, for example specifying what tags we want to rewrite before we load the rest of player interface. So we would need a nested config helper outside of the EmbedPlayer object, we can't simply load embedPlayer config per player interface. The flat config relates to global config across players. When we set a global config it applies to all players ( not imported per player ) Properties specific to the player interface do use nested config .. for example EmbedPlayer.Attributes is an object with all the player attributes. Per Comment 6, you don't save much on transfer volume, gzip is going to take care of repeating patterns. Per Comment 7, feel free to change the title of the bug to "store all TMH configuration in a single config key with object value sets" if your bug does not have to do with "cluttering the global window scope". That being said, In principal, I don't have anything against what you outline,, but is not easily accomplished because upstream we use flat config as well. Take a look at this page and how we support this config in 3 different places: http://html5video.org/wiki/Kaltura_HTML5_Configuration
