r85616 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r85615‎ \| r85616 \| r85617 >
Date:	12:07, 7 April 2011
Author:	catrope
Status:	ok (Comments)
Tags:
Comment:	For bug 27488: move the startup script, jquery+mediawiki and the mw.config.set() call for configuration variables back to the <head> . Let modules control whether they're loaded in the <head> ('top') or at the bottom of the <body> ('bottom') through the position parameter/property Also rearranges the loading order a little bit such that only=messages comes before only=scripts, and config comes before everything except startup and jquery+mediawiki
Modified paths:	/trunk/phase3/includes/OutputPage.php (modified) (history) /trunk/phase3/includes/Skin.php (modified) (history) /trunk/phase3/includes/resourceloader/ResourceLoaderFileModule.php (modified) (history) /trunk/phase3/includes/resourceloader/ResourceLoaderModule.php (modified) (history) /trunk/phase3/resources/Resources.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
—	—	@@ -390,15 +390,17 @@
391	391	* Filter an array of modules to remove insufficiently trustworthy members, and modules
392	392	* which are no longer registered (eg a page is cached before an extension is disabled)
393	393	* @param $modules Array
	394	+ * @param $position String if not null, only return modules with this position
394	395	* @return Array
395	396	*/
396		~~- protected function filterModules( $modules, $type = ResourceLoaderModule::TYPE_COMBINED ){~~
	397	+ protected function filterModules( $modules, $position = null, $type = ResourceLoaderModule::TYPE_COMBINED ){
397	398	$resourceLoader = $this->getResourceLoader();
398	399	$filteredModules = array();
399	400	foreach( $modules as $val ){
400	401	$module = $resourceLoader->getModule( $val );
401	402	if( $module instanceof ResourceLoaderModule
402		~~- && $module->getOrigin() <= $this->getAllowedModules( $type ) )~~
	403	+ && $module->getOrigin() <= $this->getAllowedModules( $type )
	404	+ && ( is_null( $position ) \|\| $module->getPosition() == $position ) )
403	405	{
404	406	$filteredModules[] = $val;
405	407	}
—	—	@@ -410,12 +412,13 @@
411	413	* Get the list of modules to include on this page
412	414	*
413	415	* @param $filter Bool whether to filter out insufficiently trustworthy modules
	416	+ * @param $position String if not null, only return modules with this position
414	417	* @return Array of module names
415	418	*/
416		~~- public function getModules( $filter = false, $param = 'mModules' ) {~~
	419	+ public function getModules( $filter = false, $position = null, $param = 'mModules' ) {
417	420	$modules = array_values( array_unique( $this->$param ) );
418	421	return $filter
419		~~- ? $this->filterModules( $modules )~~
	422	+ ? $this->filterModules( $modules, $position )
420	423	: $modules;
421	424	}
422	425
—	—	@@ -434,8 +437,8 @@
435	438	* Get the list of module JS to include on this page
436	439	* @return array of module names
437	440	*/
438		~~- public function getModuleScripts( $filter = false ) {~~
439		~~- return $this->getModules( $filter, 'mModuleScripts' );~~
	441	+ public function getModuleScripts( $filter = false, $position = null ) {
	442	+ return $this->getModules( $filter, $position, 'mModuleScripts' );
440	443	}
441	444
442	445	/**
—	—	@@ -454,8 +457,8 @@
455	458	*
456	459	* @return Array of module names
457	460	*/
458		~~- public function getModuleStyles( $filter = false ) {~~
459		~~- return $this->getModules( $filter, 'mModuleStyles' );~~
	461	+ public function getModuleStyles( $filter = false, $position = null ) {
	462	+ return $this->getModules( $filter, $position, 'mModuleStyles' );
460	463	}
461	464
462	465	/**
—	—	@@ -474,8 +477,8 @@
475	478	*
476	479	* @return Array of module names
477	480	*/
478		~~- public function getModuleMessages( $filter = false ) {~~
479		~~- return $this->getModules( $filter, 'mModuleMessages' );~~
	481	+ public function getModuleMessages( $filter = false, $position = null ) {
	482	+ return $this->getModules( $filter, $position, 'mModuleMessages' );
480	483	}
481	484
482	485	/**
—	—	@@ -2344,6 +2347,7 @@
2345	2348	$ret .= implode( "\n", array(
2346	2349	$this->getHeadLinks( $sk, true ),
2347	2350	$this->buildCssLinks( $sk ),
	2351	+ $this->getHeadScripts( $sk ),
2348	2352	$this->getHeadItems()
2349	2353	) );
2350	2354
—	—	@@ -2589,36 +2593,68 @@
2590	2594	}
2591	2595
2592	2596	/**
2593		~~- * Gets the global variables and mScripts; also adds userjs to the end if~~
2594		~~- * enabled. Despite the name, these scripts are no longer put in the~~
2595		~~- * <head> but at the bottom of the <body>~~
	2597	+ * JS stuff to put in the <head>. This is the startup module, config
	2598	+ * vars and modules marked with position 'top'
2596	2599	*
2597	2600	* @param $sk Skin object to use
2598	2601	* @return String: HTML fragment
2599	2602	*/
2600	2603	function getHeadScripts( Skin $sk ) {
2601		~~- global $wgUseSiteJs, $wgAllowUserJs;~~
2602		-
2603	2604	// Startup - this will immediately load jquery and mediawiki modules
2604	2605	$scripts = $this->makeResourceLoaderLink( $sk, 'startup', ResourceLoaderModule::TYPE_SCRIPTS, true );
2605		-
2606		~~- // Script and Messages "only" requests~~
2607		~~- $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleScripts( true ), ResourceLoaderModule::TYPE_SCRIPTS );~~
2608		~~- $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleMessages( true ), ResourceLoaderModule::TYPE_MESSAGES );~~
2609		-
2610		~~- // Modules requests - let the client calculate dependencies and batch requests as it likes~~
2611		~~- $loader = '';~~
2612		~~- if ( $this->getModules( true ) ) {~~
2613		~~- $loader = Xml::encodeJsCall( 'mw.loader.load', array( $this->getModules( true ) ) ) .~~
2614		~~- Xml::encodeJsCall( 'mw.loader.go', array() );~~
2615		~~- }~~
2616	2606
	2607	+ // Load config before anything else
2617	2608	$scripts .= Html::inlineScript(
2618	2609	ResourceLoader::makeLoaderConditionalScript(
2619		~~- ResourceLoader::makeConfigSetScript( $this->getJSVars() ) . $loader~~
	2610	+ ResourceLoader::makeConfigSetScript( $this->getJSVars() )
2620	2611	)
2621	2612	);
	2613	+
	2614	+ // Script and Messages "only" requests marked for top inclusion
	2615	+ // Messages should go first
	2616	+ $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleMessages( true, 'top' ), ResourceLoaderModule::TYPE_MESSAGES );
	2617	+ $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleScripts( true, 'top' ), ResourceLoaderModule::TYPE_SCRIPTS );
2622	2618
	2619	+ // Modules requests - let the client calculate dependencies and batch requests as it likes
	2620	+ // Only load modules that have marked themselves for loading at the top
	2621	+ $modules = $this->getModules( true, 'top' );
	2622	+ if ( $modules ) {
	2623	+ $scripts .= Html::inlineScript(
	2624	+ ResourceLoader::makeLoaderConditionalScript(
	2625	+ Xml::encodeJsCall( 'mw.loader.load', array( $modules ) ) .
	2626	+ Xml::encodeJsCall( 'mw.loader.go', array() )
	2627	+ )
	2628	+ );
	2629	+ }
	2630	+
	2631	+ return $scripts;
	2632	+ }
	2633	+
	2634	+ /**
	2635	+ * JS stuff to put at the bottom of the <body>: modules marked with position 'bottom',
	2636	+ * legacy scripts ($this->mScripts), user preferences, site JS and user JS
	2637	+ */
	2638	+ function getBottomScripts( Skin $sk ) {
	2639	+ global $wgUseSiteJs, $wgAllowUserJs;
	2640	+
	2641	+ // Script and Messages "only" requests marked for bottom inclusion
	2642	+ // Messages should go first
	2643	+ $scripts = $this->makeResourceLoaderLink( $sk, $this->getModuleMessages( true, 'bottom' ), ResourceLoaderModule::TYPE_MESSAGES );
	2644	+ $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleScripts( true, 'bottom' ), ResourceLoaderModule::TYPE_SCRIPTS );
	2645	+
	2646	+ // Modules requests - let the client calculate dependencies and batch requests as it likes
	2647	+ // Only load modules that have marked themselves for loading at the top
	2648	+ $modules = $this->getModules( true, 'bottom' );
	2649	+ if ( $modules ) {
	2650	+ $scripts .= Html::inlineScript(
	2651	+ ResourceLoader::makeLoaderConditionalScript(
	2652	+ Xml::encodeJsCall( 'mw.loader.load', array( $modules ) ) .
	2653	+ // the go() call is unnecessary if we inserted top modules, but we don't know for sure that we did
	2654	+ Xml::encodeJsCall( 'mw.loader.go', array() )
	2655	+ )
	2656	+ );
	2657	+ }
	2658	+
2623	2659	// Legacy Scripts
2624	2660	$scripts .= "\n" . $this->mScripts;
2625	2661
—	—	@@ -2645,7 +2681,7 @@
2646	2682	}
2647	2683	}
2648	2684	$scripts .= $this->makeResourceLoaderLink( $sk, $userScripts, ResourceLoaderModule::TYPE_SCRIPTS );
2649		-
	2685	+
2650	2686	return $scripts;
2651	2687	}
2652	2688
Index: trunk/phase3/includes/resourceloader/ResourceLoaderFileModule.php
—	—	@@ -78,6 +78,8 @@
79	79	protected $messages = array();
80	80	/** String: Name of group to load this module in */
81	81	protected $group;
	82	+ /** String: Position on the page to load this module at */
	83	+ protected $position = 'bottom';
82	84	/** Boolean: Link to raw files in debug mode */
83	85	protected $debugRaw = true;
84	86	/**
—	—	@@ -138,6 +140,8 @@
139	141	* 'messages' => [array of message key strings],
140	142	* // Group which this module should be loaded together with
141	143	* 'group' => [group name string],
	144	+ * // Position on the page to load this module at
	145	+ * 'position' => ['bottom' (default) or 'top']
142	146	* )
143	147	* @endcode
144	148	*/
—	—	@@ -189,6 +193,7 @@
190	194	break;
191	195	// Single strings
192	196	case 'group':
	197	+ case 'position':
193	198	case 'localBasePath':
194	199	case 'remoteBasePath':
195	200	$this->{$member} = (string) $option;
—	—	@@ -295,6 +300,10 @@
296	301	public function getGroup() {
297	302	return $this->group;
298	303	}
	304	+
	305	+ public function getPosition() {
	306	+ return $this->position;
	307	+ }
299	308
300	309	/**
301	310	* Gets list of names of modules this module depends on.
Index: trunk/phase3/includes/resourceloader/ResourceLoaderModule.php
—	—	@@ -159,6 +159,15 @@
160	160	// Stub, override expected
161	161	return null;
162	162	}
	163	+
	164	+ /**
	165	+ * Where on the HTML page should this module's JS be loaded?
	166	+ * 'top': in the <head>
	167	+ * 'bottom': at the bottom of the <body>
	168	+ */
	169	+ public function getPosition() {
	170	+ return 'bottom';
	171	+ }
163	172
164	173	/**
165	174	* Get the loader JS for this module, if set.
Index: trunk/phase3/includes/Skin.php
—	—	@@ -738,7 +738,10 @@
739	739	* @return String HTML-wrapped JS code to be put before </body>
740	740	*/
741	741	function bottomScripts( $out ) {
742		~~- $bottomScriptText = "\n" . $out->getHeadScripts( $this );~~
	742	+ // TODO and the suckage continues. This function is really just a wrapper around
	743	+ // OutputPage::getBottomScripts() which takes a Skin param. This should be cleaned
	744	+ // up at some point
	745	+ $bottomScriptText = $out->getBottomScripts( $this );
743	746	wfRunHooks( 'SkinAfterBottomScripts', array( $this, &$bottomScriptText ) );
744	747
745	748	return $bottomScriptText;
Index: trunk/phase3/resources/Resources.php
—	—	@@ -537,6 +537,7 @@
538	538	'remoteBasePath' => $GLOBALS['wgStylePath'],
539	539	'localBasePath' => "{$GLOBALS['IP']}/skins",
540	540	'dependencies' => 'mediawiki.legacy.wikibits',
	541	+ 'position' => 'top',
541	542	),
542	543	'mediawiki.legacy.edit' => array(
543	544	'scripts' => 'common/edit.js',

Follow-up revisions

Revision	Commit summary	Author	Date
r85617	Fix copypaste fail in r85616	catrope	13:14, 7 April 2011
r91089	1.17wmf1: MFT r85616, r85617. This required meticulous conflict resolution du...	catrope	18:06, 29 June 2011
r108184	ResourceLoader: Add an experimental option to move the main module loading qu...	catrope	23:32, 5 January 2012

Comments

#Comment by Krinkle (talk | contribs) 12:30, 7 April 2011

+	function getBottomScripts( Skin $sk ) {
+		// Only load modules that have marked themselves for loading at the top

"at the top", bottom, right ?

#Comment by Catrope (talk | contribs) 13:15, 7 April 2011

Yeah, copypaste fail. Fixed in r85617.

#Comment by Krinkle (talk | contribs) 07:48, 26 May 2011

Could you look at the TODO you added here ? How urgent / accurate is it towards the current HEAD ?

#Comment by Catrope (talk | contribs) 19:21, 26 May 2011

It's not urgent. Ugly, but low priority.

#Comment by 😂 (talk | contribs) 06:01, 2 July 2011

Removing 1.18 tag, this was before the branch point.

Status & tagging log

06:01, 2 July 2011 😂 (talk | contribs) changed the tags for r85616 [removed: 1.18]
02:39, 30 June 2011 Reedy (talk | contribs) changed the tags for r85616 [removed: 1.17wmf1 added: 1.18]
21:19, 31 May 2011 Catrope (talk | contribs) changed the tags for r85616 [added: 1.17wmf1]
03:21, 29 May 2011 Krinkle (talk | contribs) changed the status of r85616 [removed: new added: ok]