r100497 MediaWiki - Code Review archive

I have to extends OutputPage to use $out->makeResourceLoaderLink as it's protected. Since GadgetHooks is used statically only I see no harm.

I remember there was a bug report about this but I cannot find it now.

This should be reverted and done properly, by overriding ResourceLoaderModule::getPosition() in GadgetResourceLoaderModule.

Hmm, wait, it's not that easy. This is about loading just the styles in the head, not the scripts.

Still, what's wrong with using $out->addModuleStyles() here? Why must you use $out->addHeadItem() in conjunction with makeResourceLoaderLink()?

Styles added by $out->addModuleStyles() are loaded together with other RL styles. Gadget modules are in 'other' group, and in OutputPage.php, it says (in comments): "So the order has to be other, dynamic, site, private, user" so Gadget modules will be loaded firstly if they're added with $out->addModuleStyles(). However some of our existing gadgets depend on the fact that gadget styles are loaded after site (and maybe user) styles so site styles can be overridden. So I have to place gadget styles in HeadItems so they can be loaded after other styles which are loaded in CssLinks.

Also, the way this revision implements things, scripts will be loaded in the head as well, due to the usage of addModuleScripts(). This was probably unintended, right?

(I'm sorry for the comment flood.)

In my test, scripts are still loaded at the end. MediaWiki r100693.

Oh, right, I forgot what addModuleScripts does exactly.

Still, the way it's being done here is not optimal. I'll think about a better way to do this.

It looks like the addModuleScripts() usage here is breaking dependency resolution:

[11:53:18] <Beau_>	is resource loader or gadgets extension broken in trunk ? 
[11:53:34] <RoanKattouw>	Why?
[11:53:46] <Beau_>	it ignores dependencies
[11:54:06] <Beau_>	I have a gadget depending on mediawiki.util
[11:54:14] <Beau_>	and it gets loaded before it
[11:54:24] <RoanKattouw>	That's strange
[11:54:34] <RoanKattouw>	What's the definition for that gadget in Gadgets-definition?
[11:54:53] <Beau_>	* wikibugs [ResourceLoader | default | dependencies=mediawiki.util,mediawiki.user,jquery.ui.dialog] | JsMwApi.js | wikibugs.js | wikibugs.css
[11:57:34] <Beau_>	RoanKattouw: I have turned on debug and i get: [http://p.defau.lt/?_2Ca6g5lRMNdgLDM6IkqUA http://p.defau.lt/?_2Ca6g5lRMNdgLDM6IkqUA]
[11:57:44] <Beau_>	without debug it does not work either

Content of pastie:

<script src="[http://localhost/mwdev/load.php?debug=true&lang=pl&modules=ext.gadget.wikibugs&only=scripts&skin=vector&* http://localhost/mwdev/load.php?debug=true&lang=pl&modules=ext.gadget.wikibugs&only=scripts&skin=vector&*]"></script>
<script src="[http://localhost/mwdev/load.php?debug=true&lang=pl&modules=skins.vector&only=scripts&skin=vector&* http://localhost/mwdev/load.php?debug=true&lang=pl&modules=skins.vector&only=scripts&skin=vector&*]"></script>
<script>if(window.mw){
    mw.loader.load(["mediawiki.user", "mediawiki.util", "mediawiki.page.ready", "mediawiki.legacy.wikibits", "mediawiki.legacy.ajax", "mediawiki.action.watch.ajax", "ext.proofreadpage.article"]);
}

(bah, the CR parser is screwing up the URLs, but the point should be clear)

Fixed in r101827. I was wrong that I thought styles in gadgets are loaded after site and user css (actually they're loaded before those in both ResourceLoader and non-ResourceLoader gadgets), so it's safe to use addModuleStyles.

Since addModuleStyles and addModuleScripts don't check dependencies, I have to use addModules, but I kept addModuleStyles for the reason of FOUC. But will this delay page load?

I guess a new option for gadgets declaring their styles will cause FOUC so the extension can put their styles on the top of pages.

What problem is attempted to be solved here ?

If a gadget needs styling, there shuld be no FOUC as ResourceLoader implements the style properties before executing the JavaScript, so it shouldb'e be possible for something to be unstyled, and if it is, it's not related to Gadgets.

If it is a gadget styling something already on the page by default, then it should probably be a gadget that is in it's entirety with "position" property of the module set to "top" (this is not settable from the definitions page in the MW1.19 version of the extension, but will be in the RL2 version).

I didn't have a look at the new version, but the problem to solve is:

• A gadget styling something already on the page -- was causing FOUC
• or, a CSS gadget styling page in a browser that doesn't support JS but supports CSS -- was showing no styles at all

• Styling core elements (most likely a CSS-only gadget):

This will be fixed by RL2, which allows a gadget to set the load position to "top", which will make it load from the top queue (with JavaScript, in the same request as most other modules), which will be loaded before the page is rendered.

• CSS for any gadget in browsers without JS

Interesting case, although one could argue:

• Browsers without JS aren't officially supported, although it is the goal to preserve core article reading in a pretty way for those browsers. Gadgets don't fall under that imho.
• Adding a separate http request for all css sheets from the HEAD for each gadget doesn't seem ideal
• By being in a ‎<link> in the HTML itself means cache updates are not propagated as fast as other parts of the module, which is exactly why ResourceLoader's client-side component uses dynamic loading through JavaScript powered by the startup module. Using a LINK undoes all those advantages and will cause this style to be updates less frequently, become part from the html output, and likely have a shorter cache duration (not as good for performance, although not a huge deal for logged-in only gadgets but we have the ability to load gadgets for anonymous users and all logged-in users as well, so I think it's worth considering not allowing gadgets to insert an additional LINK tag for this reason as it goes against the very principle that ResourceLoader is trying to acomplish).
• Potential risk of CSS and JS of a gadget going out of sync.