[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42924|review?                     |review-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2009-11-13 09:28:30 PST ---
(From update of attachment 42924)
> +        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.

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

> +        // 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?

> +    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.

> +    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".

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.

> +    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.

> +    // 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.

> +                 attribute [Reflect] DOMString step;

Did you test with null?

> +    // Rounds clampedValue to minimum + N * step;

This should end with a period instead of a semicolon.

> +    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.

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

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