Hi Per,
On Wed, Oct 8, 2008 at 15:49, Per Cederberg <[EMAIL PROTECTED]> wrote:
> 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.
Ah, ok. I just thought the test was buggy. != is not actually defined
by CSS3, so I went by the jQuery definition which is this:
"Matches elements that either don't have the specified attribute or do
have the specified attribute but not with a certain value."
I.e. it explicitly says it matches elements w/o that attribute. Of
course this is no standard, if you think we should use the other
semantics I don't mind.
> 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!"
Yes, I'm actually not sure what the status on Sizzle development is -
but John has at least not updated the github version since posting it
originally. Unfortunately I don't have access to IE6 or time to set up
a virtual machine - but if you try debugging it I'd be happy to help
with the little insight I gained last night.
> 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)...
Yes. If the benchmark tests are to be trusted, that simply means that
the trunk version (which should behave like Prototype did, although
they may have changed the selector engine in Prototype now) is
incorrect. I'll look into what tests are failing and what the
standards have to say about them. If it really is incorrect, all the
more reason to move to a new implementation.
I would also vote against pushing this to a release as it stands now.
> 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?
I will have a look at the tests and confirm what is the correct
outcome. Safari was also failing on several unit tests for me, I will
also look at that tonight.
> 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.
Ok, I'll start work on that too. I think I would move everything
inside Sizzle (and submit a patch to John) and then move Sizzle into
MochiKit.Selector. It should be easy, just a little legwork.
> 6. There seems to be an unnecessary whitespace change in MochiKit/MochiKit.js.
There are? The only change I can see is the reording of submodules -
since Selector now depends on Style I had to switch their order. Or do
you mean packed/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)
That is probably a mistake, it should definitely be in the docs. I
will fix this in the trunk.
> 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;
Ok, I'll have a look. Otherwise this is a comment for John I guess.
> 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.
Right, I'm not sure what is best to do here. Should we rely on
querySelectorAll where it is available - simply to have the best
performance, or is identical semantics on all browsers more important?
For the latter there are two ways, don't use querySelectorAll (and
take the performance penalty) or strip out the features not supported
by it (and take the feature penalty).
> 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.
This is another comment for John I guess. I don't necessarily agree
with you though, its function was relatively clear after pasting it
into http://regexpal.com/ and playing with it for a minute.
> 11. This line (118):
>
> if ( parts.length > 0 && !checkSet ) {
>
> should be just:
>
> if ( parts.length > 0) {
>
> it seems.
Right, there are several "bugs" of this kind in Sizzle already.
> ... 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.
Thank you very much for excellent, detailed comments. Reviewing is
just as hard as coding, but more boring :)
cheers,
Arnar
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"MochiKit" group.
To post to this group, send email to [email protected]
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
-~----------~----~----~----~------~----~------~--~---
