From a01d8be82c6eb6a39b9715b40b71f78817161a2a Mon Sep 17 00:00:00 2001
From: jdlrobson <jdlrobson@gmail.com>
Date: Mon, 9 Apr 2018 18:22:13 -0700
Subject: [PATCH] Skins: getDefaultStyles can now define render blocking CSS

This optimisation attempts to minimise loading the styles in places
they are not needed.

The logic is kept inside Skin::getDefaultModules to avoid fragmentation
of where modules get defined.

Update ApiParse to avoid repetition of code.

Bug: T42792
Bug: T42812
Change-Id: I59f02a7bab3baa9d43f6bc2ef1f549d9d31d8456
---
 includes/OutputPage.php                   | 21 ++++++++++++++++++---
 includes/api/ApiParse.php                 |  4 +---
 includes/skins/Skin.php                   |  7 ++++++-
 tests/phpunit/includes/skins/SkinTest.php | 16 ++++++++++++++++
 4 files changed, 41 insertions(+), 7 deletions(-)
 create mode 100644 tests/phpunit/includes/skins/SkinTest.php

diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index 56df0f06eb..0b6e616b33 100644
--- a/includes/OutputPage.php
+++ b/includes/OutputPage.php
@@ -2331,6 +2331,23 @@ class OutputPage extends ContextSource {
 		}
 	}
 
+	/**
+	 * Transfer styles and JavaScript modules from skin.
+	 *
+	 * @param Skin $sk to load modules for
+	 */
+	public function loadSkinModules( $sk ) {
+		foreach ( $sk->getDefaultModules() as $group => $modules ) {
+			if ( $group === 'styles' ) {
+				foreach ( $modules as $key => $moduleMembers ) {
+					$this->addModuleStyles( $moduleMembers );
+				}
+			} else {
+				$this->addModules( $modules );
+			}
+		}
+	}
+
 	/**
 	 * Finally, all the text has been munged and accumulated into
 	 * the object, let's actually output it:
@@ -2424,9 +2441,7 @@ class OutputPage extends ContextSource {
 			}
 
 			$sk = $this->getSkin();
-			foreach ( $sk->getDefaultModules() as $group ) {
-				$this->addModules( $group );
-			}
+			$this->loadSkinModules( $sk );
 
 			MWDebug::addModules( $this );
 
diff --git a/includes/api/ApiParse.php b/includes/api/ApiParse.php
index 05b4289b80..096122d186 100644
--- a/includes/api/ApiParse.php
+++ b/includes/api/ApiParse.php
@@ -323,9 +323,7 @@ class ApiParse extends ApiBase {
 				// Based on OutputPage::headElement()
 				$skin->setupSkinUserCss( $outputPage );
 				// Based on OutputPage::output()
-				foreach ( $skin->getDefaultModules() as $group ) {
-					$outputPage->addModules( $group );
-				}
+				$outputPage->loadSkinModules( $skin );
 			}
 
 			Hooks::run( 'ApiParseMakeOutputPage', [ $this, $outputPage ] );
diff --git a/includes/skins/Skin.php b/includes/skins/Skin.php
index f3276e87ec..9c4ac50729 100644
--- a/includes/skins/Skin.php
+++ b/includes/skins/Skin.php
@@ -166,7 +166,8 @@ abstract class Skin extends ContextSource {
 	 * It is recommended that skins wishing to override call parent::getDefaultModules()
 	 * and substitute out any modules they wish to change by using a key to look them up
 	 *
-	 * For style modules, use setupSkinUserCss() instead.
+	 * Any modules defined with the 'styles' key will be added as render blocking CSS via
+	 * Output::addModuleStyles. Similarly, each key should refer to a list of modules
 	 *
 	 * @return array Array of modules with helper keys for easy overriding
 	 */
@@ -175,6 +176,10 @@ abstract class Skin extends ContextSource {
 		$config = $this->getConfig();
 		$user = $out->getUser();
 		$modules = [
+			// Styles key sets render blocking styles
+			// Unlike other keys in this definition it is an associative array
+			// where each key is the group name and points to a list of modules
+			'styles' => [],
 			// modules not specific to any specific skin or page
 			'core' => [
 				// Enforce various default modules for all pages and all skins
diff --git a/tests/phpunit/includes/skins/SkinTest.php b/tests/phpunit/includes/skins/SkinTest.php
new file mode 100644
index 0000000000..41ef2b796a
--- /dev/null
+++ b/tests/phpunit/includes/skins/SkinTest.php
@@ -0,0 +1,16 @@
+<?php
+class SkinTest extends MediaWikiTestCase {
+
+	/**
+	 * @covers Skin::getDefaultModules
+	 */
+	public function testGetDefaultModules() {
+		$skin = $this->getMockBuilder( Skin::class )
+			->setMethods( [ 'outputPage', 'setupSkinUserCss' ] )
+			->getMock();
+
+		$modules = $skin->getDefaultModules();
+		$this->assertTrue( isset( $modules['core'] ), 'core key is set by default' );
+		$this->assertTrue( isset( $modules['styles'] ), 'style key is set by default' );
+	}
+}
-- 
2.20.1