Seems the boss has been browsing web stats again – I came across a bug report from him saying a DHTML menu on onestat.com only works once. (Load page, click any "demo" link at the bottom and try the menu on the left.)
I'm not paying that much attention to stats myself, being too busy doing the work that will hopefully smoothen out compatibility problems and make it possible to grow our usage share. It's exactly the type of brokenness Jon found on onestat.com I fear the most – a bit hidden away, subtle, annoying when the user needs that page to work but perhaps not disastrous enough to notify us of. Too many of these issues, and we've lost a user! (Unless, of course, the user happens to be our CEO 🙂 – luckily he's an active surfer who comes across more compat problems than any average user and eagerly reports them ;)).
Now, deep inside the Onestat scripts is some code to handle browser differences. The script assumes that a browser below a certain level of DOM support can't be expected to create menus after the document loaded. That's of course not a bad assumption to make – until you see the sniffing they use to determine whether you have sufficient DOM support. Here is the relevant part:
if(kh.indexOf("Opera/7")>-1||kh.indexOf("Opera 7")>-1)return "Op7";
.
.
else if(kh.indexOf("Opera")>-1)return "Default";
Check the calendar, sir: we're in late 2009. Opera 7 – the only Opera version this script thinks is capable of opening the menu a second time – was released in January 2003. That's a 6 year old browser sniffer! I hope the script that generates their statistics is a bit more up to date!
And it's more than a little ironic that a web statistics company contributes to Opera's low usage share by running such old and broken code on their website. 😦
miscoded 
😮
:no:
So is that why the browser js list keeps growing?Great code digging
fail.
:faint:
LOL. Sure, sometimes Jon's bugs cause sitepatches 🙂
The Daily Reviewer writes:Hi!Congratulations! Your readers have submitted and voted for your blog at The Daily Reviewer. We compiled an exclusive list of the Top 100 opera Blogs, and we are glad to let you know that your blog was included! You can see it at http://thedailyreviewer.com/top/operaYou can claim your Top 100 Blogs Award here : http://thedailyreviewer.com/pages/badges/operaP.S. This is a one-time notice to let you know your blog was included in one of our Top 100 Blog categories. You might get notices if you are listed in two or more categories.P.P.S. If for some reason you want your blog removed from our list, just send an email to angelina@thedailyreviewer.com with the subject line "REMOVE" and the link to your blog in the body of the message.Cheers!Angelina MizakiSelection Committee PresidentThe Daily Reviewerhttp://thedailyreviewer.com
So, did anyone from your team contact the creators of the Onestat script?Are other coders, who have created similar bugs, informed of the fault(s) with their code?If so, when you contact those people and groups, do you offer suggestions on how the code can be improved?
Yes.
Аноним writes:hiSure, that's hard work to write patches and maintain them, but probably the architecture of browser.js should be rethought a bit.Currently there're several things that make it even harder, i think.Firstly, there's a lot of duplicate code. Secondly, why not make array / object / something more structured of all these patches? And last – there're numerous checks of location.hostname and a lot of code relies on them, but that just happens to work. Consider the following check:else if(hostname.indexOf('blogger.com') > -1)Obviously it matches not only blogger.com, but any other domain that has that substring. For example:blogger.com.plwww.yourblogger.comiblogger.comanysubdomainblogger.coma.comblogger.comp.orgqblogger.comblogger.com.uaThese sites may be broken because of browser.js' unnesessary "intervention".browser.js contains about 130 hostname.indexOf checks – so it can break much more sites that it actually fixes.
Hi anonymous, thanks for reviewing!Re-thinking browser.js architecture is probably a good idea, but only if you take into account all the constraints that led to choosing the current solution.Why browser.js coding style isn't more elegant boils down to one word: trade-offs.For example, location.hostname.indexOf() is, as you brilliantly demonstrate, not a reliable way to detect a certain site. A regular expression would be better. The trade-off is performance. Do you want to be correct, or do you want to be fast? In this case, we're dealing with a statement that Opera will run hundreds of times on every page load (including inside every IFRAME), and I've chosen to do the fastest thing (which means string.indexOf() rather than string.match() or regexp.test()). Opera's core implementation of these internals of course changes over time, and perhaps some day a regexp will be as fast or faster. I should re-test that some day..(Naturally, perhaps choosing performance over correctness was a mistake. You are perfectly right that there is a risk we'll break sites that do not need patching. However, that risk is in practise pretty small – I can remember one reported case where a patch with a sloppy indexOf check ran on the wrong site, and it didn't cause harm there AFAIK.)As for code duplication I can't comment on that unless you're more specific. I try to single out code that is required by several sites and use utility functions defined at the top of the file (such as the excellent and much-used addCssToDocument() function).Regarding the structure, we've considered various styles. Say, for example:
patches={}; patches['www.opera.com']=function(){/*fix opera.com here*/} if(patches[location.hostname])patches[location.hostname]();Or
switch(location.hostname){ case 'www.opera.com': /*fix opera.com here*/ }but this is a trade-off between being able to impose a single coding style and flexibility (and to a certain extent performance).I don't want the overhead of defining functions if it's not necessary (corollary: as the file grows, we may suffer from conflicting local variable names – again choosing to prioritise performance is a trade-off that might be the wrong decision down the road). And both a switch() and an object definition is much less flexible than a simple if() statement where you can throw in checks for other conditions (are we running on a Mac? Is the screen width less than 800px? etc..). Sure, these could be done inside a specific patch's function body, but that would be slower.