[Webkit-unassigned] [Bug 53705] Viewport parsing no longer accepts "1.0; " value as valid.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53705


David Kilzer (ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #83228|review?                     |review+
               Flag|                            |




--- Comment #22 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2011-02-21 21:41:39 PST ---
(From update of attachment 83228)
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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.