r96309 MediaWiki - Code Review archive

None of the JS files added in this revision alias jQuery to $ in their closures.

+		return $( '<span class="' + o.prefix + 'prop"></span>' )

Eww. Generic functions should always escape their arguments. Never make the caller responsible for escaping. This pattern is repeated throughout the plugin; only one line uses addClass().

+	 *         <span editor="jquery-prop">

Weird typo?

+	 *     - onAdd {Function} Callback when an item is added
+	 *     - onRemove {Function} Callback when an item is deleted

You should document the parameter lists of these callbacks.

+		o.prefix = o.prefix + '-';

This is why mankind invented += ;) . Also, would it be that unreasonable to require that the caller put in the dash? That seems much more logical to me.

+				// Prevent duplicate values
+				if ( o.props.indexOf( val ) === -1 ) {
+					$container.append( newPropHtml( val, o ) );
+					o.onAdd( val );
+				}

Doesn't this fail to prevent duplicate added values, because val isn't added to o.props ? The way it looks like to me at a quick glance is that if you've got o.props = ['foo','bar'] it won't let you add foo or bar, but it will let you add baz twice.

		 * @var {Object|Null} If cache, object with category ids and the member counts */

That sentence doesn't make much sense to me. It's also nice to document the structure of these things, e.g. // { gadgetID: metadataObject }

I don't really understand the architectural decision to have a single callback with a parameter that specifies success or error rather than having separate callbacks for both cases (the latter pattern is used a lot in UploadWizard). When would the success callback and the error callback ever share code? You're also using your pattern inconsistently: getGadgetMetadata() and getGadgetCategories() use callback( {}, 'error' ) but doModifyGadget() uses callback( 'error', msg ) .

+.mw-gadgetmanager-propcontainer {}
[...]
+.mw-gadgetmanager-prop-label {}

???

						<legend>Module properties</legend>\

Hardcoded English. Isn't this jquery.localize-able?

		 * @param id {String}
		 * @param displayTitle {String}
		 */
		startGadgetEditor: function( gadget ) {

Documentation doesn't match the function signature.

					alert( "Save result: \n- status: " + status + "\n- msg: " + msg );

alert()? Really? :D

								var suggestions = $.map( json.query.gadgetpages, function( val, i ) {
									return val.pagename;
								});
								suggestions = suggestions.splice( 0, suggestLimit );

Wouldn't it make more sense to do the splice before the map, to eliminate wasted effort?

There's a lot of code duplication between the createPropCloud() calls for JS and CSS suggestions, and less so between the others. At least the former looks like it could be factored out.

			$form.find( '#mw-gadgetmanager-input-category' ).append( function() {

I had no idea that worked; cool feature.

+			$html .= '<th>' . wfMessage( 'gadgetmanager-tablehead-lastmod' )->escaped() . '</tr>';

No closing th tag. Probably cool in HTML5 mode but not so much in other modes.

+				$definitionTitle = Title::newFromText( $gadget->getName() . '.js', NS_GADGET_DEFINITION );

This doesn't work correctly if you happen to have a gadget called Talk:Foo. Yes, I know that's idiotic, but code needs to be idiot-proof when it comes to dealing with user-supplied data. Use Title::makeTitleSafe( NS_GADGET_DEFINITION, $str ) instead.

Marking fixme because of the escaping, closing th tag and newFromText issues.

See follow-up r96759.

Regarding the following:

+				// Prevent duplicate values
+				if ( o.props.indexOf( val ) === -1 ) {
+					$container.append( newPropHtml( val, o ) );
+					o.onAdd( val );
+				}

Doesn't this fail to prevent duplicate added values, because val isn't added to o.props ? The way it looks like to me at a quick glance is that if you've got o.props = ['foo','bar'] it won't let you add foo or bar, but it will let you add baz twice.

I agree it's not optimal. Right now the updating of o.props is expected to be handled by the callback (among other things). However I realize this is not handy, there was a reason for it though, however it turns out to be no longer valid. The reason for this behavior was because the options are (or could be) passed by value, instead of reference. Which means updating it here would not update the real object, hence we should let the callback handle this. However this rationale is no longer valid since the actual check is looking in the options' props, not some unknown 'real object'.

The plugin is not receiving the props by value, it's maintaining the reference very well (it does call $.extend on options, but it doesn't do it recursively, it merely copies them over by key from one object to another, possibly overwriting a default but maintaining the reference)

I changed plugin in the follow-up to do handle it inside the plugin. This got rid of a fair amount of code duplication as in most (if not all) cases all the callback did was updating the props object. Nice!

+.mw-gadgetmanager-propcontainer {}
[...]
+.mw-gadgetmanager-prop-label {}

???

Still a work in progress. Already used in my local copy, accidentally left it in here while I was separating the local copy up into two parts so that I could do this commit first, before continuing.

						<legend>Module properties</legend>\

Hardcoded English. Isn't this jquery.localize-able?

The interface is still changing, so to avoid having to change 3 or 4 files all the time, left it hardcoded until a somewhat final decision is made. Next phase will take care of this. Thanks for the reminder (iirc there are some other untranslated parts as well)

+				$definitionTitle = Title::newFromText( $gadget->getName() . '.js', NS_GADGET_DEFINITION );

Aha. Now that I re-read the documentation for Title::newFromText it makes sense. The second argument isn't "namespace" but "defaultNamespace", not obvious imho since it's a pretty generic function name. Anway, what I want here is makeTitleSafe indeed. Fixed.