WebKit Javascript Woes
September 23, 2006 1:26 PM Subscribe
I'm having a strange problem with WebKit's Javascript engine. Please help. Copious explanation inside.
I have a live inline comment preview feature, much like MetaFilter's, on my blog. It works great in Opera and Firefox (and MSIE). However, it doesn't seem to work in Safari, OmniWeb, or even the WebKit nightly build-I downloaded it in the hope that this was a WebKit bug that's fixed upstream in the nightlies, but if it is their bug, it doesn't look like it's been fixed. I'm asking this question in the hope that it's something I can fix.
Here's how my comment preview code works:
1. There's a textarea with the id "comment-text" which you type your comment into. It has an onKeyUp event handler that calls doCommentPreview() in my javascript file. There's a preview field underneath the textarea with the id "comment-preview", which doCommentPreview() fills with the preview HTML.
2. I have a blacklist of disallowed words, as a spam prevention measure. The comment engine will refuse any comment that contains any of these words or phrases.
3. At the top of the document (not in the javascript file), I fill a javascript array called spams[] with the contents of the blacklist, using php, on the server side.
4. I have two String prototype functions: showSpams() and regExEscape(). I use showSpams() to highlight items in the blacklist that a commenter might have typed in, to alert them right in the inline previewer that their comment is likely to be rejected before they hit post. It just tests the comment against each element of the spam[]array, and if the comments contains that particular word, it turns red and is surrounded by a border.
5. I've enabled the Debug menu using
The line that WebKit complains about is the first line inside the for-loop in String.prototype.showSpams():
6-a. At the top of the document, in the <head> section:
In the textarea used for writing comments, I have this:
What's going on here?
I have a live inline comment preview feature, much like MetaFilter's, on my blog. It works great in Opera and Firefox (and MSIE). However, it doesn't seem to work in Safari, OmniWeb, or even the WebKit nightly build-I downloaded it in the hope that this was a WebKit bug that's fixed upstream in the nightlies, but if it is their bug, it doesn't look like it's been fixed. I'm asking this question in the hope that it's something I can fix.
Here's how my comment preview code works:
1. There's a textarea with the id "comment-text" which you type your comment into. It has an onKeyUp event handler that calls doCommentPreview() in my javascript file. There's a preview field underneath the textarea with the id "comment-preview", which doCommentPreview() fills with the preview HTML.
2. I have a blacklist of disallowed words, as a spam prevention measure. The comment engine will refuse any comment that contains any of these words or phrases.
3. At the top of the document (not in the javascript file), I fill a javascript array called spams[] with the contents of the blacklist, using php, on the server side.
4. I have two String prototype functions: showSpams() and regExEscape(). I use showSpams() to highlight items in the blacklist that a commenter might have typed in, to alert them right in the inline previewer that their comment is likely to be rejected before they hit post. It just tests the comment against each element of the spam[]array, and if the comments contains that particular word, it turns red and is surrounded by a border.
5. I've enabled the Debug menu using
defaults write com.apple.Safari EnableDebugMenu 1Safari's Javascript console, in the debug menu, shows the following error:
Value undefined (result of expression re.compile) is not object. bloggie_05.json line 16.
The line that WebKit complains about is the first line inside the for-loop in String.prototype.showSpams():
re.compile(spams[i].regExEscape(),'gim');6. Here's all the code, in context:
6-a. At the top of the document, in the <head> section:
<script language="javascript">var spams = new Array();spams[0] = 'casino';spams[1] = 'craps';spams[2] = 'backgammon';spams[3] = 'cash advance';</script> <script src="bloggie_05.js" language="javascript" type="text/javascript"></script>[I abbreviated the blacklist (the spams[] array) because this question is already way too long.]
In the textarea used for writing comments, I have this:
<textarea tabindex="1" id="comment-text" name="comment_text_body" style='width:95%;height:300px;' onKeyUp="javascript:doCommentPreview();"></textarea>And here's the empty div underneath it, which is filled by the doCommentPreview() function above:
<div class="ientrycomment_body" id="comment-preview" name="prevu"></div>Finally, here's the code in the external (bloggie_05.js) javascript file:
function doCommentPreview(doc){
doc=(typeof(doc)=="undefined"?document:doc);
field=doc.getElementById("comment-text");
preview_field=doc.getElementById("comment-preview");
preview_field.innerHTML = "<p>" + field.value.split(/\n\n/).join("</p><p>").split(/\n/).join("<br />").showSpams() + "</p>";
}
String.prototype.showSpams = function(){
despammed=this;
re = new RegExp();
for (var i=0; i < spams.length; i++) {
re.compile(spams[i].regExEscape(),'gim');
despammed=despammed.replace(re,"<span style=\"color:red;border:1px dotted;\" title=\"you can't say that here!\">" + spams[i] + "</span>");
}
return despammed;
}
String.prototype.regExEscape = function() {
text=this;
if (!arguments.callee.sRE) {
var specials = [ '/', '.', '*', '+', '?', '|', '(', ')', '[', ']', '{', '}', '\\' ];
arguments.callee.sRE = new RegExp(
'(\\' + specials.join('|\\') + ')', 'g'
);
}
return text.replace(arguments.callee.sRE, '\\$1');
}The line that WebKit has a problem with is in bold, in the String.prototype.showSpams() above.What's going on here?
Response by poster: MonkeySaltedNuts-For performance reasons. I don't want to create a new RexExp object for each of the some-80ish blacklisted terms in a loop. Reusing the same RegExp instance seemed wiser. OTOH, maybe that was a premature optimization, since I didn't actually check to see if it was causing performance problems, I just assumed it would and factored it out of the loop.
compile isn't a standard method of RegExp objects? Damn...I guess that's my problem then. Too bad, it worked in every browser but WebKit ones.
I'll try it without re.compile.
posted by evariste at 2:25 PM on September 23, 2006
compile isn't a standard method of RegExp objects? Damn...I guess that's my problem then. Too bad, it worked in every browser but WebKit ones.
I'll try it without re.compile.
posted by evariste at 2:25 PM on September 23, 2006
Response by poster: Is there anywhere out there that has a Javascript compatibility chart, like the CSS browser compatibility charts that show which browsers can do what?
posted by evariste at 2:29 PM on September 23, 2006
posted by evariste at 2:29 PM on September 23, 2006
Best answer: check to see if safari supports the compile method with a try/catch, and if not
posted by Señor Pantalones at 2:32 PM on September 23, 2006
try {
re.compile(spams[i].regExEscape(),'gim');
}
catch {
alert('guess we found the problem');
// write a longer method of doing the same thing without use of the compile method
}
or just detect
if (!re.compile) alert('this browser doesn't support compile');posted by Señor Pantalones at 2:32 PM on September 23, 2006
Response by poster: That was it, Safari doesn't support the compile method. Thanks, Monkey and Señor!
posted by evariste at 2:38 PM on September 23, 2006
posted by evariste at 2:38 PM on September 23, 2006
Best answer: Javascript code (for pages) rarely needs to be optimized.
If you really need to optimize I would pre-compute the REs for 'spams' into 'spamREs' rather than re-compute them everytime you invoke .showSpams on a string.
posted by MonkeySaltedNuts at 2:56 PM on September 23, 2006
If you really need to optimize I would pre-compute the REs for 'spams' into 'spamREs' rather than re-compute them everytime you invoke .showSpams on a string.
posted by MonkeySaltedNuts at 2:56 PM on September 23, 2006
Response by poster: If you really need to optimize I would pre-compute the REs for 'spams' into 'spamREs' rather than re-compute them everytime you invoke .showSpams on a string.
Duh.
*smacks forehead*
As it turns out, there's no need to optimize, so I'm not going to worry about it for now. Thanks again.
posted by evariste at 3:04 PM on September 23, 2006
Duh.
*smacks forehead*
As it turns out, there's no need to optimize, so I'm not going to worry about it for now. Thanks again.
posted by evariste at 3:04 PM on September 23, 2006
Best answer: Hi, I'm a Safari developer and I'd love it if you could file this bug at http://bugzilla.opendarwin.org/ in product WebKit. Even if "compile" is not a standard RegExp method, we prefer to support common extensions of other browsers.
posted by maciej at 4:04 PM on September 23, 2006 [1 favorite]
posted by maciej at 4:04 PM on September 23, 2006 [1 favorite]
Best answer: Javascript code (for pages) rarely needs to be optimized.
This is absolutely not true. Since JavaScript is not compiled, there are dozens and dozens of optimization routines that can speed up script execution. Just for example, take this loop in the code you quote:
You're calculating the length of the
When you have large datasets that you need to perform operations on, like looping through a allowed-words filter, for instance, there are a number of real-world performance benefits to these relatively simple optimizations.
That's just a simple, small example. There are much larger issues looming in your routine. For example, this is abhorrant:
Why are you calculating the entire field value for every single keystroke? You could have a function in between your expression tester that checks whether a space or enter has been hit, and only then run the expression tester.
posted by Civil_Disobedient at 4:24 PM on September 23, 2006 [2 favorites]
This is absolutely not true. Since JavaScript is not compiled, there are dozens and dozens of optimization routines that can speed up script execution. Just for example, take this loop in the code you quote:
for (var i=0; i < spams.length; i++) {br>
...
You're calculating the length of the
spams array every time you iterate through the loop. You could optimize it by declaring a variable in the for...loop that holds the value of the length as such:
for (i=0, j=spams.length; i < j; i++) {
...
When you have large datasets that you need to perform operations on, like looping through a allowed-words filter, for instance, there are a number of real-world performance benefits to these relatively simple optimizations.
That's just a simple, small example. There are much larger issues looming in your routine. For example, this is abhorrant:
preview_field.innerHTML = "<p>" + field.value.split(/\n\n/).join("</p><p>").split(/\n/).join("<br />").showSpams() + "</p>";
Why are you calculating the entire field value for every single keystroke? You could have a function in between your expression tester that checks whether a space or enter has been hit, and only then run the expression tester.
posted by Civil_Disobedient at 4:24 PM on September 23, 2006 [2 favorites]
oh awesome, maciej is part of the hive mind! now fix my filed bugs! ;) (j/k).
civil_disobedient: crapping in the ice cream machine at a Dairy Queen is "abhorrent." poorly optimized javascript is "poorly optimized" :)
posted by Señor Pantalones at 4:40 PM on September 23, 2006
civil_disobedient: crapping in the ice cream machine at a Dairy Queen is "abhorrent." poorly optimized javascript is "poorly optimized" :)
posted by Señor Pantalones at 4:40 PM on September 23, 2006
Response by poster: maciej-great, I'll do that!
Civil_Disobedient: thanks, I've actually thought about that (only checking for blacklisted expressions on word boundary keystrokes) and decided not to do it, for now. Since there isn't any perceptible lag between a keystroke in the comment field and its mirror image showing up in the preview field, I'm not too concerned about it.
posted by evariste at 4:46 PM on September 23, 2006
Civil_Disobedient: thanks, I've actually thought about that (only checking for blacklisted expressions on word boundary keystrokes) and decided not to do it, for now. Since there isn't any perceptible lag between a keystroke in the comment field and its mirror image showing up in the preview field, I'm not too concerned about it.
posted by evariste at 4:46 PM on September 23, 2006
Response by poster: Filed bug #11001 against WebKit, thanks again maciej.
posted by evariste at 4:58 PM on September 23, 2006
posted by evariste at 4:58 PM on September 23, 2006
#Civil_Disobedient:
(ps spams.length does not 'calculate' the length, it accesses the pre-calculated length, and the issue of when optimization is really needed has nothing to do with the interpreted/compiled distinctions)
posted by MonkeySaltedNuts at 5:45 PM on September 23, 2006
Javascript code (for pages) rarely needs to be optimized.Are you insane? If a page takes .1 seconds to load and render, only the peversely obsessive will optimize associated code to save .0001 seconds and make it more opaque.
This is absolutely not true. Since JavaScript is not compiled, there are dozens and dozens of optimization routines that can speed up script execution. Just for example, take this loop in the code you quote:
for (var i=0; i < spams.length; i++) {br>
...
You're calculating the length of thespamsarray every time you iterate through the loop. You could optimize it by declaring a variable in the for...loop that holds the value of the length as such:
for (i=0, j=spams.length; i < j; i++) {
...
(ps spams.length does not 'calculate' the length, it accesses the pre-calculated length, and the issue of when optimization is really needed has nothing to do with the interpreted/compiled distinctions)
posted by MonkeySaltedNuts at 5:45 PM on September 23, 2006
Best answer: ps spams.length does not 'calculate' the length, it accesses the pre-calculated length
No, you're right, it doesn't calculate the length, but it has to access an object property. The less property accessing that is done off of objects (or objects of objects of objects), the better. For example:
Will benefit from declaring the nested object outside the loop, like this:
Are you insane? If a page takes .1 seconds to load and render, only the peversely obsessive will optimize associated code to save .0001 seconds and make it more opaque.
I was merely giving but one example to illustrate a point. Iterating a nested object's value that never changes is a performance hit. Not a big one, naturally. And actually, a better way to do it would be something like this:
This will be roughly twice as fast as the original version. If it's the only optimization you're making, well sure, saving .001 second is pretty pointless. But as your codebase gets bigger, these optimizations can make a palpable difference.
As for keeping it more opaque, you'll note that I made no mention of Duff Devices or the like. I agree that there's a point when optimization negatively affects maintainability, but there are plenty of techniques that won't destroy code readability.
posted by Civil_Disobedient at 8:40 PM on September 23, 2006 [1 favorite]
No, you're right, it doesn't calculate the length, but it has to access an object property. The less property accessing that is done off of objects (or objects of objects of objects), the better. For example:
for (x=0; x < 100; x++) {
yo = document.forms[0].elements[1].value * x;
Will benefit from declaring the nested object outside the loop, like this:
formValue = document.forms[0].elements[1].value;
for (x=0; x < 100; x++) {
yo = formValue * x;
...
Are you insane? If a page takes .1 seconds to load and render, only the peversely obsessive will optimize associated code to save .0001 seconds and make it more opaque.
I was merely giving but one example to illustrate a point. Iterating a nested object's value that never changes is a performance hit. Not a big one, naturally. And actually, a better way to do it would be something like this:
for (i=spams.length; i > 0; i--) {
...
This will be roughly twice as fast as the original version. If it's the only optimization you're making, well sure, saving .001 second is pretty pointless. But as your codebase gets bigger, these optimizations can make a palpable difference.
As for keeping it more opaque, you'll note that I made no mention of Duff Devices or the like. I agree that there's a point when optimization negatively affects maintainability, but there are plenty of techniques that won't destroy code readability.
posted by Civil_Disobedient at 8:40 PM on September 23, 2006 [1 favorite]
« Older Snow Plowing with an Automatic Transmission Truck? | Jewelry like Niwaka in San Francisco? Newer »
This thread is closed to new comments.
Why don't you just pass the pattern to 'new RegExp(pat)'?
posted by MonkeySaltedNuts at 1:46 PM on September 23, 2006