[webkit-reviews] review granted: [Bug 7118] Property values with extra items do not get treated as invalid (they should) : [Attachment 7490] revised fix

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Mon Apr 3 14:59:25 PDT 2006


Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 7118: Property values with extra items do not get treated as invalid (they
should)
http://bugzilla.opendarwin.org/show_bug.cgi?id=7118

Attachment 7490: revised fix
http://bugzilla.opendarwin.org/attachment.cgi?id=7490&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks great.

+    int num = inShorthand() ? 1 : valueList->size();

It's a little strange here -- people never really want to know the size, they
just want to know if it's greater than 1, 2, or greater. But if the value list
happened to be huge we'd walk the entire list. On the one hand, I don't think
this is the only n^2 algorithm here, but on the other it would be slightly
better not to do this (here or the other places in the file where we already do
this). It's faster to check 1 vs. not-1 than it is to get num, etc.

ShorthandScope is a great idea. I'd like ShorthandScope to be private to the
.cpp file if possible. The only reason to have it in the .h file would be if we
need to share with some other source file.

+		 if (!(m_parser->m_inParseShorthand++))
+		     m_parser->m_currentShorthand = propId;

I think the way this works is strange. Should it really just leave
m_currentShorthand alone if we're already in a shorthand? It seems to me that
either we need to change this to save and restore m_currentShorthand (which is
easy now that you have the ShorthandScope object) or maybe just change
m_inParseShorthand to a boolean or something. This is sloppy the way it is
right now, although it's not your fault. You made things better, not worse,
with your patch.

I think I'll r+ even though I have these comments.



More information about the webkit-reviews mailing list