[Webkit-unassigned] [Bug 31331] step attribute and ValidityState.stepMismatch for type=number/range

[bugzilla-daemon at webkit.org]

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


TAMURA, Kent <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42924|0                           |1
        is obsolete|                            |
  Attachment #43246|                            |review?
               Flag|                            |




--- Comment #3 from TAMURA, Kent <tkent at chromium.org>  2009-11-15 08:50:50 PST ---
Created an attachment (id=43246)
 --> (https://bugs.webkit.org/attachment.cgi?id=43246)
Proposed patch (rev.3)

Thank you for reviewing.

(In reply to comment #2)
> (From update of attachment 42924 [details])
> > +        double doubleValue = 0.0;
> > +        if (!formStringToDouble(value(), &doubleValue))
> > +            return false;
> 
> Why does doubleValue to be initialized to 0? It's an out parameter of
> formStringToDouble and we don't continue if that function fails.

Removed it.
I had been afraid MSVC's "uninitialized variable" warning we saw in
rangeOverflow() and rangeUnderflow().
In this case, I confirmed MSVC worked well.

> Is it really right to call fabs on doubleValue - min? Is that the right thing
> to do with cases where doubleValue is < min?

Yes.  HTML5 draft doesn't define exceptional handling for such case and we need
to check stepMismatch.
Making the differential value non-negative is reasonable for the following
process.

> > +        // double's fractional part size is 52-bit.  If the current value is
> > +        // greater than step*2^52, the following fmod() makes no sense.
> > +        static const double fractionalSize = 52;
> > +        if (doubleValue / pow(2, fractionalSize) > step)
> > +            return false;
> 
> Is there a way to do this same check without hard-coding the value 52?

I found DBL_MANT_DIG, which is 53.  I replaced 52 with DBL_MANT_DIG.
Note: double's precision is 53-bit, and the fractional part size is 52-bit
because the most significant bit is omitted.

> > +    if (inputType() == RANGE) {
> > +        // stepMismatch doesn't occur for RANGE. RenderSlider guarantees the
> > +        // value matches to step.
> > +        return false;
> > +    }
> > +    ASSERT_NOT_REACHED();
> > +    return false;
> 
> The check for RANGE could be made assertion-only, since we have no special work
> to do for RANGE. How about just:
> 
>     ASSERT(inputType() == RANGE);
>     return false;
> 
> With an appropriate comment.

Done.

> > +    double defaultStep = 1.0;
> > +    double stepScaleFactor = 1.0;
> 
> It seems these are declared too early. You should declare variables just before
> where they are used rather than at the top of a function.
> 
> Also, since these are constants, I suggest defining them outside the function
> at the top of the file with "const".

Done: define them at the top.

> I have no idea why we are defining a constant stepScaleFactor of 1 and then
> multiplying by it. Is this to prepare for some future work? If not, I suggest
> just dropping that.
> 
> Also, it's rare that I say this, but the default of 1 seems so straightforward
> that I'm not sure it needs a named constant.

We'll need to use different values for
date/datetime/datetime-local/month/time/week.
So the ideal form would be:
    double defaultStep;
    double stepScaleFactor;
    switch (inputType()) {
    case NUMBER:
    case RANGE:
        defaultStep = numberDefaultStep;
        stepScaleFactor = numberStepScaleFactor;
        break;
    case DATE:
        defaultStep = ...;
        ...

But MSVC warns "uninitialized variable" for defaltStep and stepScaleFactor.
I changed the code like the following:
    double defaultStep = 0;
    double stepScaleFactor = 0;
    switch (inputType()) {
    case NUMBER:
    case RANGE:
        defaultStep = numberDefaultStep;
        stepScaleFactor = numberStepScaleFactor;
        break;
    case DATE:
        // FIXME

> > +    const String stepString = getAttribute(stepAttr);
> 
> As a matter of coding style and to avoid a slight bit of extra work, the result
> of getAttribute should go into a local variable of type const AtomicString&,
> not String.

Fixed.

> > +    // Returns the "allowed value step" defined in the HTML spec.
> > +    bool allowedValueStep(double*) const;
> 
> Since this function does not return the allowed value step, our naming scheme
> in WebKit would normally call it getAllowedValueStep. We use nouns when the
> function's result is described by its name, but here the function result is a
> success/failure boolean.

Renamed to getAllowedStep().

> > +                 attribute [Reflect] DOMString step;
> 
> Did you test with null?

Added tests for null, undefined, and non-string type.

> > +    // Rounds clampedValue to minimum + N * step;
> 
> This should end with a period instead of a semicolon.

Fixed.

> > +    double n = round((clampedValue - minimum) / step);
> > +    clampedValue = minimum + n * step;
> 
> I suggest merging these two lines into a single expression. You won't need "n"
> on subsequent lines if you take my suggestion below.
> 
> > +    if (clampedValue > maximum)
> > +       clampedValue = minimum + (n - 1) * step;
> 
> I suggest doing:
> 
>     clampedValue -= step;
> 
> instead of doing more multiplication.

Done.

> I see tests of stepMismatch, but no tests of SliderRange::clampValue.

I added a test that needs clampValue behavior.

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