resourceloader: Move logo preload from OutputPage to SkinModule
authorTimo Tijhof <krinklemail@gmail.com>
Mon, 20 Aug 2018 23:42:42 +0000 (00:42 +0100)
committerKrinkle <krinklemail@gmail.com>
Mon, 27 Aug 2018 23:05:51 +0000 (23:05 +0000)
This was introduced in OutputPage before support for getPreloadLinks()
was added to ResourceLoader. The introduction in ResourceLoader was
actually inspired by this original implementation.

Now that we have it, we should make use of it for this module
as well. Doing so has several benefits:

* Makes the code cleaner by not requiring every skin to implement
  the extra boolean method. Instead, it naturally works. If
  the skin loads the SkinModule, it gets the preload as well.
  If not (such as Minerva, which has a different logo config),
  then it also doesn't get the preload link.
  Naturally, automatic.

* Makes code cleaner by not having static methods, and by not
  having OutputPage call into a Module class.

* Fixes the problem where, if a site's logo is changed, all cached
  HTML is preloading the old logo whilst the stylesheet fetches
  the newer one. Causing both to be downloaded.

* Still preloads the logo well before it can render.

Change-Id: I11b390f2e4f5e7db8b4506ab547839152888005c

includes/OutputPage.php
includes/resourceloader/ResourceLoaderSkinModule.php
includes/skins/Skin.php
tests/phpunit/includes/OutputPageTest.php

index c8e18e0..3675e8a 100644 (file)
@@ -2364,10 +2364,6 @@ class OutputPage extends ContextSource {
 
                if ( !$this->mArticleBodyOnly ) {
                        $sk = $this->getSkin();
-
-                       if ( $sk->shouldPreloadLogo() ) {
-                               $this->addLogoPreloadLinkHeaders();
-                       }
                }
 
                $linkHeader = $this->getLinkHeader();
@@ -3915,80 +3911,6 @@ class OutputPage extends ContextSource {
                ] );
        }
 
-       /**
-        * Add Link headers for preloading the wiki's logo.
-        *
-        * @since 1.26
-        */
-       protected function addLogoPreloadLinkHeaders() {
-               $logo = ResourceLoaderSkinModule::getLogo( $this->getConfig() );
-
-               $tags = [];
-               $logosPerDppx = [];
-               $logos = [];
-
-               if ( !is_array( $logo ) ) {
-                       // No media queries required if we only have one variant
-                       $this->addLinkHeader( '<' . $logo . '>;rel=preload;as=image' );
-                       return;
-               }
-
-               if ( isset( $logo['svg'] ) ) {
-                       // No media queries required if we only have a 1x and svg variant
-                       // because all preload-capable browsers support SVGs
-                       $this->addLinkHeader( '<' . $logo['svg'] . '>;rel=preload;as=image' );
-                       return;
-               }
-
-               foreach ( $logo as $dppx => $src ) {
-                       // Keys are in this format: "1.5x"
-                       $dppx = substr( $dppx, 0, -1 );
-                       $logosPerDppx[$dppx] = $src;
-               }
-
-               // Because PHP can't have floats as array keys
-               uksort( $logosPerDppx, function ( $a , $b ) {
-                       $a = floatval( $a );
-                       $b = floatval( $b );
-                       // Sort from smallest to largest (e.g. 1x, 1.5x, 2x)
-                       return $a <=> $b;
-               } );
-
-               foreach ( $logosPerDppx as $dppx => $src ) {
-                       $logos[] = [ 'dppx' => $dppx, 'src' => $src ];
-               }
-
-               $logosCount = count( $logos );
-               // Logic must match ResourceLoaderSkinModule:
-               // - 1x applies to resolution < 1.5dppx
-               // - 1.5x applies to resolution >= 1.5dppx && < 2dppx
-               // - 2x applies to resolution >= 2dppx
-               // Note that min-resolution and max-resolution are both inclusive.
-               for ( $i = 0; $i < $logosCount; $i++ ) {
-                       if ( $i === 0 ) {
-                               // Smallest dppx
-                               // min-resolution is ">=" (larger than or equal to)
-                               // "not min-resolution" is essentially "<"
-                               $media_query = 'not all and (min-resolution: ' . $logos[ 1 ]['dppx'] . 'dppx)';
-                       } elseif ( $i !== $logosCount - 1 ) {
-                               // In between
-                               // Media query expressions can only apply "not" to the entire expression
-                               // (e.g. can't express ">= 1.5 and not >= 2).
-                               // Workaround: Use <= 1.9999 in place of < 2.
-                               $upper_bound = floatval( $logos[ $i + 1 ]['dppx'] ) - 0.000001;
-                               $media_query = '(min-resolution: ' . $logos[ $i ]['dppx'] .
-                                       'dppx) and (max-resolution: ' . $upper_bound . 'dppx)';
-                       } else {
-                               // Largest dppx
-                               $media_query = '(min-resolution: ' . $logos[ $i ]['dppx'] . 'dppx)';
-                       }
-
-                       $this->addLinkHeader(
-                               '<' . $logos[$i]['src'] . '>;rel=preload;as=image;media=' . $media_query
-                       );
-               }
-       }
-
        /**
         * Get (and set if not yet set) the CSP nonce.
         *
index de25d32..69313bf 100644 (file)
@@ -76,6 +76,89 @@ class ResourceLoaderSkinModule extends ResourceLoaderFileModule {
                return $styles;
        }
 
+       /**
+        * @param ResourceLoaderContext $context
+        * @return array
+        */
+       public function getPreloadLinks( ResourceLoaderContext $context ) {
+               return $this->getLogoPreloadlinks();
+       }
+
+       /**
+        * Helper method for getPreloadLinks()
+        * @return array
+        */
+       private function getLogoPreloadlinks() {
+               $logo = $this->getLogoData( $this->getConfig() );
+
+               $tags = [];
+               $logosPerDppx = [];
+               $logos = [];
+
+               $preloadLinks = [];
+
+               if ( !is_array( $logo ) ) {
+                       // No media queries required if we only have one variant
+                       $preloadLinks[ $logo ] = [ 'as' => 'image' ];
+                       return $preloadLinks;
+               }
+
+               if ( isset( $logo['svg'] ) ) {
+                       // No media queries required if we only have a 1x and svg variant
+                       // because all preload-capable browsers support SVGs
+                       $preloadLinks [ $logo['svg'] ] = [ 'as' => 'image' ];
+                       return $preloadLinks;
+               }
+
+               foreach ( $logo as $dppx => $src ) {
+                       // Keys are in this format: "1.5x"
+                       $dppx = substr( $dppx, 0, -1 );
+                       $logosPerDppx[$dppx] = $src;
+               }
+
+               // Because PHP can't have floats as array keys
+               uksort( $logosPerDppx, function ( $a , $b ) {
+                       $a = floatval( $a );
+                       $b = floatval( $b );
+                       // Sort from smallest to largest (e.g. 1x, 1.5x, 2x)
+                       return $a <=> $b;
+               } );
+
+               foreach ( $logosPerDppx as $dppx => $src ) {
+                       $logos[] = [ 'dppx' => $dppx, 'src' => $src ];
+               }
+
+               $logosCount = count( $logos );
+               // Logic must match ResourceLoaderSkinModule:
+               // - 1x applies to resolution < 1.5dppx
+               // - 1.5x applies to resolution >= 1.5dppx && < 2dppx
+               // - 2x applies to resolution >= 2dppx
+               // Note that min-resolution and max-resolution are both inclusive.
+               for ( $i = 0; $i < $logosCount; $i++ ) {
+                       if ( $i === 0 ) {
+                               // Smallest dppx
+                               // min-resolution is ">=" (larger than or equal to)
+                               // "not min-resolution" is essentially "<"
+                               $media_query = 'not all and (min-resolution: ' . $logos[ 1 ]['dppx'] . 'dppx)';
+                       } elseif ( $i !== $logosCount - 1 ) {
+                               // In between
+                               // Media query expressions can only apply "not" to the entire expression
+                               // (e.g. can't express ">= 1.5 and not >= 2).
+                               // Workaround: Use <= 1.9999 in place of < 2.
+                               $upper_bound = floatval( $logos[ $i + 1 ]['dppx'] ) - 0.000001;
+                               $media_query = '(min-resolution: ' . $logos[ $i ]['dppx'] .
+                                       'dppx) and (max-resolution: ' . $upper_bound . 'dppx)';
+                       } else {
+                               // Largest dppx
+                               $media_query = '(min-resolution: ' . $logos[ $i ]['dppx'] . 'dppx)';
+                       }
+
+                       $preloadLinks[ $logos[$i]['src'] ] = [ 'as' => 'image', 'media' => $media_query ];
+               }
+
+               return $preloadLinks;
+       }
+
        /**
         * Ensure all media keys use array values.
         *
index b05fb0b..2f5e0c8 100644 (file)
@@ -503,6 +503,10 @@ abstract class Skin extends ContextSource {
 
        /**
         * Whether the logo should be preloaded with an HTTP link header or not
+        *
+        * @deprecated since 1.32 Redundant. It now happens automatically based on whether
+        *  the skin loads a stylesheet based on ResourceLoaderSkinModule, which all
+        *  skins that use wgLogo in CSS do, and other's would not.
         * @since 1.29
         * @return bool
         */
index b0cefc7..1f3ee27 100644 (file)
@@ -2039,26 +2039,26 @@ class OutputPageTest extends MediaWikiTestCase {
 
        /**
         * @dataProvider providePreloadLinkHeaders
-        * @covers OutputPage::addLogoPreloadLinkHeaders
+        * @covers ResourceLoaderSkinModule::getPreloadLinks
+        * @covers ResourceLoaderSkinModule::getLogoPreloadlinks
         * @covers ResourceLoaderSkinModule::getLogo
         */
-       public function testPreloadLinkHeaders( $config, $result, $baseDir = null ) {
-               if ( $baseDir ) {
-                       $this->setMwGlobals( 'IP', $baseDir );
-               }
-               $out = TestingAccessWrapper::newFromObject( $this->newInstance( $config ) );
-               $out->addLogoPreloadLinkHeaders();
+       public function testPreloadLinkHeaders( $config, $result ) {
+               $this->setMwGlobals( $config );
+               $ctx = $this->getMockBuilder( ResourceLoaderContext::class )
+                       ->disableOriginalConstructor()->getMock();
+               $module = new ResourceLoaderSkinModule();
 
-               $this->assertEquals( $result, $out->getLinkHeader() );
+               $this->assertEquals( [ $result ], $module->getHeaders( $ctx ) );
        }
 
        public function providePreloadLinkHeaders() {
                return [
                        [
                                [
-                                       'ResourceBasePath' => '/w',
-                                       'Logo' => '/img/default.png',
-                                       'LogoHD' => [
+                                       'wgResourceBasePath' => '/w',
+                                       'wgLogo' => '/img/default.png',
+                                       'wgLogoHD' => [
                                                '1.5x' => '/img/one-point-five.png',
                                                '2x' => '/img/two-x.png',
                                        ],
@@ -2071,17 +2071,17 @@ class OutputPageTest extends MediaWikiTestCase {
                        ],
                        [
                                [
-                                       'ResourceBasePath' => '/w',
-                                       'Logo' => '/img/default.png',
-                                       'LogoHD' => false,
+                                       'wgResourceBasePath' => '/w',
+                                       'wgLogo' => '/img/default.png',
+                                       'wgLogoHD' => false,
                                ],
                                'Link: </img/default.png>;rel=preload;as=image'
                        ],
                        [
                                [
-                                       'ResourceBasePath' => '/w',
-                                       'Logo' => '/img/default.png',
-                                       'LogoHD' => [
+                                       'wgResourceBasePath' => '/w',
+                                       'wgLogo' => '/img/default.png',
+                                       'wgLogoHD' => [
                                                '2x' => '/img/two-x.png',
                                        ],
                                ],
@@ -2091,9 +2091,9 @@ class OutputPageTest extends MediaWikiTestCase {
                        ],
                        [
                                [
-                                       'ResourceBasePath' => '/w',
-                                       'Logo' => '/img/default.png',
-                                       'LogoHD' => [
+                                       'wgResourceBasePath' => '/w',
+                                       'wgLogo' => '/img/default.png',
+                                       'wgLogoHD' => [
                                                'svg' => '/img/vector.svg',
                                        ],
                                ],
@@ -2102,13 +2102,13 @@ class OutputPageTest extends MediaWikiTestCase {
                        ],
                        [
                                [
-                                       'ResourceBasePath' => '/w',
-                                       'Logo' => '/w/test.jpg',
-                                       'LogoHD' => false,
-                                       'UploadPath' => '/w/images',
+                                       'wgResourceBasePath' => '/w',
+                                       'wgLogo' => '/w/test.jpg',
+                                       'wgLogoHD' => false,
+                                       'wgUploadPath' => '/w/images',
+                                       'IP' => dirname( __DIR__ ) . '/data/media',
                                ],
                                'Link: </w/test.jpg?edcf2>;rel=preload;as=image',
-                               'baseDir' => dirname( __DIR__ ) . '/data/media',
                        ],
                ];
        }