[webkit-reviews] review requested: [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]

Alexey Proskuryakov <ap at nypop.com> has asked  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 Alexey Proskuryakov <ap at nypop.com>
> Why is there any need to check "num" in functions that call parseShorthand?
It
> seems to me that parseShorthand is already guaranteed to do the right thing;
it
> can't possibly return true if there are more values than properties.

  Indeed, but turns out that it has a slightly different bug - it returns
false, but the properties stay applied. Thanks for noticing! Added a fix (in
CSSGrammar.y) and a test.

  Also added a class to automatically match enterShortcut/exitShortcut pairs;
hopefully there's no hidden drawback in doing so.

> Could we perhaps fix the whole bug another way? Instead of looking at
> valueList->size(), could we look at valueList->current() at the end of the
> function to see if it's 0? That seems to go more with the flow of the code.

  Ok, done.

> What about the CSS_VAL_INHERIT and CSS_VAL_INITIAL cases? Don't those need
one
> of these checks?

  Fixed (a num == 1 check seemed more appropriate here).

> What about the SVG properties? Those should have the same issue, but they're
in
> a different source file.

  The fix would be similar, but there's no test case to validate it, so I was
going to open a new bug for SVG.