From 9435cd81b07f94a8283d23aff8835c9c734f05db Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 27 Oct 2015 03:39:34 +0000 Subject: [PATCH] resoureloader: Consolidate styles-only queue at the top This effectively reverts d6b4d3c537 and declines T97420. This was previously attempted in b7c0e537eb. Drop support for position "bottom" for addModuleStyles(). This feature was only recently introduced with the intent to optimise page load performance, but had an adverse effect. It increases chances of FOUC due to late discovery of these styles. It also caused minor problems for some gadgets and extensions that did not or were unable to set these flags. Some mobile code was introduced around the same time as this feature and was never given position=top. Stylesheets that don't affect initial render or are only needed on interaction should be loaded via addModules() instead; which is handled by the asynchronous loader in JavaScript. Change-Id: Ib9821b7b87cfc8a59062bb6ca358974fdb01ced1 --- includes/OutputPage.php | 23 +------------------ .../ResourceLoaderFileModule.php | 1 - .../ResourceLoaderImageModule.php | 6 ----- .../resourceloader/ResourceLoaderModule.php | 17 -------------- .../ResourceLoaderWikiModule.php | 2 -- 5 files changed, 1 insertion(+), 48 deletions(-) diff --git a/includes/OutputPage.php b/includes/OutputPage.php index d29ec54d4b..147527e54c 100644 --- a/includes/OutputPage.php +++ b/includes/OutputPage.php @@ -610,20 +610,6 @@ class OutputPage extends ContextSource { * @return array Array of module names */ public function getModuleStyles( $filter = false, $position = null ) { - // T97420 - $resourceLoader = $this->getResourceLoader(); - - foreach ( $this->mModuleStyles as $val ) { - $module = $resourceLoader->getModule( $val ); - - if ( $module instanceof ResourceLoaderModule && $module->isPositionDefault() ) { - $warning = __METHOD__ . ': style module should define its position explicitly: ' . - $val . ' ' . get_class( $module ); - wfDebugLog( 'resourceloader', $warning ); - wfLogWarning( $warning ); - } - } - return $this->getModules( $filter, $position, 'mModuleStyles' ); } @@ -3074,10 +3060,6 @@ class OutputPage extends ContextSource { ResourceLoaderModule::TYPE_SCRIPTS ); - $links[] = $this->makeResourceLoaderLink( $this->getModuleStyles( true, 'bottom' ), - ResourceLoaderModule::TYPE_STYLES - ); - // Modules requests - let the client calculate dependencies and batch requests as it likes // Only load modules that have marked themselves for loading at the bottom $modules = $this->getModules( true, 'bottom' ); @@ -3143,9 +3125,6 @@ class OutputPage extends ContextSource { * @return string */ function getBottomScripts() { - // In case the skin wants to add bottom CSS - $this->getSkin()->setupSkinUserCss( $this ); - return $this->getScriptsForBottomQueue(); } @@ -3684,7 +3663,7 @@ class OutputPage extends ContextSource { $otherTags = array(); // Tags to append after the normal tags $resourceLoader = $this->getResourceLoader(); - $moduleStyles = $this->getModuleStyles( true, 'top' ); + $moduleStyles = $this->getModuleStyles(); // Per-site custom styles $moduleStyles[] = 'site'; diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php index 1b827dbae1..e4def4db4a 100644 --- a/includes/resourceloader/ResourceLoaderFileModule.php +++ b/includes/resourceloader/ResourceLoaderFileModule.php @@ -279,7 +279,6 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { break; // Single strings case 'position': - $this->isPositionDefined = true; case 'group': case 'skipFunction': $this->{$member} = (string)$option; diff --git a/includes/resourceloader/ResourceLoaderImageModule.php b/includes/resourceloader/ResourceLoaderImageModule.php index 8de87f2ef5..e2da28bb53 100644 --- a/includes/resourceloader/ResourceLoaderImageModule.php +++ b/includes/resourceloader/ResourceLoaderImageModule.php @@ -184,7 +184,6 @@ class ResourceLoaderImageModule extends ResourceLoaderModule { break; case 'position': - $this->isPositionDefined = true; case 'prefix': case 'selectorWithoutVariant': case 'selectorWithVariant': @@ -456,9 +455,4 @@ class ResourceLoaderImageModule extends ResourceLoaderModule { $this->loadFromDefinition(); return $this->position; } - - public function isPositionDefault() { - $this->loadFromDefinition(); - return parent::isPositionDefault(); - } } diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 714a8ffc3e..3dd7a4b0e2 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -67,10 +67,6 @@ abstract class ResourceLoaderModule { // In-object cache for module content protected $contents = array(); - // Whether the position returned by getPosition() is defined in the module configuration - // and not a default value - protected $isPositionDefined = false; - /** * @var Config */ @@ -291,19 +287,6 @@ abstract class ResourceLoaderModule { return 'bottom'; } - /** - * Whether the position returned by getPosition() is a default value or comes from the module - * definition. This method is meant to be short-lived, and is only useful until classes added - * via addModuleStyles with a default value define an explicit position. See getModuleStyles() - * in OutputPage for the related migration warning. - * - * @return bool - * @since 1.26 - */ - public function isPositionDefault() { - return !$this->isPositionDefined; - } - /** * Whether this module's JS expects to work without the client-side ResourceLoader module. * Returning true from this function will prevent mw.loader.state() call from being diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php index 156ff4e455..57555f0dce 100644 --- a/includes/resourceloader/ResourceLoaderWikiModule.php +++ b/includes/resourceloader/ResourceLoaderWikiModule.php @@ -72,8 +72,6 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule { foreach ( $options as $member => $option ) { switch ( $member ) { case 'position': - $this->isPositionDefined = true; - // Don't break since we need the member set as well case 'styles': case 'scripts': case 'group': -- 2.20.1