From 2b6eb60ce53cd71af8b6ef1d860ef6d3b3a684a2 Mon Sep 17 00:00:00 2001
From: "Ori.livneh" <ori@wikimedia.org>
Date: Sat, 21 Mar 2015 05:14:02 +0000
Subject: [PATCH] Revert "Optimize order of styles and scripts"

The patch did not improve performance. I'd like to think that the increased
control over when inline scripts are executed makes the patch worthwhile
regardless, but that is post hoc justification and possibly a bit of personal
ego. Krinkle agrees that we may use some of the ideas in this patch in the
future but he thinks we're better off not heading down this path before we
have a better sense of where we're going, and I trust his judgment.

This reverts commit e86e5f8460b922cadac231142a495f3259c67b43.

Change-Id: I151f74a41dd664b5a0aa5cfd99fcc95e2686a1e6
---
 includes/EditPage.php                         |  2 +-
 includes/OutputPage.php                       | 88 ++++++++++---------
 includes/debug/MWDebug.php                    |  6 +-
 includes/resourceloader/ResourceLoader.php    | 15 ----
 .../ResourceLoaderStartUpModule.php           | 11 +--
 includes/skins/Skin.php                       |  4 +-
 includes/specials/SpecialJavaScriptTest.php   | 16 ++--
 tests/phpunit/includes/OutputPageTest.php     | 52 +++++------
 8 files changed, 84 insertions(+), 110 deletions(-)

diff --git a/includes/EditPage.php b/includes/EditPage.php
index fe96f9a1cf..6bdb85c3b1 100644
--- a/includes/EditPage.php
+++ b/includes/EditPage.php
@@ -3708,7 +3708,7 @@ HTML
 		}
 
 		$script .= '});';
-		$wgOut->addScript( ResourceLoader::makeInlineScript( $script ) );
+		$wgOut->addScript( Html::inlineScript( ResourceLoader::makeLoaderConditionalScript( $script ) ) );
 
 		$toolbar = '<div id="toolbar"></div>';
 
diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index 5ad33fad0f..edeae0d139 100644
--- a/includes/OutputPage.php
+++ b/includes/OutputPage.php
@@ -2674,12 +2674,10 @@ class OutputPage extends ContextSource {
 			$ret .= $item . "\n";
 		}
 
-		$ret .= $this->getInlineHeadScript();
-
 		// No newline after buildCssLinks since makeResourceLoaderLink did that already
 		$ret .= $this->buildCssLinks();
 
-		$ret .= $this->getHeadScripts();
+		$ret .= $this->getHeadScripts() . "\n";
 
 		foreach ( $this->mHeadItems as $item ) {
 			$ret .= $item . "\n";
@@ -2856,8 +2854,10 @@ class OutputPage extends ContextSource {
 							$resourceLoader->makeModuleResponse( $context, $grpModules )
 						);
 					} else {
-						$links['html'] .= ResourceLoader::makeInlineScript(
-							$resourceLoader->makeModuleResponse( $context, $grpModules )
+						$links['html'] .= Html::inlineScript(
+							ResourceLoader::makeLoaderConditionalScript(
+								$resourceLoader->makeModuleResponse( $context, $grpModules )
+							)
 						);
 					}
 					$links['html'] .= "\n";
@@ -2896,8 +2896,10 @@ class OutputPage extends ContextSource {
 					if ( $only === ResourceLoaderModule::TYPE_STYLES ) {
 						$link = Html::linkedStyle( $url );
 					} elseif ( $loadCall ) {
-						$link = ResourceLoader::makeInlineScript(
-							Xml::encodeJsCall( 'mw.loader.load', array( $url, 'text/javascript', true ) )
+						$link = Html::inlineScript(
+							ResourceLoader::makeLoaderConditionalScript(
+								Xml::encodeJsCall( 'mw.loader.load', array( $url, 'text/javascript', true ) )
+							)
 						);
 					} else {
 						$link = Html::linkedScript( $url );
@@ -2906,8 +2908,10 @@ class OutputPage extends ContextSource {
 							// browsers not supported by the startup module would unconditionally
 							// execute this module. Otherwise users will get "ReferenceError: mw is
 							// undefined" or "jQuery is undefined" from e.g. a "site" module.
-							$link = ResourceLoader::makeInlineScript(
-								Xml::encodeJsCall( 'document.write', array( $link ) )
+							$link = Html::inlineScript(
+								ResourceLoader::makeLoaderConditionalScript(
+									Xml::encodeJsCall( 'document.write', array( $link ) )
+								)
 							);
 						}
 
@@ -2951,44 +2955,16 @@ class OutputPage extends ContextSource {
 		}
 
 		if ( count( $states ) ) {
-			$html = ResourceLoader::makeInlineScript(
-				ResourceLoader::makeLoaderStateScript( $states )
+			$html = Html::inlineScript(
+				ResourceLoader::makeLoaderConditionalScript(
+					ResourceLoader::makeLoaderStateScript( $states )
+				)
 			) . "\n" . $html;
 		}
 
 		return $html;
 	}
 
-	/**
-	 * Get <script> tags for <head> whose source is inline.
-	 *
-	 * @since 1.25
-	 * @return string HTML fragment
-	 */
-	public function getInlineHeadScript() {
-		// Load config before anything else.
-		$html = ResourceLoader::makeInlineScript(
-			ResourceLoader::makeConfigSetScript( $this->getJSVars() )
-		);
-
-		// Load embeddable private modules before any loader links.
-		$inlineModulesLink = $this->makeResourceLoaderLink(
-			array( 'user.options', 'user.tokens' ), ResourceLoaderModule::TYPE_COMBINED
-		);
-		$html .= "\n" . self::getHtmlFromLoaderLinks( array( $inlineModulesLink ) );
-
-		// Construct mw.loader.load() call for top-loaded modules.
-		// Client-side code will request these modules and their dependencies.
-		$topModules = $this->getModules( true, 'top' );
-		if ( $topModules ) {
-			$html .= ResourceLoader::makeInlineScript(
-				Xml::encodeJsCall( 'mw.loader.load', array( $topModules ) )
-			) . "\n";
-		}
-
-		return $html;
-	}
-
 	/**
 	 * JS stuff to put in the "<head>". This is the startup module, config
 	 * vars and modules marked with position 'top'
@@ -3000,6 +2976,19 @@ class OutputPage extends ContextSource {
 		$links = array();
 		$links[] = $this->makeResourceLoaderLink( 'startup', ResourceLoaderModule::TYPE_SCRIPTS, true );
 
+		// Load config before anything else
+		$links[] = Html::inlineScript(
+			ResourceLoader::makeLoaderConditionalScript(
+				ResourceLoader::makeConfigSetScript( $this->getJSVars() )
+			)
+		);
+
+		// Load embeddable private modules before any loader links
+		// This needs to be TYPE_COMBINED so these modules are properly wrapped
+		// in mw.loader.implement() calls and deferred until mw.user is available
+		$embedScripts = array( 'user.options', 'user.tokens' );
+		$links[] = $this->makeResourceLoaderLink( $embedScripts, ResourceLoaderModule::TYPE_COMBINED );
+
 		// Scripts and messages "only" requests marked for top inclusion
 		// Messages should go first
 		$links[] = $this->makeResourceLoaderLink(
@@ -3011,6 +3000,17 @@ class OutputPage extends ContextSource {
 			ResourceLoaderModule::TYPE_SCRIPTS
 		);
 
+		// Modules requests - let the client calculate dependencies and batch requests as it likes
+		// Only load modules that have marked themselves for loading at the top
+		$modules = $this->getModules( true, 'top' );
+		if ( $modules ) {
+			$links[] = Html::inlineScript(
+				ResourceLoader::makeLoaderConditionalScript(
+					Xml::encodeJsCall( 'mw.loader.load', array( $modules ) )
+				)
+			);
+		}
+
 		if ( $this->getConfig()->get( 'ResourceLoaderExperimentalAsyncLoading' ) ) {
 			$links[] = $this->getScriptsForBottomQueue( true );
 		}
@@ -3047,8 +3047,10 @@ class OutputPage extends ContextSource {
 		// Only load modules that have marked themselves for loading at the bottom
 		$modules = $this->getModules( true, 'bottom' );
 		if ( $modules ) {
-			$links[] = ResourceLoader::makeInlineScript(
-				Xml::encodeJsCall( 'mw.loader.load', array( $modules, null, true ) )
+			$links[] = Html::inlineScript(
+				ResourceLoader::makeLoaderConditionalScript(
+					Xml::encodeJsCall( 'mw.loader.load', array( $modules, null, true ) )
+				)
 			);
 		}
 
diff --git a/includes/debug/MWDebug.php b/includes/debug/MWDebug.php
index dc80a46b26..7a4a3b6bef 100644
--- a/includes/debug/MWDebug.php
+++ b/includes/debug/MWDebug.php
@@ -432,8 +432,10 @@ class MWDebug {
 
 			// Cannot use OutputPage::addJsConfigVars because those are already outputted
 			// by the time this method is called.
-			$html = ResourceLoader::makeInlineScript(
-				ResourceLoader::makeConfigSetScript( array( 'debugInfo' => $debugInfo ) )
+			$html = Html::inlineScript(
+				ResourceLoader::makeLoaderConditionalScript(
+					ResourceLoader::makeConfigSetScript( array( 'debugInfo' => $debugInfo ) )
+				)
 			);
 		}
 
diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index 803afbfaa1..5eab3cb66b 100644
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -1326,7 +1326,6 @@ MESSAGE;
 	 * Returns JS code which runs given JS code if the client-side framework is
 	 * present.
 	 *
-	 * @deprecated since 1.25; use makeInlineScript instead
 	 * @param string $script JavaScript code
 	 * @return string
 	 */
@@ -1334,20 +1333,6 @@ MESSAGE;
 		return "if(window.mw){\n" . trim( $script ) . "\n}";
 	}
 
-	/**
-	 * Construct an inline script tag with given JS code.
-	 *
-	 * The code will be wrapped in a closure, and it will be executed by ResourceLoader
-	 * only if the client has adequate support for MediaWiki JavaScript code.
-	 *
-	 * @param string $script JavaScript code
-	 * @return string HTML
-	 */
-	public static function makeInlineScript( $script ) {
-		$js = 'var _mwq = _mwq || []; _mwq.push( function ( mw ) { ' . $script . ' } );';
-		return Html::inlineScript( $js );
-	}
-
 	/**
 	 * Returns JS code which will set the MediaWiki configuration array to
 	 * the given value.
diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php
index 6ee098caf8..48b357653d 100644
--- a/includes/resourceloader/ResourceLoaderStartUpModule.php
+++ b/includes/resourceloader/ResourceLoaderStartUpModule.php
@@ -357,20 +357,11 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
 				ResourceLoader::inDebugMode()
 			);
 
-			// Process the deferred inline script queue, and ensure that any
-			// functions enqueued after this point are executed immediately.
-			$mwqJs = (
-				'window._mwq = window._mwq || [];' .
-				'while ( _mwq.length ) _mwq.shift()( mw );' .
-				'_mwq.push = function ( f ) { f( mw ); };'
-			);
-
 			$out .= "var startUp = function () {\n" .
 				"\tmw.config = new " .
 				$mwMapJsCall . "\n" .
 				"\t$registrations\n" .
-				"\t" . $mwConfigSetJsCall . "\n" .
-				"\t" . $mwqJs . "\n" .
+				"\t" . $mwConfigSetJsCall .
 				"};\n";
 
 			// Conditional script injection
diff --git a/includes/skins/Skin.php b/includes/skins/Skin.php
index d649c2e71e..48bce6796e 100644
--- a/includes/skins/Skin.php
+++ b/includes/skins/Skin.php
@@ -360,8 +360,8 @@ abstract class Skin extends ContextSource {
 	 */
 	static function makeVariablesScript( $data ) {
 		if ( $data ) {
-			return ResourceLoader::makeInlineScript(
-				ResourceLoader::makeConfigSetScript( $data )
+			return Html::inlineScript(
+				ResourceLoader::makeLoaderConditionalScript( ResourceLoader::makeConfigSetScript( $data ) )
 			);
 		} else {
 			return '';
diff --git a/includes/specials/SpecialJavaScriptTest.php b/includes/specials/SpecialJavaScriptTest.php
index e9639e1944..ecb166a435 100644
--- a/includes/specials/SpecialJavaScriptTest.php
+++ b/includes/specials/SpecialJavaScriptTest.php
@@ -168,13 +168,15 @@ HTML;
 		// The testrunner configures QUnit and essentially depends on it. However, test suites
 		// are reusable in environments that preload QUnit (or a compatibility interface to
 		// another framework). Therefore we have to load it ourselves.
-		$out->addHtml( ResourceLoader::makeInlineScript(
-			Xml::encodeJsCall( 'mw.loader.using', array(
-				array( 'jquery.qunit', 'jquery.qunit.completenessTest' ),
-				new XmlJsCode(
-					'function () {' . Xml::encodeJsCall( 'mw.loader.load', array( $modules ) ) . '}'
-				)
-			) )
+		$out->addHtml( Html::inlineScript(
+			ResourceLoader::makeLoaderConditionalScript(
+				Xml::encodeJsCall( 'mw.loader.using', array(
+					array( 'jquery.qunit', 'jquery.qunit.completenessTest' ),
+					new XmlJsCode(
+						'function () {' . Xml::encodeJsCall( 'mw.loader.load', array( $modules ) ) . '}'
+					)
+				) )
+			)
 		) );
 	}
 
diff --git a/tests/phpunit/includes/OutputPageTest.php b/tests/phpunit/includes/OutputPageTest.php
index 9a3bf17366..2526fccd83 100644
--- a/tests/phpunit/includes/OutputPageTest.php
+++ b/tests/phpunit/includes/OutputPageTest.php
@@ -141,56 +141,52 @@ class OutputPageTest extends MediaWikiTestCase {
 			// Load module script only
 			array(
 				array( 'test.foo', ResourceLoaderModule::TYPE_SCRIPTS ),
-				'<script>var _mwq = _mwq || []; _mwq.push( function ( mw ) {' .
-				' document.write("\u003Cscript src=\"http://127.0.0.1:8080/w/load.php?debug=' .
-				'false\u0026amp;lang=en\u0026amp;modules=test.foo\u0026amp;only=scripts\u0026' .
-				'amp;skin=fallback\u0026amp;*\"\u003E\u003C/script\u003E"); } );</script>
+				'<script>if(window.mw){
+document.write("\u003Cscript src=\"http://127.0.0.1:8080/w/load.php?debug=false\u0026amp;lang=en\u0026amp;modules=test.foo\u0026amp;only=scripts\u0026amp;skin=fallback\u0026amp;*\"\u003E\u003C/script\u003E");
+}</script>
 '
 			),
 			array(
 				// Don't condition wrap raw modules (like the startup module)
 				array( 'test.raw', ResourceLoaderModule::TYPE_SCRIPTS ),
-				'<script src="http://127.0.0.1:8080/w/load.php?debug=false&amp;lang=en&amp;' .
-				'modules=test.raw&amp;only=scripts&amp;skin=fallback&amp;*"></script>
+				'<script src="http://127.0.0.1:8080/w/load.php?debug=false&amp;lang=en&amp;modules=test.raw&amp;only=scripts&amp;skin=fallback&amp;*"></script>
 '
 			),
 			// Load module styles only
 			// This also tests the order the modules are put into the url
 			array(
 				array( array( 'test.baz', 'test.foo', 'test.bar' ), ResourceLoaderModule::TYPE_STYLES ),
-				'<link rel=stylesheet href="http://127.0.0.1:8080/w/load.php?debug=false&amp;' .
-				'lang=en&amp;modules=test.bar%2Cbaz%2Cfoo&amp;only=styles&amp;skin=fallback&amp;*">
+				'<link rel=stylesheet href="http://127.0.0.1:8080/w/load.php?debug=false&amp;lang=en&amp;modules=test.bar%2Cbaz%2Cfoo&amp;only=styles&amp;skin=fallback&amp;*">
 '
 			),
 			// Load private module (only=scripts)
 			array(
 				array( 'test.quux', ResourceLoaderModule::TYPE_SCRIPTS ),
-				'<script>var _mwq = _mwq || []; _mwq.push( function ( mw ) {' .
-				' mw.test.baz({token:123});mw.loader.state({"test.quux":"ready"});
-' . ' } );</script>
+				'<script>if(window.mw){
+mw.test.baz({token:123});mw.loader.state({"test.quux":"ready"});
+
+}</script>
 '
 			),
 			// Load private module (combined)
 			array(
 				array( 'test.quux', ResourceLoaderModule::TYPE_COMBINED ),
-				'<script>var _mwq = _mwq || []; _mwq.push( function ( mw ) {' .
-				' mw.loader.implement("test.quux",function($,jQuery){mw.test.baz({token:123});}' .
-				',{"css":[".mw-icon{transition:none}\n"]},{},{});
-' . ' } );</script>
+				'<script>if(window.mw){
+mw.loader.implement("test.quux",function($,jQuery){mw.test.baz({token:123});},{"css":[".mw-icon{transition:none}\n"]},{},{});
+
+}</script>
 '
 			),
 			// Load module script with ESI
 			array(
 				array( 'test.foo', ResourceLoaderModule::TYPE_SCRIPTS, true ),
-				'<script><esi:include src="http://127.0.0.1:8080/w/load.php?debug=false&amp;' .
-				'lang=en&amp;modules=test.foo&amp;only=scripts&amp;skin=fallback&amp;*" /></script>
+				'<script><esi:include src="http://127.0.0.1:8080/w/load.php?debug=false&amp;lang=en&amp;modules=test.foo&amp;only=scripts&amp;skin=fallback&amp;*" /></script>
 '
 			),
 			// Load module styles with ESI
 			array(
 				array( 'test.foo', ResourceLoaderModule::TYPE_STYLES, true ),
-				'<style><esi:include src="http://127.0.0.1:8080/w/load.php?debug=false&amp;' .
-				'lang=en&amp;modules=test.foo&amp;only=styles&amp;skin=fallback&amp;*" /></style>
+				'<style><esi:include src="http://127.0.0.1:8080/w/load.php?debug=false&amp;lang=en&amp;modules=test.foo&amp;only=styles&amp;skin=fallback&amp;*" /></style>
 ',
 			),
 			// Load no modules
@@ -201,22 +197,18 @@ class OutputPageTest extends MediaWikiTestCase {
 			// noscript group
 			array(
 				array( 'test.noscript', ResourceLoaderModule::TYPE_STYLES ),
-				'<noscript><link rel=stylesheet href="http://127.0.0.1:8080/w/load.php?debug=' .
-				'false&amp;lang=en&amp;modules=test.noscript&amp;only=styles&amp;skin=fallback' .
-				'&amp;*"></noscript>
+				'<noscript><link rel=stylesheet href="http://127.0.0.1:8080/w/load.php?debug=false&amp;lang=en&amp;modules=test.noscript&amp;only=styles&amp;skin=fallback&amp;*"></noscript>
 '
 			),
 			// Load two modules in separate groups
 			array(
 				array( array( 'test.group.foo', 'test.group.bar' ), ResourceLoaderModule::TYPE_COMBINED ),
-				'<script>var _mwq = _mwq || []; _mwq.push( function ( mw ) { ' .
-				'document.write("\u003Cscript src=\"http://127.0.0.1:8080/w/load.php?debug=false' .
-				'\u0026amp;lang=en\u0026amp;modules=test.group.bar\u0026amp;skin=fallback\u0026' .
-				'amp;*\"\u003E\u003C/script\u003E"); } );</script>
-<script>var _mwq = _mwq || []; _mwq.push( function ( mw ) { document.write(' .
-				'"\u003Cscript src=\"http://127.0.0.1:8080/w/load.php?debug=false\u0026amp;' .
-				'lang=en\u0026amp;modules=test.group.foo\u0026amp;skin=fallback\u0026amp;*\\' .
-				'"\u003E\u003C/script\u003E"); } );</script>
+				'<script>if(window.mw){
+document.write("\u003Cscript src=\"http://127.0.0.1:8080/w/load.php?debug=false\u0026amp;lang=en\u0026amp;modules=test.group.bar\u0026amp;skin=fallback\u0026amp;*\"\u003E\u003C/script\u003E");
+}</script>
+<script>if(window.mw){
+document.write("\u003Cscript src=\"http://127.0.0.1:8080/w/load.php?debug=false\u0026amp;lang=en\u0026amp;modules=test.group.foo\u0026amp;skin=fallback\u0026amp;*\"\u003E\u003C/script\u003E");
+}</script>
 '
 			),
 		);
-- 
2.20.1