I've done jQuery plug-ins before and an important step in these (especially if you want to compress or minimize them) is to run them thru jslint (jslint.com). This finds a lot of potential JS bugs like missing semi's, closure errors, etc. It also forces some "good" stylistic stuff like wrapping all if / else in {}'s.

IMHO, this might help fix some problems due to missing semi's or other malformed JS. It will also help prevent future bugs and possibly make it a little more browser independent. (Adding the missing semi's will make EclipsePhP happy too...)

Anyway, I've run webfm.js thru jslint until I corrected all the problems. Attached is a patch that updates the current CVS head file to this "lint free" version.

It looks like it changes a lot,but it really doesn't do many code changes, most of the changes are adding missing semi's, wrapping if/else stuff in curly braces, removing extraneous var statements, and adding a radix to parseInt functions.

A couple of functions were moved around in the file to make them defined before use but not changed.

The biggest "changes" where the spots that JSLint identified closure issues. These are places that created functions (for listeners generally) inside loops. Some of these functions used local variables that might be cleaned up by garbage collection. I've replaced the inline code with Static functions that return the required listener functions.

Also, some switch statements were flagged as better done as if statements and convert.

One item I'm not sure of was in the Webfm.popup.prototype.objPosition function. There was a while loop that had an assignment ( x = y ) instead of a logical test (x == y ). I converted this to the logical.

I've done some testing with this, but it would be nice if others could try this out and verify that no problems slipped in.

CommentFileSizeAuthor
#1 webfm.zip28.63 KBcgmonroe
webfm-jslint-free.patch112.9 KBcgmonroe

Comments

cgmonroe’s picture

cgmonroe commented
StatusFileSize
new webfm.zip28.63 KB

Oh, here's the fully patched webfm.js file so you don't have to patch it unless you want to.... just replace your webfm.js file with this one (after making a backup).

Note that this is from the CVS head but should work fine with the -dev version... and possibly with the production version.

nhck’s picture

nhck commented
Status: Needs review » Needs work

Thank you for helping to make webfm better again cgmonroe. A good idea - I like it a lot.

However after applying the patch (and I compared the patched file to your *zip-file) webfm ceased working completely for me.

This is probably due to converting "== null" to "=== null", for example in this case:

Webfm.hht.prototype.put = function(key, obj) {
	if((key === null) || (obj === null)) {
		throw "NullPointerException {" + key + "},{" + obj + "}";
	}

	if(this.hash[key] === null) {
		this.keys[this.keys.length] = key;
		this.hash[key] = [];
  }

	// append new object to array
	this.hash[key].push(obj);
};

The browsers I tested decided that this.hash[key] is not === null if this.hash[key] === undefined.

I appreciate your effort a lot and think that you are correct about putting more work into this. I'll try to fix the issues and provide an other patch for you to test.

Thank you!

cgmonroe’s picture

cgmonroe commented

Sigh, I knew I should have tested more... but I'm not surprised. I'm thinking the best compromise here is to just fix missing semi's and make sure all if statements are wrapped in curly braces. This should clean up / prevent a few problems but not effect the current code at all.

Eventually, we will need to figure out the issues with logicals.. but I think that should be done with a full re-write to convert to full jQuery use. IMHO, this would clear up a lot of the code, make it more browser independent (because jQuery does a good job of this), and open the js up to mods from the large jQuery savvy population. This is probably a V3.0 goal and not an immediate one.

nhck’s picture

nhck commented

I realized parts of this request, cgmonroe - its in cvs to be tested.

Still to do would be passing jslint with also those options set:

  • Tolerate unfiltered for in
  • Disallow == and !=
  • Require "use strict";

Right now I have:

/*jslint white: true, browser: true, devel: true, windows: true, forin: true, onevar: true, undef: true, nomen: true, plusplus: true, regexp: true, newcap: true, immed: true */

with the following predefined:

Drupal, window, $, Components, clipboardData, netscape, getModUrl, getWebfmIconDir,getWebfmIETreeOffset, getWebfmDateFormat, getWebfmIEListOffset, getWebfmOwnerColumn,getWebfmCleanUrl, getWebfmUid, getBaseUrl
robmilne’s picture

robmilne commented
Status: Needs work » Closed (fixed)

The javascript was created before the coding standards were set. I agree that lint processing is a good idea but I do not approve of rearranging the whitespace such that it doesn't compare properly to the way I wrote it.

I've restored the whitespace to my preference after running the following jslint options:

/*jslint browser: true, devel: true, windows: true, forin: true, nomen: true, regexp: true, newcap: true, immed: true */

Offline I've agreed that the undef option is a good idea - I'll check it next time it passes through jslint.