I think we said we were going to wait until next sprint (starting middle of the 8th).

@Mattflaschen I've recently updated the documentation and re-enabled the gadget by default on testwiki. Please let me know if you have questions, or need anything else from me to complete this task!

Okay, it's hard to code-review a MediaWiki page, since I can't put inline notes. (Some gadgets (especially ones this big) are hosted elsewhere, such as Gerrit or GitHub, for that reason, among others)

I've ignored coding style (quotation marks, spacing, number of vars, stuff like that).

There's a lot of of stuff I could have flagged in the review, but didn't. I tried to mainly flag the most important or easy to fix items.

Here goes:

• IMPORTANT: textHolder.html(text); is a security vulnerability. Use textHolder.text(text);. Note that this requires it actually be text. Same with textHolder.html(dict['string'])[0] and $('<option>').attr('value',values[elem]).html(values[elem]), $('.formsGadget .ui-dialog-title').html (use .text).

• FAIRLY IMPORTANT: Don't ask for a page to be parsed, just to get the wikitext (under "Getting the infobox"). It could (depending on caching) make the server do a lot of work for no reason. Instead, use the 'query' action and the 'revisions' property: https://en.wikipedia.org/w/api.php?action=query&prop=revisions&titles=Earth&rvprop=content&indexpageids

• Don't use importStylesheet. List the CSS file in the gadget definition. See https://en.wikipedia.org/wiki/MediaWiki:Gadgets-definition for examples (just search on css and look for one that also has js). It basically just works though (just list both files).

• Your that property ('that' : this) is never used.

• There are unused variables some places, e.g. in createDialog.

• Not a big deal (especially for ID selectors), but when there is something like:

		if($('#formsDialogExpand').length){
			this.dialog = $('#formsDialogExpand').dialog(dialogDict);
		}

it's better for performance to store the value $() returns, so it's not necessary to call it twice.

• There's some dead code, such as getPageTitle.

• "To remove extra spaces & cleanup the comment string" should clarify "cleanup the comment string" includes/means "remove the signature code".

• setPostEditFeedbackCookie and checkPostEditFeedbackCookie are misleadingly named/implemented. The functions themselves don't know anything about post-edit cookies. (Also, to stop worrying about paths and things like that, use mw.cookie). checkPostEditFeedbackCookie also doesn't document that it clears the cookie.

• If you want me to suggest some ways to do clearer asynchronous programming, let me know.

• checkbox.type = type; (and the code that checks if the checkbox is a numeric input) is very confusing to me since the name checkbox implies the type should always be 'checkbox'. I would choose another name if it could be a different input type.

• In CSS, visibility: hidden and display: none are different, so it's confusing that the first is mapped to the second (if(dict['visibility'] == 'hidden'){).

• 'textbox' is not an input type ('text' is, but it's also the default). (input.setAttribute('type','textbox');).

• Instead of data-character-length, consider maxlength (browser-enforced limit to how many characters the user can type).

• URLs starting with // shouldn't cause issues, so there's no need to prefix with https: (callback(element,'https:'+src);).

• select.setAttribute('type','textbox'); and var description = document.createTextNode(text); should be removed, since they are unneeded.

• Dead code:

			$.when(gettingInfobox).then(function(){
			});

• createWikiPage could potentially use the infobox-generation logic (stringifyInfobox and friends)

• Constructors should be capitalized, and instances should be lower-case. Instead of:

				var Node = new node(null,null,id);
				nodeList[id] = Node;

do the opposite:

				var node = new Node(null,null,id);
				nodeList[id] = node;

Note that requires changing the function name to match. Same with tree (should be Tree).

Instead of:

mw.loader.using( ['jquery.ui.dialog', 'mediawiki.api', 'mediawiki.ui','jquery.chosen'], function() {

specify these dependencies at Gadgets-definition (again, see https://en.wikipedia.org/wiki/MediaWiki:Gadgets-definition for examples).

The code is also updated on github, https://github.com/WMFGrants/formsGadget/tree/enWiki. @Mattflaschen shall we do another round of review after the above have been fixed?

Thanks, Matt and Jeph. @Jeph_paul: could you address the following issues first, and let me know when they're fixed?

1. textHolder to .text
2. use query:revisions to parse page text
3. remove mw.loader.using(DEPENDENCIES) -- I will specify these at Gadgets-definition
4. remove importStylesheet -- I will specify these at Gadgets-definition

From the look of it, I don't think we'll need a second round of code review (and I will validate that the code works after you make your fixes). But let me know if you disagree, @Mattflaschen. Also, I apologize for not putting the link to the Git repo in the task description--it's linked from the documentation, but I forgot to include it here--I apologize for the inconvenience!

• Jonathan

Fixed all of the issues flagged by Mattflaschen except the ones mentioned below.

1. In CSS, visibility: hidden and display: none are different, so it's confusing ....
   • I assume most of the time the config will be created/edited by users who don't know/understand css. 'hidden' makes more sense to such a user than say 'display:none' or some such string. A person who knows css will find it doubly confusing though. I have left it as 'hidden', what do you guys think?
2. createWikiPage could potentially ....
   • Haven't yet generalized it.

@Mattflaschen if you have the time we could do another round & fix whatever else we have to or I have missed.
@Capt_Swing fixed all the four you have mentioned above. 3,4 have not been moved to https://test.wikipedia.org/wiki/User:Jeph_paul/formsGadget.js as I needed them to test my changes. https://github.com/WMFGrants/formsGadget/tree/enWiki has all the changes.

In T94595#1212469, @Capt_Swing wrote:

From the look of it, I don't think we'll need a second round of code review (and I will validate that the code works after you make your fixes). But let me know if you disagree, @Mattflaschen.

No, that's fine with me.

In T94595#1218493, @Jephpaul wrote:

Fixed all of the issues flagged by Mattflaschen except the ones mentioned below.

Thanks. :)

1. In CSS, visibility: hidden and display: none are different, so it's confusing ....
   • I assume most of the time the config will be created/edited by users who don't know/understand css. 'hidden' makes more sense to such a user than say 'display:none' or some such string. A person who knows css will find it doubly confusing though. I have left it as 'hidden', what do you guys think?

I think display: none would probably be better. In my experience, things like this that are intended for non-technical users usually end up configured by technical or semi-technical users. They'll be able to cope with display: none even if initially unfamiliar with CSS.