[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





--- Comment #18 from Joseph Pecoraro <joepeck at webkit.org>  2011-02-17 10:25:03 PST ---
(In reply to comment #17)
> (From update of attachment 82738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82738&action=review
> 
> > LayoutTests/ChangeLog:17
> > +        FIXME: OTHER TESTS WOULD BE AFFECTED.
> > +        fast/viewport/viewport-65.html and potentially others.
> > +        ------
> 
> Notice that the Qt port already skips a few of the tests.

Good point, I forgot to check that.


> > Source/WebCore/dom/ViewportArguments.cpp:182
> > +static float numericPrefix(const String& keyString, const String& valueString, Document* document, bool* ok)
> 
> The valueString and the ok are related to the actual parsing here, where as the keyString and the document are used for error reporting.
> 
> What about:
> 
> numericPrefix(const String& valueString, bool* ok, ViewportErrorCode* error)
> 
> We could then even add a NoError to the ViewportErrorCode if we wanted.

I see. I would prefer to perform the error reporting in this function, otherwise each of the call sites would need to handle the error code and report the error.

Also because its possible that multiple errors/tips can be reported. The truncation
tip is really just that, a tip, that is not exclusive of other errors/tips. An example
of multiple tips is the truncation tip, which produces a valid value, and the
"use keyword" tip.

  <meta name="viewport" content="width=320;">
    => TIP: "320;" was truncated to numeric prefix
    => TIP: width set to a constant equal to the physical device width, try the "device-width" keyword.

> > Source/WebCore/dom/ViewportArguments.cpp:192
> > +        if (!isASCIIDigit(firstChar) || (firstChar == '-' && !isASCIIDigit(valueString[1]))) {
> 
> What if there is only the firstChar as - and no second one?

Arg, excellent point. I get lucky here its handled in WTFString which returns 0:

    UChar operator[](unsigned index) const
    {
        if (!m_impl || index >= m_impl->length())
            return 0;
        return m_impl->characters()[index];
    }

And isASCIIDigit('\0') is false, since it checks the '0'..'9' range. But still, this
feels "lucky". Would you prefer I explicitly add some bounds checking? I've
almost convinced myself I should =)

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