resourceloader: Consistently set state=ready after script execution (not before)

For the case of 'module.script' being an array, mw.loader already sets state=ready
after execution. For the cases of it being a function or string, we were setting
state=ready before execution.

This looks like it may have intended to prevent double execution where e.g.
some other module may need this module and see it has state 'loading' and start
executing it. However ResourceLoader doesn't expose execute(). The only code
calling execute() is code that also sets 'state=loading', or handlePending.

In fact, this early setting of "ready" could actually cause double execution.
Not of the current module, but of dependent modules. If a module's script loads
other modules that are cache hits, that could may trigger handlePending() which
will assume the current module to be ready, when it isn't.

Now that these code paths match, re-use the markModuleReady callback instead of
repeating this code.

Update tests:
* Make assertion captions less verbose.
* Change load instruction from "test.implement" to "test.implement.import".
  Module "test.implement" doesn't exist. This worked by accident because
  implement() can sometimes run a module immediately if it doesn't exist.
* Update test to reflect internal detail that state=loading during script
  execution.
* Assert afterwards that script ran and state=ready.

Change-Id: I6b0542edb113d58b8f24cc8587e98ee88b514c55

diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js
index 56a4699..2632902 100644 (file)
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1210,9 +1210,8 @@
                                                        } else if ( $.isFunction( script ) ) {
                                                                // Pass jQuery twice so that the signature of the closure which wraps
                                                                // the script can bind both '$' and 'jQuery'.
-                                                               registry[module].state = 'ready';
                                                                script( $, $ );
-                                                               handlePending( module );
+                                                               markModuleReady();
                                                        } else if ( typeof script === 'string' ) {
                                                                // Site and user modules are a legacy scripts that run in the global scope.
                                                                // This is transported as a string instead of a function to avoid needing
@@ -1221,14 +1220,12 @@
                                                                        // Implicit dependency on the site module. Not real dependency because
                                                                        // it should run after 'site' regardless of whether it succeeds or fails.
                                                                        mw.loader.using( 'site' ).always( function () {
-                                                                               registry[module].state = 'ready';
                                                                                $.globalEval( script );
-                                                                               handlePending( module );
+                                                                               markModuleReady();
                                                                        } );
                                                                } else {
-                                                                       registry[module].state = 'ready';
                                                                        $.globalEval( script );
-                                                                       handlePending( module );
+                                                                       markModuleReady();
                                                                }
                                                        }
                                                } );

diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
index c8f0011..fcf6dda 100644 (file)
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
@@ -630,20 +630,20 @@
        } );
 
        // @import (bug 31676)
-       QUnit.asyncTest( 'mw.loader.implement( styles has @import)', 5, function ( assert ) {
+       QUnit.asyncTest( 'mw.loader.implement( styles has @import)', 7, function ( assert ) {
                var isJsExecuted, $element;
 
                mw.loader.implement(
                        'test.implement.import',
                        function () {
-                               assert.strictEqual( isJsExecuted, undefined, 'javascript not executed multiple times' );
+                               assert.strictEqual( isJsExecuted, undefined, 'script not executed multiple times' );
                                isJsExecuted = true;
 
-                               assert.equal( mw.loader.getState( 'test.implement.import' ), 'ready', 'module state is "ready" while implement() is executing javascript' );
+                               assert.equal( mw.loader.getState( 'test.implement.import' ), 'loading', 'module state during implement() script execution' );
 
                                $element = $( '<div class="mw-test-implement-import">Foo bar</div>' ).appendTo( '#qunit-fixture' );
 
-                               assert.equal( mw.msg( 'test-foobar' ), 'Hello Foobar, $1!', 'Messages are loaded before javascript execution' );
+                               assert.equal( mw.msg( 'test-foobar' ), 'Hello Foobar, $1!', 'messages load before script execution' );
 
                                assertStyleAsync( assert, $element, 'float', 'right', function () {
                                        assert.equal( $element.css( 'text-align' ), 'center',
@@ -666,8 +666,10 @@
                        }
                );
 
-               mw.loader.load( 'test.implement' );
-
+               mw.loader.using( 'test.implement.import' ).always( function () {
+                       assert.strictEqual( isJsExecuted, true, 'script executed' );
+                       assert.equal( mw.loader.getState( 'test.implement.import' ), 'ready', 'module state after script execution' );
+               } );
        } );
 
        QUnit.asyncTest( 'mw.loader.implement( dependency with styles )', 4, function ( assert ) {