Bug 31331

Created attachment 42924 [details] Proposed patch (rev.2)

Comment on attachment 42924 [details] Proposed patch (rev.2) > + 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.

Created attachment 43246 [details] 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.

(In reply to comment #3) > > 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. Did you send mail to HTML5 or WHATWG mentioning this is undefined? I'd like to see this case covered in the specification. I'm not sure absolute value is best for this. Perhaps we should treat negative numbers as if they were zero or always just use 1 in that case? Having the same behavior in edge cases can sometimes prevent accidental site incompatibilities so I'd prefer to have this defined in the specification and covered by test cases even if it's not. > > > + // 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. Is DBL_MANT_DIG available on all the platforms we target? > 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 The warning is due to the case where inputType() is not one of the values in the switch statement. I suggest we break out the code to compute the default step and scale factor into a separate function with two out arguments (I'd use references for those, but others seem to often chose pointers for that purpose). Named something like getStep, getStepConstraints, getStepDefaults, getStepParameters, or something along those lines. That function can use a switch with return statements, and then have code after the switch that does: ASSERT_NOT_REACHED(); defaultStep = 1; stepScaleFactor = 1;

Created attachment 43350 [details] Proposed patch (rev.4) (In reply to comment #4) > (In reply to comment #3) > > > 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. > > Did you send mail to HTML5 or WHATWG mentioning this is undefined? I'd like to > see this case covered in the specification. > > I'm not sure absolute value is best for this. Perhaps we should treat negative > numbers as if they were zero or always just use 1 in that case? Having the same > behavior in edge cases can sometimes prevent accidental site incompatibilities > so I'd prefer to have this defined in the specification and covered by test > cases even if it's not. I renamed the variable `min' here to `stepBase.' I'm asking about this to WHATWG now, and have no reply yet. If the spec will be updated as "stepMismatch is false if rangeUnderflow or rangeOverflow", I'll upadte the code. I don't think this section in the current spec is ambiguous. We need to handle cases without min attribute. stepMismatch for <input type=number step=2 value=-2> should be false, and <input type=number step=2 value=-1> should be true because the `step base' is 0 in these cases. So we must handle a case of value < stepBase anyway. > > > 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. > > Is DBL_MANT_DIG available on all the platforms we target? It's defined in ISO C standard. I confirmed on Mac OS, VS 2005, and Linux. If other ports have no DBL_MANT_DIG, we may add it to wtf/MathExtras.h. > I suggest we break out the code to compute the default step and scale factor > into a separate function with two out arguments (I'd use references for those, > but others seem to often chose pointers for that purpose). Named something like > getStep, getStepConstraints, getStepDefaults, getStepParameters, or something > along those lines. That function can use a switch with return statements, and > then have code after the switch that does: Ok, I introduced getStepParameters(double*,double*).

Comment on attachment 43350 [details] Proposed patch (rev.4) > +// Constant values for getAllowedValueStep(). > +static const double numberDefaultStep = 1.0; > +static const double numberStepScaleFactor = 1.0; These are actually for getStepParameters; the comment is wrong. > + doubleValue = fabs(doubleValue - stepBase); I still remain skeptical about the absolute value here. But it's really the tests that will guarantee we have this right. > + if (isinf(doubleValue)) > + return false; > + // double's fractional part size is DBL_MAN_DIG-bit. If the current > + // value is greater than step*2^DBL_MANT_DIG, the following fmod() makes > + // no sense. > + if (doubleValue / pow(2, DBL_MANT_DIG) > step) > + return false; The pow-related check will take care of the infinity case naturally, so you don't need the explicit isinf check. > + double remainder = fmod(doubleValue, step); > + // Accepts errors in lower 7-bit. > + double acceptableError = step / pow(2, DBL_MANT_DIG - 7); > + return acceptableError < remainder && remainder < (step - acceptableError); Where does the constant 7 come from? The HTML5 specification? The comment should make that clear. > + // Sets the "allowed value step" defined in the HTML spec to the specified double pointer. > + // Returns false if there is no "allowed value step." > + bool getAllowedValueStep(double*) const; For non-optional out parameters, I really prefer references to pointers. But I suppose that's just a personal preference. > PassRefPtr<HTMLFormElement> createTemporaryFormForIsIndex(); > - > + // Helper for getAllowedValueStep(); > + bool getStepParameters(double* defaultStep, double* stepScaleFactor) const; > #if ENABLE(DATALIST) > HTMLDataListElement* dataList() const; > #endif The new function should not be put in the same paragraph with the other functions, especially the dataList function which is inside an ifdef. I think we need a blank line. r=me as is but it would be nice if you did another round considering my comments above

Comment on attachment 43350 [details] Proposed patch (rev.4) Oops, I said r=me By the way, if you look at the test output you'll see that it's a little hard to understand what's going on. A great test is something you can understand from the output, not just by reading the test. One technique is to use the "debug" function to write out text about what's being tested. Another is to put more of the code that triggers the test case inside the shouldBe expression, by calling a function to perform the test with arguments, so the new value and step values could be in the expression that shouldBe shows instead of just in the code between shouldBe statements. Think of test design as a creative endeavor where you are trying to communicate to the next person trying to understand your test.

Comment on attachment 43350 [details] Proposed patch (rev.4) Thank you for the advice about tests. I'll take care of it next time, or fix it by another patch.

Comment on attachment 43350 [details] Proposed patch (rev.4) Clearing flags on attachment: 43350 Committed r51159: <http://trac.webkit.org/changeset/51159>

All reviewed patches have been landed. Closing bug.