Bug 88383

Created attachment 145929 [details] Patch 1

Comment on attachment 145929 [details] Patch 1 Could you review this patch? Thanks in advance. * Local build with enabling all input type. * Test files increase size of this patch. They are 22KB.

Comment on attachment 145929 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=145929&action=review > Source/WebCore/html/MonthInputType.cpp:97 > - return months; > + return convertDoubleToInputNumber(months); nit: we had better add this function before this patch. > Source/WebCore/html/MonthInputType.cpp:118 > - return months; > + return convertDoubleToInputNumber(months); nit: we had better add this function before this patch. > Source/WebCore/html/NumberInputType.cpp:96 > - const double floatMax = convertDoubleToInputNumber(numeric_limits<float>::max()); > + const double floatMax = numeric_limits<float>::max(); > if (newValue < -floatMax) { nit: we had better add this function before this patch. > Source/WebCore/html/NumberInputType.cpp:161 > > - unsigned minValueDecimalPlaces; > - String minValue = element()->fastGetAttribute(minAttr); > - double minValueDouble = parseToDoubleForNumberTypeWithDecimalPlaces(minValue, &minValueDecimalPlaces); > - if (!isfinite(minValueDouble)) > - return false; > - > - unsigned maxValueDecimalPlaces; > - String maxValue = element()->fastGetAttribute(maxAttr); > - double maxValueDouble = parseToDoubleForNumberTypeWithDecimalPlaces(maxValue, &maxValueDecimalPlaces); > - if (!isfinite(maxValueDouble)) > + String stepString = element()->fastGetAttribute(stepAttr); > + if (equalIgnoringCase(stepString, "any")) > return false; > > - if (maxValueDouble < minValueDouble) { > - maxValueDouble = minValueDouble; > - maxValueDecimalPlaces = minValueDecimalPlaces; > + const Decimal minimum = parseToDecimalForNumberType(element()->fastGetAttribute(minAttr)); > + const Decimal maximum = parseToDecimalForNumberType(element()->fastGetAttribute(maxAttr)); > + const Decimal step = parseToDecimalForNumberType(stepString, 1); > + if (minimum.isFinite() && maximum.isFinite() && step.isFinite()) { > + preferredSize = max(max(calculateRenderSize(minimum), calculateRenderSize(maximum)), calculateRenderSize(step)); > + return true; > } > > - String stepValue = element()->fastGetAttribute(stepAttr); > - if (equalIgnoringCase(stepValue, "any")) > - return false; > - unsigned stepValueDecimalPlaces; > - double stepValueDouble = parseToDoubleForNumberTypeWithDecimalPlaces(stepValue, &stepValueDecimalPlaces); > - if (!isfinite(stepValueDouble)) { > - stepValueDouble = 1; > - stepValueDecimalPlaces = 0; > - } > - > - unsigned length = lengthBeforeDecimalPoint(minValueDouble); > - length = max(length, lengthBeforeDecimalPoint(maxValueDouble)); > - length = max(length, lengthBeforeDecimalPoint(stepValueDouble)); > - > - unsigned lengthAfterDecimalPoint = minValueDecimalPlaces; > - lengthAfterDecimalPoint = max(lengthAfterDecimalPoint, maxValueDecimalPlaces); > - lengthAfterDecimalPoint = max(lengthAfterDecimalPoint, stepValueDecimalPlaces); > - > - // '.' should be counted if the value has decimal places. > - if (lengthAfterDecimalPoint > 0) > - length += lengthAfterDecimalPoint + 1; > - > - preferredSize = length; > - return true; > + return false; I think this causes a behavior change. Would you add a test case for the following to fast/forms/number/input-number-size.html please? shouldBe('numberWidth(123456, 123456, 0.0000005)', 'textWidthPlusSpinButtonWidth(14)'); > Source/WebCore/html/StepRange.cpp:169 > + if (!isfinite(doubleValue)) > + return Decimal::nan(); > + > + NumberToStringBuffer buffer; > + return Decimal::fromString(numberToString(doubleValue, buffer)); Decimal class should have this implementation. Do we need to handle infinity? If not, we should have a comment and an assertion. > Source/WebCore/html/StepRange.cpp:179 > + if (!decimalValue.isFinite()) > + return std::numeric_limits<double>::quiet_NaN(); > + > + bool valid; > + const double doubleValue = decimalValue.toString().toDouble(&valid); > + return valid ? doubleValue : std::numeric_limits<double>::quiet_NaN(); ditto. > Source/WebCore/html/StepRange.h:38 > +// FIXME: We should rename InputNumber to Decimal in all places. > +typedef Decimal InputNumber; > +InputNumber convertDoubleToInputNumber(double); > +double convertInputNumberToDouble(const InputNumber&); > Please introduce isnan(const Decimal&) and isfinite(const Decimal&) to reduce the patch size. > Source/WebCore/html/StepRange.h:129 > + InputNumber roundByStep(const InputNumber& value, const InputNumber& base) const > + { > + return base + ((value - base) / m_step).round() * m_step; > + } > + nit: we had better add this function before this patch. > Source/WebCore/html/parser/HTMLParserIdioms.cpp:94 > + const uint64_t leadingDigitsOf2Power1024 = UINT64_C(17976931348623159); > + DEFINE_STATIC_LOCAL(const Decimal, htmlMax, (Decimal::Positive, 292, leadingDigitsOf2Power1024)); > + DEFINE_STATIC_LOCAL(const Decimal, htmlMin, (Decimal::Negative, 292, leadingDigitsOf2Power1024)); Could you explain why 17976931348623159E+292 ? Is it compatible with parseToDoubelForNumberType() below?

(In reply to comment #3) > > Source/WebCore/html/NumberInputType.cpp:96 > > - const double floatMax = convertDoubleToInputNumber(numeric_limits<float>::max()); > > + const double floatMax = numeric_limits<float>::max(); > > if (newValue < -floatMax) { > > nit: we had better add this function before this patch. correction: nit: we had better change it before this patch.

(In reply to comment #3) > > Source/WebCore/html/StepRange.h:129 > > + InputNumber roundByStep(const InputNumber& value, const InputNumber& base) const > > + { > > + return base + ((value - base) / m_step).round() * m_step; > > + } > > + > > nit: we had better add this function before this patch. Before using Decimal, one has scale and another is above. So, we can share code after Decimal.

(In reply to comment #5) > (In reply to comment #3) > > > Source/WebCore/html/StepRange.h:129 > > > + InputNumber roundByStep(const InputNumber& value, const InputNumber& base) const > > > + { > > > + return base + ((value - base) / m_step).round() * m_step; > > > + } > > > + > > > > nit: we had better add this function before this patch. > > Before using Decimal, one has scale and another is above. So, we can share code after Decimal. It seems passing "scale" as parameter can reduce size of patch. I'll try it.

Created attachment 146240 [details] Patch 2

Created attachment 146241 [details] Patch 3

Created attachment 146242 [details] Patch 4

Comment on attachment 146242 [details] Patch 4 Could you review this patch? Thanks in advance. * Changes are ** NumericInputType::sizeShouldIncludeDecoration * convertDecimalToDouble * convertDoubleToDecimal * Local build with enabling all INPUT_TYPE * GTK build failure is caused by previous build.

Comment on attachment 146242 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=146242&action=review Almost ok > Source/WebCore/html/NumberInputType.cpp:215 > + return value.isFinite() ? serializeForNumberType(value) : emptyString(); emptyString() should be String(). Do not change the function behavior though it might have no side-effect. > Source/WebCore/html/RangeInputType.cpp:223 > + return value.isFinite() ? serializeForNumberType(value) : emptyString(); ditto. > Source/WebCore/html/StepRange.h:49 > + struct DecimalOrMissing { Can you remove DecimalOrMissing by representing the missing state with Decimal::nan()? > Source/WebCore/html/parser/HTMLParserIdioms.cpp:65 > + // Decimal::toString append exponent, e.g. "0e-18" append -> appends

Created attachment 146508 [details] Patch 5

Created attachment 146522 [details] Patch 6 - Update ChnageLog

Comment on attachment 146522 [details] Patch 6 - Update ChnageLog Could you review this patch? Thanks in advance. * Patch 6 changes emptyString() => String() and NumberWithDecimalPlaces => InputNumber * Patch 6 changes ChangeLog only from Patch 5. * Local build on CR-Linux with enabling all input type

(In reply to comment #14) > (From update of attachment 146522 [details]) > Could you review this patch? > Thanks in advance. > > * Patch 6 changes emptyString() => String() and NumberWithDecimalPlaces => InputNumber > * Patch 6 changes ChangeLog only from Patch 5. > * Local build on CR-Linux with enabling all input type Patch 6 also changes comment in Source/WebCore/html/parser/HTMLParserIdioms.cpp as pointed.

Comment on attachment 146522 [details] Patch 6 - Update ChnageLog View in context: https://bugs.webkit.org/attachment.cgi?id=146522&action=review > Source/WebCore/ChangeLog:73 > + (WebCore::StepRange::roundByStep): Added for sharing code. Looks false. > Source/WebCore/html/NumberInputType.cpp:215 > - if (!isfinite(value)) > - return String(); > - return serializeForNumberType(value); > + return value.isFinite() ? serializeForNumberType(value) : String(); You could reduce the patch size if you didn't change 'if' to a ternary operator. > Source/WebCore/html/RangeInputType.cpp:223 > - if (!isfinite(value)) > - return String(); > - return serializeForNumberType(value); > + return value.isFinite() ? serializeForNumberType(value) : String(); You could reduce the patch size if you didn't change 'if' to a ternary operator. > Source/WebCore/html/StepRange.cpp:27 > +#include <wtf/dtoa.h> Is it needed? > Source/WebCore/html/StepRange.cpp:-30 > -using namespace std; > - Is this change needed now?

Created attachment 146542 [details] Patch 7

Comment on attachment 146542 [details] Patch 7 Could you review this patch? Thanks in advance. * Changes according to review comments * Local build on CR-Linux with enable all input types * Patch size is decreased 1KB then 67KB

Comment on attachment 146542 [details] Patch 7 ok

Comment on attachment 146542 [details] Patch 7 Clearing flags on attachment: 146542 Committed r119948: <http://trac.webkit.org/changeset/119948>

All reviewed patches have been landed. Closing bug.