This is great work! But since I just took on my code-review-glasses,
here comes a bunch of random comments:
1. The test result from p[lang!=is-IS] has been modified. The
MochiKit.Selector implementation was recently fixed just here,
changing the semantics of all attribute operators to imply attribute
existance. One can argue either way, I guess, but I kind of liked the
existing semantics here.
2. From your speed comparison page for the various framework
implementations, I noded a number of issues in IE 6 (have not tested
IE 7 yet). Sizzle returns errors in 4 of the tests there. In the
README it also says: "It's a work in progress! Not ready for use yet!"
3. Also on the speed page, I noted that the trunk MochiKit.Selector is
reported to throw errors on a bunch of tests. And there are also a few
places where it returns a different number of results than the others.
This is problematic, since it implies that the exising
MochiKit.Selector is not only slow but also incorrect. I have no great
desire for fixing more issues there, but neither am I a fan of pushing
new (and also not finished) stuff immediately out into a release.
Finally, I'm really trying to push for a release real-soon-now (see my
other mails on the list)...
4. Have you investigated all the cases where the frameworks differ in
the number of returned elements? Just so that we can be reasonably
sure that the new implementation is indeed correct. Also... I seem to
get 1 result for nearly all tests on MochiKit.Selector w/ Sizzle when
running in Safari 3. Could it be that querySelectorAll returns a
NodeList, not an Array?
5. I agree that the global namespace pollution is an issue. If only
the Sizzle namespace was used in the code, we could easily refactor it
to use MochiKit.Selector.Sizzle or similar to further avoid any
issues.
6. There seems to be an unnecessary whitespace change in MochiKit/MochiKit.js.
7. The MochiKit.Selector.findDocElements function is not is the
current API docs. So why do we even export it? Or is that omission by
mistake? (This is a trunk issue)
8. The Sizzle result cache seems to break if people start manipulating
the returned arrays (using shift, pop or similar). Basically this:
return cache && doCache ?
(cache[ selector ] = results) :
results;
should become:
if (cache && doCache) {
cache[selector] = results.slice(0);
}
return results;
9. Note that when testing $$ in Safari, you are really only using the
built-in support for document.querySelectorAll (unless that function
throws an error). So adding or modifying functionality requires that
the query is checked before using document.querySelectorAll.
10. I'm not afraid of regexps, but the "chunker" one in the code is
just ridiculous. It is much too dense to be in any kind of production
code without proper comments and/or explanations as to its function
and use.
11. This line (118):
if ( parts.length > 0 && !checkSet ) {
should be just:
if ( parts.length > 0) {
it seems.
... and now I've got to take a break. This email is already quite
long. I'll continue looking at the code some other day.
Cheers,
/Per
PS. I just discovered that Google Groups silently dropped all my
emails that used another sender address, so I'm currently resending
all my recent postings. Hence the sudden email bombing...
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"MochiKit" group.
To post to this group, send email to mochikit@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/mochikit?hl=en
-~----------~----~----~----~------~----~------~--~---
