r108184 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r108183‎ | r108184 | r108185 >
Date:23:32, 5 January 2012
Author:catrope
Status:ok (Comments)
Tags:
Comment:
ResourceLoader: Add an experimental option to move the main module loading queue (the bottom queue) from the bottom of the <body> up into the <head> , while still being loaded asynchronously. This makes them load earlier, which should make the page load faster. This is the product of a long discussion on bug 27488

* Added a "blocking" state to mw.loader . When loading scripts while the document is not ready, the loader will use document.write() if blocking is true, and append to the <body> or the <head> if blocking is false. If the document is ready, the loader will always append to the <body>
* Enable blocking mode while loading the top queue, and disable it after. This ensures that modules in the top queue are still loaded in a blocking way as they were before
* If $wgResourceLoaderExperimentalAsyncLoading is true, the bottom queue is also loaded in the head, but with blocking mode disabled. Otherwise, it's loaded at the bottom of the <body> as before
* scripts-only and messages-only requests need special treatment:
** in the top queue, they can continue to use <script src="..."> tags because they are blocking
** if the bottom queue is at the bottom of the <body> (experimental async loading disabled), they can continue to use <script src="..."> tags as before
** if the bottom queue is in the <head> (experimental async loading enabled), they cannot use <script src="..."> tags, because those would block. Instead, call mw.loader.load() on the load.php URL
Modified paths:
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
——@@ -2494,9 +2494,10 @@
24952495 * @param $only String ResourceLoaderModule TYPE_ class constant
24962496 * @param $useESI boolean
24972497 * @param $extraQuery Array with extra query parameters to add to each request. array( param => value )
 2498+ * @param $loadCall boolean If true, output a mw.loader.load() call rather than a <script src="..."> tag
24982499 * @return string html <script> and <style> tags
24992500 */
2500 - protected function makeResourceLoaderLink( $modules, $only, $useESI = false, array $extraQuery = array() ) {
 2501+ protected function makeResourceLoaderLink( $modules, $only, $useESI = false, array $extraQuery = array(), $loadCall = false ) {
25012502 global $wgResourceLoaderUseESI, $wgResourceLoaderInlinePrivateModules;
25022503
25032504 if ( !count( $modules ) ) {
——@@ -2633,6 +2634,12 @@
26342635 // Automatically select style/script elements
26352636 if ( $only === ResourceLoaderModule::TYPE_STYLES ) {
26362637 $link = Html::linkedStyle( $url );
 2638+ } else if ( $loadCall ) {
 2639+ $link = Html::inlineScript(
 2640+ ResourceLoader::makeLoaderConditionalScript(
 2641+ Xml::encodeJsCall( 'mw.loader.load', array( $url ) )
 2642+ )
 2643+ );
26372644 } else {
26382645 $link = Html::linkedScript( $url );
26392646 }
——@@ -2654,6 +2661,8 @@
26552662 * @return String: HTML fragment
26562663 */
26572664 function getHeadScripts() {
 2665+ global $wgResourceLoaderExperimentalAsyncLoading;
 2666+
26582667 // Startup - this will immediately load jquery and mediawiki modules
26592668 $scripts = $this->makeResourceLoaderLink( 'startup', ResourceLoaderModule::TYPE_SCRIPTS, true );
26602669
——@@ -2679,27 +2688,43 @@
26802689 if ( $modules ) {
26812690 $scripts .= Html::inlineScript(
26822691 ResourceLoader::makeLoaderConditionalScript(
2683 - Xml::encodeJsCall( 'mw.loader.load', array( $modules ) )
 2692+ "mw.loader.setBlocking( true );\n" .
 2693+ Xml::encodeJsCall( 'mw.loader.load', array( $modules ) ) .
 2694+ "\nmw.loader.setBlocking( false );"
26842695 )
26852696 );
26862697 }
 2698+
 2699+ if ( $wgResourceLoaderExperimentalAsyncLoading ) {
 2700+ $scripts .= $this->getScriptsForBottomQueue( true );
 2701+ }
26872702
26882703 return $scripts;
26892704 }
26902705
26912706 /**
2692 - * JS stuff to put at the bottom of the <body>: modules marked with position 'bottom',
2693 - * legacy scripts ($this->mScripts), user preferences, site JS and user JS
 2707+ * JS stuff to put at the 'bottom', which can either be the bottom of the <body>
 2708+ * or the bottom of the <head> depending on $wgResourceLoaderExperimentalAsyncLoading:
 2709+ * modules marked with position 'bottom', legacy scripts ($this->mScripts),
 2710+ * user preferences, site JS and user JS
26942711 *
 2712+ * @param $inHead boolean If true, this HTML goes into the <head>, if false it goes into the <body>
26952713 * @return string
26962714 */
2697 - function getBottomScripts() {
 2715+ function getScriptsForBottomQueue( $inHead ) {
26982716 global $wgUseSiteJs, $wgAllowUserJs;
26992717
27002718 // Script and Messages "only" requests marked for bottom inclusion
 2719+ // If we're in the <head>, use load() calls rather than <script src="..."> tags
27012720 // Messages should go first
2702 - $scripts = $this->makeResourceLoaderLink( $this->getModuleMessages( true, 'bottom' ), ResourceLoaderModule::TYPE_MESSAGES );
2703 - $scripts .= $this->makeResourceLoaderLink( $this->getModuleScripts( true, 'bottom' ), ResourceLoaderModule::TYPE_SCRIPTS );
 2721+ $scripts = $this->makeResourceLoaderLink( $this->getModuleMessages( true, 'bottom' ),
 2722+ ResourceLoaderModule::TYPE_MESSAGES, /* $useESI = */ false, /* $extraQuery = */ array(),
 2723+ /* $loadCall = */ $inHead
 2724+ );
 2725+ $scripts .= $this->makeResourceLoaderLink( $this->getModuleScripts( true, 'bottom' ),
 2726+ ResourceLoaderModule::TYPE_SCRIPTS, /* $useESI = */ false, /* $extraQuery = */ array(),
 2727+ /* $loadCall = */ $inHead
 2728+ );
27042729
27052730 // Modules requests - let the client calculate dependencies and batch requests as it likes
27062731 // Only load modules that have marked themselves for loading at the bottom
——@@ -2719,7 +2744,9 @@
27202745
27212746 // Add site JS if enabled
27222747 if ( $wgUseSiteJs ) {
2723 - $scripts .= $this->makeResourceLoaderLink( 'site', ResourceLoaderModule::TYPE_SCRIPTS );
 2748+ $scripts .= $this->makeResourceLoaderLink( 'site', ResourceLoaderModule::TYPE_SCRIPTS,
 2749+ /* $useESI = */ false, /* $extraQuery = */ array(), /* $loadCall = */ $inHead
 2750+ );
27242751 if( $this->getUser()->isLoggedIn() ){
27252752 $userScripts[] = 'user.groups';
27262753 }
——@@ -2732,7 +2759,7 @@
27332760 // We're on a preview of a JS subpage
27342761 // Exclude this page from the user module in case it's in there (bug 26283)
27352762 $scripts .= $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_SCRIPTS, false,
2736 - array( 'excludepage' => $this->getTitle()->getPrefixedDBkey() )
 2763+ array( 'excludepage' => $this->getTitle()->getPrefixedDBkey() ), $inHead
27372764 );
27382765 // Load the previewed JS
27392766 $scripts .= Html::inlineScript( "\n" . $this->getRequest()->getText( 'wpTextbox1' ) . "\n" ) . "\n";
——@@ -2740,15 +2767,31 @@
27412768 // Include the user module normally
27422769 // We can't do $userScripts[] = 'user'; because the user module would end up
27432770 // being wrapped in a closure, so load it raw like 'site'
2744 - $scripts .= $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_SCRIPTS );
 2771+ $scripts .= $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_SCRIPTS,
 2772+ /* $useESI = */ false, /* $extraQuery = */ array(), /* $loadCall = */ $inHead
 2773+ );
27452774 }
27462775 }
2747 - $scripts .= $this->makeResourceLoaderLink( $userScripts, ResourceLoaderModule::TYPE_COMBINED );
 2776+ $scripts .= $this->makeResourceLoaderLink( $userScripts, ResourceLoaderModule::TYPE_COMBINED,
 2777+ /* $useESI = */ false, /* $extraQuery = */ array(), /* $loadCall = */ $inHead
 2778+ );
27482779
27492780 return $scripts;
27502781 }
27512782
27522783 /**
 2784+ * JS stuff to put at the bottom of the <body>
 2785+ */
 2786+ function getBottomScripts() {
 2787+ global $wgResourceLoaderExperimentalAsyncLoading;
 2788+ if ( !$wgResourceLoaderExperimentalAsyncLoading ) {
 2789+ return $this->getScriptsForBottomQueue( false );
 2790+ } else {
 2791+ return '';
 2792+ }
 2793+ }
 2794+
 2795+ /**
27532796 * Add one or more variables to be set in mw.config in JavaScript.
27542797 *
27552798 * @param $key {String|Array} Key or array of key/value pars.
Index: trunk/phase3/includes/DefaultSettings.php
——@@ -2646,6 +2646,13 @@
26472647 */
26482648 $wgResourceLoaderValidateStaticJS = false;
26492649
 2650+/**
 2651+ * If set to true, asynchronous loading of bottom-queue scripts in the <head>
 2652+ * will be enabled. This is an experimental feature that's supposed to make
 2653+ * JavaScript load faster.
 2654+ */
 2655+$wgResourceLoaderExperimentalAsyncLoading = false;
 2656+
26502657 /** @} */ # End of resource loader settings }
26512658
26522659
Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -346,8 +346,11 @@
347347 queue = [],
348348 // List of callback functions waiting for modules to be ready to be called
349349 jobs = [],
350 - // Flag inidicating that document ready has occured
 350+ // Flag indicating that document ready has occured
351351 ready = false,
 352+ // Whether we should try to load scripts in a blocking way
 353+ // Set with setBlocking()
 354+ blocking = false,
352355 // Selector cache for the marker element. Use getMarker() to get/use the marker!
353356 $marker = null;
354357
——@@ -569,15 +572,15 @@
570573 }
571574
572575 /**
573 - * Adds a script tag to the body, either using document.write or low-level DOM manipulation,
574 - * depending on whether document-ready has occured yet.
 576+ * Adds a script tag to the DOM, either using document.write or low-level DOM manipulation,
 577+ * depending on whether document-ready has occured yet and whether we are in blocking mode.
575578 *
576579 * @param src String: URL to script, will be used as the src attribute in the script tag
577580 * @param callback Function: Optional callback which will be run when the script is done
578581 */
579582 function addScript( src, callback ) {
580 - var done = false, script;
581 - if ( ready ) {
 583+ var done = false, script, head;
 584+ if ( ready || !blocking ) {
582585 // jQuery's getScript method is NOT better than doing this the old-fashioned way
583586 // because jQuery will eval the script's code, and errors will not have sane
584587 // line numbers.
——@@ -612,13 +615,18 @@
613616 }
614617 };
615618 }
616 - document.body.appendChild( script );
 619+ // IE-safe way of getting the <head> . document.documentElement.head doesn't
 620+ // work in scripts that run in the <head>
 621+ head = document.getElementsByTagName( 'head' )[0];
 622+ // Append to the <body> if available, to the <head> otherwise
 623+ (document.body || head).appendChild( script );
617624 } else {
618625 document.write( mw.html.element(
619626 'script', { 'type': 'text/javascript', 'src': src }, ''
620627 ) );
621628 if ( $.isFunction( callback ) ) {
622629 // Document.write is synchronous, so this is called when it's done
 630+ // FIXME: that's a lie. doc.write isn't actually synchronous
623631 callback();
624632 }
625633 }
——@@ -1221,6 +1229,18 @@
12221230 return key;
12231231 } );
12241232 },
 1233+
 1234+ /**
 1235+ * Enable or disable blocking. If blocking is enabled and
 1236+ * document ready has not yet occurred, scripts will be loaded
 1237+ * in a blocking way (using document.write) rather than
 1238+ * asynchronously using DOM manipulation
 1239+ *
 1240+ * @param b {Boolean} True to enable blocking, false to disable it
 1241+ */
 1242+ setBlocking: function( b ) {
 1243+ blocking = b;
 1244+ },
12251245
12261246 /**
12271247 * For backwards-compatibility with Squid-cached pages. Loads mw.user

Follow-up revisions

RevisionCommit summaryAuthorDate
r108235Followup r108184: fix loading in Opera. Before, Opera would only begin to ren...catrope15:06, 6 January 2012
r110592Followup r108184: per CR, blocking loads in debug mode were broken because de...catrope16:38, 2 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r85616For bug 27488: move the startup script, jquery+mediawiki and the mw.config.se...catrope12:07, 7 April 2011

Comments

#Comment by Catrope (talk | contribs)   23:41, 5 January 2012

This is broken on Opera if the experimental setting is enabled: the display of the entire page is delayed until all scripts have loaded. This is a regression compared to the current state, where DOM ready is delayed but display of the page isn't. I'll look into circumventing this issue tomorrow.

#Comment by Krinkle (talk | contribs)   01:08, 6 January 2012

See [https://docs.google.com/spreadsheet/ccc?key=0ApsGwg5TzvlwdG11YVBWMU9WYnVYbzI3Rm9lbGd6S2c google document] for the browser support for this, as tested in a simplified HTML document.

#Comment by Krinkle (talk | contribs)   01:16, 6 January 2012

In very old Firefox versions and all versions of Opera, the document ready event is delayed until after the default ('bottom') queue is loaded. This is not a huge problem, and besides, there is nothing we can do about it, and it's also the case for the current ResourceLoader technique in MediaWiki 1.18 as well as before MediaWiki 1.17 and ResourceLoader, so not new.

What was a regression (or more of a side-effect) is that due to the fact that Opera is the only browser not offering "async" for script tags, anything in document after the load command for the default queue, will remain invisible / not rendered, until after that queue is loaded. This was also the case before this change, however, previously that load command was at the bottom of the body, so there is nothing left to block. Since we're now doing it from the HEAD, it is blocking the page content.

Short summary about scripts and async behavior:

Scripts tags naturally in the document or inserted via document.write are blocking further rendering and also block each other and do so in order. Script DOM elements inserted are asynchronous by default (except for Opera), this is true for new browsers (as they support the HTML5 async property and have it set to true for default), as well as for old browsers (which didn't offer the async property but have been loading them asynchronously by default without a way to do otherwise, and have been doing so for a long time)
#Comment by Nikerabbit (talk | contribs)   10:12, 6 January 2012

Looks like this is breaking some scripts even when the new global is not set to true.

#Comment by Nikerabbit (talk | contribs)   10:16, 6 January 2012

Some examples which I think are all caused by this: 


      3 Uncaught TypeError: Cannot call method 'setup' of undefined     [http://translatewiki.net/w/extensions/Kieli/ext.kieli.js:4 http://translatewiki.net/w/extensions/Kieli/ext.kieli.js:4]
      4 mw.toolbar is undefined [https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-editbuttons.js&action=raw&ctype=text/javascript&3337614:98 https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-editbuttons.js&action=raw&ctype=text/javascript&3337614:98]
     14 Uncaught ReferenceError: hookEvent is not defined       [http://translatewiki.net/w/i.php?title=MediaWiki:Gadget-prota.js&action=raw&ctype=text/javascript&924496:39 http://translatewiki.net/w/i.php?title=MediaWiki:Gadget-prota.js&action=raw&ctype=text/javascript&924496:39]
     20 addOnloadHook is not defined    [https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-MsgToPortal.js&action=raw&ctype=text/javascript&3585808:20 https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-MsgToPortal.js&action=raw&ctype=text/javascript&3585808:20]
     22 Uncaught ReferenceError: addOnloadHook is not defined   [http://translatewiki.net/w/i.php?title=MediaWiki:Gadget-translatetab.js&action=raw&ctype=text/javascript&3585797:11 http://translatewiki.net/w/i.php?title=MediaWiki:Gadget-translatetab.js&action=raw&ctype=text/javascript&3585797:11]
     23 addOnloadHook is not defined    [https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-renameuser.js&action=raw&ctype=text/javascript&3585802:2 https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-renameuser.js&action=raw&ctype=text/javascript&3585802:2]
     23 addOnloadHook is not defined    [https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-setuserrights.js&action=raw&ctype=text/javascript&3585786:16 https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-setuserrights.js&action=raw&ctype=text/javascript&3585786:16]
     34 mw.toolbar is undefined [http://translatewiki.net/w/i.php?title=MediaWiki:Gadget-editbuttons.js&action=raw&ctype=text/javascript&3337614:98 http://translatewiki.net/w/i.php?title=MediaWiki:Gadget-editbuttons.js&action=raw&ctype=text/javascript&3337614:98]
     39 addOnloadHook is not defined    [https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-translatetab.js&action=raw&ctype=text/javascript&3585797:11 https://translatewiki.net/w/i.php?title=MediaWiki:Gadget-translatetab.js&action=raw&ctype=text/javascript&3585797:11]
#Comment by Catrope (talk | contribs)   14:56, 6 January 2012

This was fixed in r108220 and r108222.

#Comment by Nikerabbit (talk | contribs)   17:55, 13 January 2012

There are some JS errors in twn log: 

2012-01-11T03:17:22UTC  mw.loader.setBlocking is not a function http://translatewiki.net/wiki/Translating:Kiwix:31
>X.Y.Z.K  Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
>URL: http://translatewiki.net/wiki/Translating:Kiwix
>STACK: ("mw.loader.setBlocking is not a function","http://translatewiki.net/wiki/Translating:Kiwix",31)@http://translatewiki.net/w/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki%7Ctwn.jserrorlog&only=scripts&skin=vector&version=20111230T233043Z:156
#Comment by Krinkle (talk | contribs)   02:47, 22 January 2012

Re-opening. Top queue looks like this: 

mw.loader.setBlocking( true );
mw.loader.load( .. );
mw.loader.setBlocking( false );

The requests made to load.php by mw.loader.load() are added synchronously, these are all correctly made blocking. However, as soon as mw.loader.load() returns, blocking is set to false. This works well for production mode, but fails in debug mode.

In debug mode the load.php calls contain mw.loader.implement(), which contain file paths. Which implement() will addScript, but at that point blocking is apparently false. I haven't quite figured out what the exact cause is).

Status & tagging log