r106007 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r106006‎ | r106007 | r106008 >
Date:10:41, 13 December 2011
Author:liangent
Status:reverted (Comments)
Tags:
Comment:
Followup r100497, r101827: use another way which doesn't look that hacky to load styles in gadgets before documents. Also avoid double loading of the same stylesheet.

Now a gadget generates up to three modules:
* The first is known as meta module which still use its original name to keep B/C. It contains no contents, with dependencies of ones specified by user, scripts module, and styles module. See below.
* The second is known as scripts module whose name ends with ".scripts". It contains all scripts in this gadgets, with dependencies specified by user
* The third is known as styles module whose name ends with ".styles". It contains all styles in this gadgets and loaded before documents. After its load it's instantly marked as ready without dependencies resolved (which is the same as dependencies of .scripts but may not be used at all)

The meta module is loaded as if there's only one module under that name, and it's safe for users to reuse that name. The styles module is loaded and marked as ready in <head>, as described above. Normally the scripts and styles modules shouldn't be used by users.

A possible bug that dependency-only gadgets (ie. no scripts or styles) may not be loaded is fixed.
Modified paths:
  • /trunk/extensions/Gadgets/Gadgets_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Gadgets/Gadgets_body.php
——@@ -11,7 +11,7 @@
1212 * @license GNU General Public Licence 2.0 or later
1313 */
1414
15 -class GadgetHooks {
 15+class GadgetHooks extends OutputPage {
1616 /**
1717 * ArticleSaveComplete hook handler.
1818 *
——@@ -123,9 +123,20 @@
124124
125125 foreach ( $gadgets as $g ) {
126126 $module = $g->getModule();
 127+ $scriptsModule = $g->getScriptsModule();
 128+ $stylesModule = $g->getStylesModule();
 129+
127130 if ( $module ) {
128131 $resourceLoader->register( $g->getModuleName(), $module );
129132 }
 133+
 134+ if ( $scriptsModule ) {
 135+ $resourceLoader->register( $g->getScriptsModuleName(), $scriptsModule );
 136+ }
 137+
 138+ if ( $stylesModule ) {
 139+ $resourceLoader->register( $g->getStylesModuleName(), $stylesModule );
 140+ }
130141 }
131142 return true;
132143
——@@ -150,13 +161,17 @@
151162 $lb->setCaller( __METHOD__ );
152163 $pages = array();
153164
 165+ $stylesModules = array();
154166 foreach ( $gadgets as $gadget ) {
155167 if ( $gadget->isEnabled( $wgUser ) && $gadget->isAllowed( $wgUser ) ) {
156168 if ( $gadget->hasModule() ) {
157 - $out->addModuleStyles( $gadget->getModuleName() );
158169 $out->addModules( $gadget->getModuleName() );
159170 }
160171
 172+ if ( $gadget->hasStylesModule() ) {
 173+ $stylesModules[] = $gadget->getStylesModuleName();
 174+ }
 175+
161176 foreach ( $gadget->getLegacyScripts() as $page ) {
162177 $lb->add( NS_MEDIAWIKI, $page );
163178 $pages[] = $page;
——@@ -164,6 +179,18 @@
165180 }
166181 }
167182
 183+ if ( count( $stylesModules ) ) {
 184+ $out->addHeadItem( 'ext.gadget', $out->makeResourceLoaderLink(
 185+ $stylesModules, ResourceLoaderModule::TYPE_STYLES
 186+ ) . Html::inlineScript(
 187+ ResourceLoader::makeLoaderConditionalScript(
 188+ ResourceLoader::makeLoaderStateScript(
 189+ array_fill_keys( $stylesModules, 'ready' )
 190+ )
 191+ )
 192+ ) );
 193+ }
 194+
168195 $lb->execute( __METHOD__ );
169196
170197 $done = array();
——@@ -328,13 +355,27 @@
329356 }
330357
331358 /**
332 - * @return String: Name of ResourceLoader module for this gadget
 359+ * @return String: Name of meta ResourceLoader module for this gadget
333360 */
334361 public function getModuleName() {
335362 return "ext.gadget.{$this->name}";
336363 }
337364
338365 /**
 366+ * @return String: Name of scripts ResourceLoader module for this gadget
 367+ */
 368+ public function getScriptsModuleName() {
 369+ return "ext.gadget.{$this->name}.scripts";
 370+ }
 371+
 372+ /**
 373+ * @return String: Name of styles ResourceLoader module for this gadget
 374+ */
 375+ public function getStylesModuleName() {
 376+ return "ext.gadget.{$this->name}.styles";
 377+ }
 378+
 379+ /**
339380 * Checks whether this is an instance of an older version of this class deserialized from cache
340381 * @return Boolean
341382 */
——@@ -381,12 +422,24 @@
382423 * @return Boolean: Whether this gadget has resources that can be loaded via ResourceLoader
383424 */
384425 public function hasModule() {
385 - return count( $this->styles )
386 - + ( $this->supportsResourceLoader() ? count( $this->scripts ) : 0 )
387 - > 0;
 426+ return $this->hasScriptsModule() || $this->hasStylesModule() || count( $this->dependencies );
388427 }
389428
390429 /**
 430+ * @return Boolean: Whether this gadget has scripts module to be loaded via ResourceLoader
 431+ */
 432+ public function hasScriptsModule() {
 433+ return $this->supportsResourceLoader() && ( count( $this->scripts ) > 0 );
 434+ }
 435+
 436+ /**
 437+ * @return Boolean: Whether this gadget has styles module to be loaded via ResourceLoader
 438+ */
 439+ public function hasStylesModule() {
 440+ return count( $this->styles ) > 0;
 441+ }
 442+
 443+ /**
391444 * @return String: Definition for this gadget from MediaWiki:gadgets-definition
392445 */
393446 public function getDefinition() {
——@@ -415,17 +468,43 @@
416469 }
417470
418471 /**
419 - * Returns module for ResourceLoader, see getModuleName() for its name.
420 - * If our gadget has no scripts or styles suitable for RL, false will be returned.
 472+ * Returns meta module for ResourceLoader, see getModuleName() for its name.
 473+ * Meta module contains no real data to be loaded but has dependencies of
 474+ * this gadget's scripts and styles module as well as other dependencies.
 475+ * If our gadget has no scripts or styles or dependencies suitable for RL,
 476+ * false will be returned.
421477 * @return Mixed: GadgetResourceLoaderModule or false
422478 */
423479 public function getModule() {
424 - $pages = array();
 480+ // Don't omit this. Styles module may be marked as ready with
 481+ // mw.loader.state() without dependencies fully resolved.
 482+ $dependencies = $this->dependencies;
425483
426 - foreach ( $this->styles as $style ) {
427 - $pages['MediaWiki:' . $style] = array( 'type' => 'style' );
 484+ if ( $this->hasScriptsModule() ) {
 485+ $dependencies[] = $this->getScriptsModuleName();
428486 }
429487
 488+ if ( $this->hasStylesModule() ) {
 489+ $dependencies[] = $this->getStylesModuleName();
 490+ }
 491+
 492+ if ( !count( $dependencies ) ) {
 493+ return false;
 494+ }
 495+
 496+ return new GadgetResourceLoaderModule( array(), $dependencies );
 497+ }
 498+
 499+ /**
 500+ * Returns scripts module for ResourceLoader, see getScriptsModuleName()
 501+ * for its name. This module contains all scripts in the gadget if this
 502+ * gadget support ResourceLoader, or false if it doesn't, or there's no
 503+ * script at all.
 504+ * @return Mixed: GadgetResourceLoaderModule or false
 505+ */
 506+ public function getScriptsModule() {
 507+ $pages = array();
 508+
430509 if ( $this->supportsResourceLoader() ) {
431510 foreach ( $this->scripts as $script ) {
432511 $pages['MediaWiki:' . $script] = array( 'type' => 'script' );
——@@ -433,13 +512,33 @@
434513 }
435514
436515 if ( !count( $pages ) ) {
437 - return null;
 516+ return false;
438517 }
439518
440519 return new GadgetResourceLoaderModule( $pages, $this->dependencies );
441520 }
442521
443522 /**
 523+ * Returns styles module for ResourceLoader, see getStylesModuleName()
 524+ * for its name. This module contains all styles in the gadget, or
 525+ * false if there's no style.
 526+ * @return Mixed: GadgetResourceLoaderModule or false
 527+ */
 528+ public function getStylesModule() {
 529+ $pages = array();
 530+
 531+ foreach ( $this->styles as $style ) {
 532+ $pages['MediaWiki:' . $style] = array( 'type' => 'style' );
 533+ }
 534+
 535+ if ( !count( $pages ) ) {
 536+ return false;
 537+ }
 538+
 539+ return new GadgetResourceLoaderModule( $pages, $this->dependencies );
 540+ }
 541+
 542+ /**
444543 * Returns list of scripts that don't support ResourceLoader
445544 * @return Array
446545 */

Follow-up revisions

RevisionCommit summaryAuthorDate
r110600Revert r106007 (Split Gadget modules into two, one for scripts and one for st...catrope19:04, 2 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r100497Move styles in gadgets to the top of pages to avoid flash of unstyled content....liangent04:32, 22 October 2011
r101827Followup r100497: Fix stuff described thereliangent16:11, 3 November 2011

Comments

#Comment by Liangent (talk | contribs)   13:32, 13 December 2011

Number of modules per gadget can be reduced to two by merging meta and scripts (call them main and styles then) but having three seems more natural... anyway in case there is another type of component that we have to do something special for it in the future (will there be?).

#Comment by Johnduhart (talk | contribs)   19:30, 7 January 2012 
-class GadgetHooks {
+class GadgetHooks extends OutputPage {

This does not look right.

#Comment by Liangent (talk | contribs)   23:20, 7 January 2012

That's set so $out->makeResourceLoaderLink can be used.

#Comment by Johnduhart (talk | contribs)   05:44, 8 January 2012

Extending a class so you can access a protected function is not how you solve this problem. Figure out another solution. Leave this fixme unless another developer disagrees.

#Comment by Krinkle (talk | contribs)   19:51, 7 January 2012

The gadgets extension has been pretty much rewritten from scratch in the RL2 branch, primary focus on three features in there: Gadget repositories, Full ResourceLoader integration and a Gadget manager that allows easy modification and tracking of individual gadgets.

I'd recommend not adding new features or changing behavior of this extension without prior discussion in a bug ticket. So far the RL2 branch is trying to keep everything backwards compatible, it also has a maintenance script migration. It would be great if that would not have to be redone.

See also RL/V2SPEC

#Comment by Krinkle (talk | contribs)   19:55, 7 January 2012

Ah, follows up other revs, my bad.

Status & tagging log