Cleaning of strict warnings

[Alfonso Martínez de Lizarrondo]

In my development branch I've tried to "fix" many of the strict warnings because if everything is ok, then it's easier to spot any new trouble.

many where due to this kind of code:

   if ( oToolbarSet = eToolbarTarget.__FCKToolbarSet ) 

or

   while ( el = el.parentNode )

in some cases I've changed the code so it's more explicit because it wasn't really clear what it was doing:

   oToolbarSet = eToolbarTarget.__FCKToolbarSet ;
   if ( oToolbarSet ) 

for the whiles, I've wrapped most of them in an extra pair of () because it's clear that they are harmless.

It raised several warnings with the code to read the attributes of an embed so I've changed to a cleaner and more extensible code:

	var aAttributes = [ 'scale', 'play', 'loop', 'menu', 'wmode', 'quality' ] ;
	for ( var iAtt = 0 ; i < aAttributes.length ; i++ )
	{
		var oAtt = oEmbed.getAttribute( aAttributes[iAtt] ) ;
		if ( oAtt ) oCloned.setAttribute( aAttributes[iAtt], oAtt ) ;
	}

In several places I've made sure that the function always return a value (usually null or false).

in fcktablehandler.js I've made several changes as it seemed that the file had a mix of line-endings and wasn't following the same style that other files and fixed the minor issues that brought me there.

in fckdialog.html line 156 there was a reference to oTabs.length, but oTabs is an object, not an array so it doesn't have a length property.

in fcktoolbarbuttonui.js line 145 there was an "assignment to undeclared variable bEnableEvents". That bEnableEvents variable isn't defined and used elsewhere in the _source files

in fckpanel.js I've checked that the functions this.OnHide and this.OnShow does exist before calling the FCKTools.RunFunction

In the file browser I've added code so it shows an alert if it can't load some xml file as XML (even if the responseStatus is 200)

There are still more strict warnings, but now each warning should be checked to understand what does it means.

[Alfonso Martínez de Lizarrondo]

I've finished with most of the cleaning and now there's something interesting, while loading this warning appears:

reference to undefined property FCK.EditorWindow
http://localhost/TRAC/editor/_source/internals/fck_contextmenu.js
Line: 21

So the initialization of the context menu isn't done right. It doesn't fail but the EditorWindow still doesn't exist.

I'm not gonna touch the code, but it could be interesting to see if it's possible to move the context menu initialization (creation of the panel) done only the first time the user tries to launch a context menu.

[Frederico Caldeira Knabben]

I'm ok with almost all changes. I'm just curious with the FCKTools.RunFunction checks. Did you have any problem with it? Did you have specific warnings?

I'm asking just because the RunFunction automatically takes care of the existence of the function before calling it, so now we have a unnecessary check.

Also, do you have any info about the provenance of the strict warnings definitions? Does it come from a standard or specifications doc, or from the plugin author's head? I'm curious.

Thanks again.

[Frederico Caldeira Knabben]

I've just committed a fix for the FCK.EditorWindow to the trunk. Actually, we can completely avoid using that reference there.

[Alfonso Martínez de Lizarrondo]

The strict warnings are generated by the js engine itself, here's an explanation about it: ​http://www.javascriptkit.com/javatutors/serror.shtml

They are useful to avoid little mistakes, and there is also the javascript lint tool that analyzes a full javascript file in order to find code that doesn't look right (although it's too strong in its checks so it isn't too pleasant) ​http://www.javascriptlint.com/

I'll remove the checks before the FCKTools.RunFunction calls, they aren't errors, just a warning that I was using a property that didn't exist (like the one about FCK.EditorWindow in the initialization of the context menu)

[Alfonso Martínez de Lizarrondo]

merged most of the fixes in revision 61 There are still a few warnings and the IE files haven't been reviewed, but I don't feel right now to completely review all of this.

Maybe we could look to integrate the javascript lint into the automated tests (with the proper warning level)

[Frederico Caldeira Knabben]

Thanks for the info about lint.

The idea of having some automatic check with JavaScript Lint is great. I don't believe we are able to do that in the automated test system, but we could at least create a batch or dedicated application to have some report generation so we can easily control all files in the package in a single action. I've created Ticket #43 to not forget it.

[Alfonso Martínez de Lizarrondo]

So I'll close this one and any other improvement can be done in ticket #43