[webkit-reviews] review granted: [Bug 53705] Viewport parsing no longer accepts "1.0; " value as valid. : [Attachment 83228] [PATCH] Explicitly Handle String Bounds Checking

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 53705: Viewport parsing no longer accepts "1.0;" value as valid.
https://bugs.webkit.org/show_bug.cgi?id=53705

Attachment 83228: [PATCH] Explicitly Handle String Bounds Checking
https://bugs.webkit.org/attachment.cgi?id=83228&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83228&action=review

r=me, but consider removing the unused default statement, and making toFloat()
accept an "ignore trailing garbage" flag (if that's even possible or easily
done).

> Source/WebCore/dom/ViewportArguments.cpp:192
> +	   if (!length || !isASCIIDigit(valueString[0]) || (length > 1 &&
valueString[0] == '-' && !isASCIIDigit(valueString[1]))) {

Should we allow a unary "+" operator as well?

What about a decimal number without a leading zero?  ".5"?  Or "+.5"?  Let's
not go crazy here, but I want you to consider all possibilities.

It's too bad the toFloat() method doesn't have a flag to say "ignore trailing
garbage".  That way, if you sent "pure" garbage to it, you'd still end up with
a value of "0.0f" (which is what you want in this case; or NaN which you could
convert to 0.0f here), otherwise the method tries its best to convert whatever
numeric string is passed to it (ignoring trailing garbage).  Then you don't
have to reparse the string locally to guess if it started with something
numeric.

Otherwise, I think it would be cleaner if this code was pulled out into a
static inline method with a descriptive name.  It's a bit hard to read on one
line.

> Source/WebCore/dom/ViewportArguments.cpp:365
> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   return ErrorMessageLevel;

If the switch statement has case statements for every ViewportErrorCode enum,
then you don't need a "default" case because a missing enum will cause a
compiler warning (and thus an error with -Werror set).