In my previous post explaining the Drupal JavaScript problem I posted a litmus test to see if anyone else noticed what was wrong. I was asked to share the answer I was looking for and some tried to answer it. Sadly, no one approached me with what I was looking for. So, here is a walk through explaining some of he problems we have in our JavaScript (all examples are from Drupal 7 Beta 2).

Drupal.checkPlain()

`Drupal.checkPlain` suffers from two problems with one of them being quite significant. The code for the function is:

Drupal.checkPlain = function (str) {
  str = String(str);
  var replace = { '&': '&amp;', '"': '&quot;', '<': '&lt;', '>': '&gt;' };
  for (var character in replace) {
    var regex = new RegExp(character, 'g');
    str = str.replace(regex, replace[character]);
  }
  return str;
};

Variables are scoped at the function level and are initiated at the top of the function definition no matter where they are defined. To the JavaScript engine the function actually starts out looking like:

Drupal.checkPlain = function (str) {
  var replace, character, regex;
  ....
};

The var in front of regex in the loop does nothing. This is an important concept to grasp but the smaller of the two issues in this function because it doesn’t open the door for damage like the other one.

JavaScript is a prototype language and all our objects inherit properties and functions from Object.prototype. This means that if a script adds something to Object.prototype all our objects inherit it. Lets look at an example.

Object.prototype.foo = "I'm littering in JavaScript."

If I add this to a page I can run the command alert(Drupal.foo); and get the response "I'm littering in JavaScript."

When I point out these are inherited it is important to note that they are inherited unless the base object has that variable or function. If Drupal had a foo property it would override the Object.prototype.foo property.

Going back to Drupal.checkPlain, if someone adds a variable to Object.prototype it will be iterated over and replaced in the for (var character in replace) loop. This loops over all of the objects own properties and any added to Object.prototype. This is not what we want. But, the solution is simple.

for (var character in replace) {
  if (replace.hasOwnProperty(character)) {
    regex = new RegExp(character, 'g');
    str = str.replace(regex, replace[character]);
  }
}

hasOwnProperty tests if a property is a native one to the object and only returns true for its own properties. Wrapping our modifications to the string in the if statement filters out any prototype properties from getting in there.

Drupal.theme()

Drupal.theme = function (func) {
  for (var i = 1, args = []; i < arguments.length; i++) {
    args.push(arguments[i]);
  }

  return (Drupal.theme[func] || Drupal.theme.prototype[func]).apply(this, args);
};

In Drupal.theme we start by getting all the arguments except the first one to pass into the JavaScript theme function. The code that does this is slow performing.

In the middle of the loop, the point where i is being evaluated, arguments.length is being calculated each time through the loop. This takes time and the value does not change. To speed up this loop we could rewrite it like:

var length = arguments.length;
for (var i = 1, args = []; i < length; i++) {
  args.push(arguments[i]);
}

To really get a speed boost we should use a native function instead iterating over a loop. For example:

var args = Array.prototype.slice.apply(arguments, [1]);

We can use the Array objects slice function to create a new array starting from the index point of 1, slicing off the first item. This is fast because it is using native code.

I know someone will tell me to submit a patch with the changes. That is a good idea and what I did.