resourceloader: Add ResourceLoaderModule::shouldEmbedModule and use it
authorBrad Jorsch <bjorsch@wikimedia.org>
Fri, 30 Jun 2017 18:19:38 +0000 (14:19 -0400)
committerKrinkle <krinklemail@gmail.com>
Fri, 21 Jul 2017 17:57:34 +0000 (17:57 +0000)
Rather than only the 'private' group triggering embedding, allow modules
to explicitly specify if they should be embedded.

The default is still to only embed when the group is 'private', and the
'private' group is still special in that ResourceLoader::respond() will
still refuse to serve it from load.php.

Change-Id: Ib9a043c566822e278baecc15e87f9c5cebc2eb98

includes/resourceloader/ResourceLoaderClientHtml.php
includes/resourceloader/ResourceLoaderModule.php
tests/phpunit/ResourceLoaderTestCase.php
tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php

index 197ac51..06f9841 100644 (file)
@@ -149,9 +149,7 @@ class ResourceLoaderClientHtml {
                                continue;
                        }
 
-                       $group = $module->getGroup();
-
-                       if ( $group === 'private' ) {
+                       if ( $module->shouldEmbedModule( $this->context ) ) {
                                // Embed via mw.loader.implement per T36907.
                                $data['embed']['general'][] = $name;
                                // Avoid duplicate request from mw.loader
@@ -186,7 +184,7 @@ class ResourceLoaderClientHtml {
                                // Avoid needless request for empty module
                                $data['states'][$name] = 'ready';
                        } else {
-                               if ( $group === 'private' ) {
+                               if ( $module->shouldEmbedModule( $this->context ) ) {
                                        // Embed via style element
                                        $data['embed']['styles'][] = $name;
                                        // Avoid duplicate request from mw.loader
@@ -392,62 +390,75 @@ class ResourceLoaderClientHtml {
                foreach ( $sortedModules as $source => $groups ) {
                        foreach ( $groups as $group => $grpModules ) {
                                $context = self::makeContext( $mainContext, $group, $only, $extraQuery );
-                               $context->setModules( array_keys( $grpModules ) );
-
-                               if ( $group === 'private' ) {
-                                       // Decide whether to use style or script element
-                                       if ( $only == ResourceLoaderModule::TYPE_STYLES ) {
-                                               $chunks[] = Html::inlineStyle(
-                                                       $rl->makeModuleResponse( $context, $grpModules )
-                                               );
-                                       } else {
-                                               $chunks[] = ResourceLoader::makeInlineScript(
-                                                       $rl->makeModuleResponse( $context, $grpModules )
-                                               );
-                                       }
-                                       continue;
-                               }
-
-                               // See if we have one or more raw modules
-                               $isRaw = false;
-                               foreach ( $grpModules as $key => $module ) {
-                                       $isRaw |= $module->isRaw();
-                               }
 
-                               // Special handling for the user group; because users might change their stuff
-                               // on-wiki like user pages, or user preferences; we need to find the highest
-                               // timestamp of these user-changeable modules so we can ensure cache misses on change
-                               // This should NOT be done for the site group (T29564) because anons get that too
-                               // and we shouldn't be putting timestamps in CDN-cached HTML
-                               if ( $group === 'user' ) {
-                                       // Must setModules() before makeVersionQuery()
-                                       $context->setVersion( $rl->makeVersionQuery( $context ) );
+                               // Separate sets of linked and embedded modules while preserving order
+                               $moduleSets = [];
+                               $idx = -1;
+                               foreach ( $grpModules as $name => $module ) {
+                                       $shouldEmbed = $module->shouldEmbedModule( $context );
+                                       if ( !$moduleSets || $moduleSets[$idx][0] !== $shouldEmbed ) {
+                                               $moduleSets[++$idx] = [ $shouldEmbed, [] ];
+                                       }
+                                       $moduleSets[$idx][1][$name] = $module;
                                }
 
-                               $url = $rl->createLoaderURL( $source, $context, $extraQuery );
-
-                               // Decide whether to use 'style' or 'script' element
-                               if ( $only === ResourceLoaderModule::TYPE_STYLES ) {
-                                       $chunk = Html::linkedStyle( $url );
-                               } else {
-                                       if ( $context->getRaw() || $isRaw ) {
-                                               $chunk = Html::element( 'script', [
-                                                       // In SpecialJavaScriptTest, QUnit must load synchronous
-                                                       'async' => !isset( $extraQuery['sync'] ),
-                                                       'src' => $url
-                                               ] );
+                               // Link/embed each set
+                               foreach ( $moduleSets as list( $embed, $moduleSet ) ) {
+                                       $context->setModules( array_keys( $moduleSet ) );
+                                       if ( $embed ) {
+                                               // Decide whether to use style or script element
+                                               if ( $only == ResourceLoaderModule::TYPE_STYLES ) {
+                                                       $chunks[] = Html::inlineStyle(
+                                                               $rl->makeModuleResponse( $context, $moduleSet )
+                                                       );
+                                               } else {
+                                                       $chunks[] = ResourceLoader::makeInlineScript(
+                                                               $rl->makeModuleResponse( $context, $moduleSet )
+                                                       );
+                                               }
                                        } else {
-                                               $chunk = ResourceLoader::makeInlineScript(
-                                                       Xml::encodeJsCall( 'mw.loader.load', [ $url ] )
-                                               );
+                                               // See if we have one or more raw modules
+                                               $isRaw = false;
+                                               foreach ( $moduleSet as $key => $module ) {
+                                                       $isRaw |= $module->isRaw();
+                                               }
+
+                                               // Special handling for the user group; because users might change their stuff
+                                               // on-wiki like user pages, or user preferences; we need to find the highest
+                                               // timestamp of these user-changeable modules so we can ensure cache misses on change
+                                               // This should NOT be done for the site group (T29564) because anons get that too
+                                               // and we shouldn't be putting timestamps in CDN-cached HTML
+                                               if ( $group === 'user' ) {
+                                                       // Must setModules() before makeVersionQuery()
+                                                       $context->setVersion( $rl->makeVersionQuery( $context ) );
+                                               }
+
+                                               $url = $rl->createLoaderURL( $source, $context, $extraQuery );
+
+                                               // Decide whether to use 'style' or 'script' element
+                                               if ( $only === ResourceLoaderModule::TYPE_STYLES ) {
+                                                       $chunk = Html::linkedStyle( $url );
+                                               } else {
+                                                       if ( $context->getRaw() || $isRaw ) {
+                                                               $chunk = Html::element( 'script', [
+                                                                       // In SpecialJavaScriptTest, QUnit must load synchronous
+                                                                       'async' => !isset( $extraQuery['sync'] ),
+                                                                       'src' => $url
+                                                               ] );
+                                                       } else {
+                                                               $chunk = ResourceLoader::makeInlineScript(
+                                                                       Xml::encodeJsCall( 'mw.loader.load', [ $url ] )
+                                                               );
+                                                       }
+                                               }
+
+                                               if ( $group == 'noscript' ) {
+                                                       $chunks[] = Html::rawElement( 'noscript', [], $chunk );
+                                               } else {
+                                                       $chunks[] = $chunk;
+                                               }
                                        }
                                }
-
-                               if ( $group == 'noscript' ) {
-                                       $chunks[] = Html::rawElement( 'noscript', [], $chunk );
-                               } else {
-                                       $chunks[] = $chunk;
-                               }
                        }
                }
 
index 1c767fa..1608901 100644 (file)
@@ -918,6 +918,20 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface {
                return false;
        }
 
+       /**
+        * Check whether this module should be embeded rather than linked
+        *
+        * Modules returning true here will be embedded rather than loaded by
+        * ResourceLoaderClientHtml.
+        *
+        * @since 1.30
+        * @param ResourceLoaderContext $context
+        * @return bool
+        */
+       public function shouldEmbedModule( ResourceLoaderContext $context ) {
+               return $this->getGroup() === 'private';
+       }
+
        /** @var JSParser Lazy-initialized; use self::javaScriptParser() */
        private static $jsParser;
        private static $parseCacheVersion = 1;
index a8a8f4d..d8f89fb 100644 (file)
@@ -94,6 +94,7 @@ class ResourceLoaderTestModule extends ResourceLoaderModule {
        protected $isKnownEmpty = false;
        protected $type = ResourceLoaderModule::LOAD_GENERAL;
        protected $targets = [ 'phpunit' ];
+       protected $shouldEmbed = null;
 
        public function __construct( $options = [] ) {
                foreach ( $options as $key => $value ) {
@@ -143,6 +144,10 @@ class ResourceLoaderTestModule extends ResourceLoaderModule {
                return $this->isKnownEmpty;
        }
 
+       public function shouldEmbedModule( ResourceLoaderContext $context ) {
+               return $this->shouldEmbed !== null ? $this->shouldEmbed : parent::shouldEmbedModule( $context );
+       }
+
        public function enableModuleContentVersion() {
                return true;
        }
index 3e0d883..3530d3c 100644 (file)
@@ -43,6 +43,7 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                        'test.top' => [ 'position' => 'top' ],
                        'test.private.top' => [ 'group' => 'private', 'position' => 'top' ],
                        'test.private.bottom' => [ 'group' => 'private', 'position' => 'bottom' ],
+                       'test.shouldembed' => [ 'shouldEmbed' => true ],
 
                        'test.styles.pure' => [ 'type' => ResourceLoaderModule::LOAD_STYLES ],
                        'test.styles.mixed' => [],
@@ -64,12 +65,24 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                                'group' => 'private',
                                'styles' => '.private{}',
                        ],
+                       'test.styles.shouldembed' => [
+                               'type' => ResourceLoaderModule::LOAD_STYLES,
+                               'shouldEmbed' => true,
+                               'styles' => '.shouldembed{}',
+                       ],
 
                        'test.scripts' => [],
                        'test.scripts.top' => [ 'position' => 'top' ],
                        'test.scripts.user' => [ 'group' => 'user' ],
                        'test.scripts.user.empty' => [ 'group' => 'user', 'isKnownEmpty' => true ],
                        'test.scripts.raw' => [ 'isRaw' => true ],
+                       'test.scripts.shouldembed' => [ 'shouldEmbed' => true ],
+
+                       'test.ordering.a' => [ 'shouldEmbed' => false ],
+                       'test.ordering.b' => [ 'shouldEmbed' => false ],
+                       'test.ordering.c' => [ 'shouldEmbed' => true, 'styles' => '.orderingC{}' ],
+                       'test.ordering.d' => [ 'shouldEmbed' => true, 'styles' => '.orderingD{}' ],
+                       'test.ordering.e' => [ 'shouldEmbed' => false ],
                ];
                return array_map( function ( $options ) {
                        return self::makeModule( $options );
@@ -102,6 +115,7 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                        'test.private.bottom',
                        'test.private.top',
                        'test.top',
+                       'test.shouldembed',
                        'test.unregistered',
                ] );
                $client->setModuleStyles( [
@@ -109,12 +123,14 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                        'test.styles.user.empty',
                        'test.styles.private',
                        'test.styles.pure',
+                       'test.styles.shouldembed',
                        'test.unregistered.styles',
                ] );
                $client->setModuleScripts( [
                        'test.scripts',
                        'test.scripts.user.empty',
                        'test.scripts.top',
+                       'test.scripts.shouldembed',
                        'test.unregistered.scripts',
                ] );
 
@@ -122,12 +138,15 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                        'states' => [
                                'test.private.top' => 'loading',
                                'test.private.bottom' => 'loading',
+                               'test.shouldembed' => 'loading',
                                'test.styles.pure' => 'ready',
                                'test.styles.user.empty' => 'ready',
                                'test.styles.private' => 'ready',
+                               'test.styles.shouldembed' => 'ready',
                                'test.scripts' => 'loading',
                                'test.scripts.top' => 'loading',
                                'test.scripts.user.empty' => 'ready',
+                               'test.scripts.shouldembed' => 'loading',
                        ],
                        'general' => [
                                'test',
@@ -139,12 +158,14 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                        'scripts' => [
                                'test.scripts',
                                'test.scripts.top',
+                               'test.scripts.shouldembed',
                        ],
                        'embed' => [
-                               'styles' => [ 'test.styles.private' ],
+                               'styles' => [ 'test.styles.private', 'test.styles.shouldembed' ],
                                'general' => [
                                        'test.private.bottom',
                                        'test.private.top',
+                                       'test.shouldembed',
                                ],
                        ],
                ];
@@ -276,6 +297,47 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                                'only' => ResourceLoaderModule::TYPE_STYLES,
                                'output' => '<noscript><link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=nl&amp;modules=test.styles.noscript&amp;only=styles&amp;skin=fallback"/></noscript>',
                        ],
+                       [
+                               'context' => [],
+                               'modules' => [ 'test.shouldembed' ],
+                               'only' => ResourceLoaderModule::TYPE_COMBINED,
+                               'output' => '<script>(window.RLQ=window.RLQ||[]).push(function(){mw.loader.implement("test.shouldembed@09p30q0",function($,jQuery,require,module){},{"css":[]});});</script>',
+                       ],
+                       [
+                               'context' => [],
+                               'modules' => [ 'test.styles.shouldembed' ],
+                               'only' => ResourceLoaderModule::TYPE_STYLES,
+                               'output' => '<style>.shouldembed{}</style>',
+                       ],
+                       [
+                               'context' => [],
+                               'modules' => [ 'test.scripts.shouldembed' ],
+                               'only' => ResourceLoaderModule::TYPE_SCRIPTS,
+                               'output' => '<script>(window.RLQ=window.RLQ||[]).push(function(){mw.loader.state({"test.scripts.shouldembed":"ready"});});</script>',
+                       ],
+                       [
+                               'context' => [],
+                               'modules' => [ 'test', 'test.shouldembed' ],
+                               'only' => ResourceLoaderModule::TYPE_COMBINED,
+                               'output' => '<script>(window.RLQ=window.RLQ||[]).push(function(){mw.loader.load("/w/load.php?debug=false\u0026lang=nl\u0026modules=test\u0026skin=fallback");mw.loader.implement("test.shouldembed@09p30q0",function($,jQuery,require,module){},{"css":[]});});</script>',
+                       ],
+                       [
+                               'context' => [],
+                               'modules' => [ 'test.styles.pure', 'test.styles.shouldembed' ],
+                               'only' => ResourceLoaderModule::TYPE_STYLES,
+                               'output' =>
+                                       '<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=nl&amp;modules=test.styles.pure&amp;only=styles&amp;skin=fallback"/>' . "\n"
+                                       . '<style>.shouldembed{}</style>'
+                       ],
+                       [
+                               'context' => [],
+                               'modules' => [ 'test.ordering.a', 'test.ordering.e', 'test.ordering.b', 'test.ordering.d', 'test.ordering.c' ],
+                               'only' => ResourceLoaderModule::TYPE_STYLES,
+                               'output' =>
+                                       '<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=nl&amp;modules=test.ordering.a%2Cb&amp;only=styles&amp;skin=fallback"/>' . "\n"
+                                       . '<style>.orderingC{}.orderingD{}</style>' . "\n"
+                                       . '<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=nl&amp;modules=test.ordering.e&amp;only=styles&amp;skin=fallback"/>'
+                       ],
                        // @codingStandardsIgnoreEnd
                ];
        }