To reduce size of patch, we would like to introduce new type "InputNumber" type as an alias of double which will be replaced with "Decimal". This bug is a part of bug 80009.
Created attachment 145711 [details] Patch 1
Comment on attachment 145711 [details] Patch 1 * Build locally with enabling all input types. * EWS bots are too busy. Could you review this patch? Thanks in advance. P.S. It seems this patch is too big, however, replacing InputType to Decimal patch will be smaller. I expect to touch following files only: 1. Decimal.cpp - for convertDoubleToInputType, convertInputTypeToDouble 2. Decimal.h - for convertDoubleToInputType, convertInputTypeToDouble 3. InputType.cpp - for applyStep and stepFromRenderer 4. InputType.h - for removing parseNumberWithDecimalPlaces 2. NumberInputType.cpp - for removing parseNumberWithDecimalPlaces and changing sizeShouldIncludeDecoration. 3. StepRange.cpp - for alignValueForStep, parseStep and stepMismatch
Comment on attachment 145711 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=145711&action=review > Source/WebCore/html/BaseDateAndTimeInputType.cpp:60 > - element()->setValue(serializeWithMilliseconds(value)); > + element()->setValue(serializeWithMilliseconds(convertDoubleToInputNumber(value))); serializeWithMilliseconds() should take a double argument because it has strong relationship with DateComponents, and the number type and the range type don't have it. > Source/WebCore/html/BaseDateAndTimeInputType.cpp:115 > + return numeric_limits<InputNumber>::quiet_NaN(); This should be DateComponents::invalidMilliseconds(). > Source/WebCore/html/BaseDateAndTimeInputType.cpp:140 > -String BaseDateAndTimeInputType::serialize(double value) const > +String BaseDateAndTimeInputType::serialize(const InputNumber& value) const > { > if (!isfinite(value)) > return String(); should add convertInputNumberToDouble() for setMillisecondToDateComponents() below. > Source/WebCore/html/BaseDateAndTimeInputType.cpp:159 > -String BaseDateAndTimeInputType::serializeWithMilliseconds(double value) const > +String BaseDateAndTimeInputType::serializeWithMilliseconds(const InputNumber& value) const should revert. > Source/WebCore/html/BaseDateAndTimeInputType.cpp:188 > - return serializeWithMilliseconds(parsedValue); > + return serializeWithMilliseconds(convertDoubleToInputNumber(parsedValue)); ditto. > Source/WebCore/html/BaseDateAndTimeInputType.h:51 > - virtual bool setMillisecondToDateComponents(double, DateComponents*) const = 0; > + virtual bool setMillisecondToDateComponents(const InputNumber&, DateComponents*) const = 0; Same comment as serializeWithMilliseconds(). setMillisecondToDateComponents() should take a double argument. > Source/WebCore/html/DateInputType.cpp:93 > -bool DateInputType::setMillisecondToDateComponents(double value, DateComponents* date) const > +bool DateInputType::setMillisecondToDateComponents(const InputNumber& value, DateComponents* date) const > { > ASSERT(date); > - return date->setMillisecondsSinceEpochForDate(value); > + return date->setMillisecondsSinceEpochForDate(convertInputNumberToDouble(value)); should revert > Source/WebCore/html/DateInputType.h:53 > - virtual bool setMillisecondToDateComponents(double, DateComponents*) const OVERRIDE; > + virtual bool setMillisecondToDateComponents(const InputNumber&, DateComponents*) const OVERRIDE; should revert. > Source/WebCore/html/DateTimeInputType.cpp:91 > -bool DateTimeInputType::setMillisecondToDateComponents(double value, DateComponents* date) const > +bool DateTimeInputType::setMillisecondToDateComponents(const InputNumber& value, DateComponents* date) const > { > ASSERT(date); > - return date->setMillisecondsSinceEpochForDateTime(value); > + return date->setMillisecondsSinceEpochForDateTime(convertInputNumberToDouble(value)); should revert > Source/WebCore/html/DateTimeInputType.h:51 > - virtual bool setMillisecondToDateComponents(double, DateComponents*) const OVERRIDE; > + virtual bool setMillisecondToDateComponents(const InputNumber&, DateComponents*) const OVERRIDE; should revert > Source/WebCore/html/DateTimeLocalInputType.cpp:97 > -bool DateTimeLocalInputType::setMillisecondToDateComponents(double value, DateComponents* date) const > +bool DateTimeLocalInputType::setMillisecondToDateComponents(const InputNumber& value, DateComponents* date) const > { > ASSERT(date); > - return date->setMillisecondsSinceEpochForDateTimeLocal(value); > + return date->setMillisecondsSinceEpochForDateTimeLocal(convertInputNumberToDouble(value)); should revert > Source/WebCore/html/DateTimeLocalInputType.h:52 > - virtual bool setMillisecondToDateComponents(double, DateComponents*) const OVERRIDE; > + virtual bool setMillisecondToDateComponents(const InputNumber&, DateComponents*) const OVERRIDE; should revert > Source/WebCore/html/InputType.cpp:924 > > - element()->setValueAsNumber(newValue, ec, eventBehavior); > + element()->setValueAsInputNumber(newValue, ec, eventBehavior); > We can call InputType::setValueAsInputNumber() directly, and can remove HTMLInputElement::setValueAsInputNumber(). > Source/WebCore/html/NumberInputType.cpp:102 > + const InputNumber floatMax = convertDoubleToInputNumber(numeric_limits<float>::max()); Please add a FIXME comment that we need to change this doubleMax in the future. > Source/WebCore/html/StepRange.h:33 > +// FIXME: type InputNumber will be replaced with Decimal. A sentence should be started with a capital letter. type -> Type
Created attachment 145744 [details] Patch 2
Comment on attachment 145744 [details] Patch 2 * Build with all input types. * Revert serializeWithMilliseconds and setMillisecondToDateComponents ** size of patch is decreased by 8K. * Changed as review comments * EWS bots are too busy... Could you review again? Thanks in advance.
Comment on attachment 145744 [details] Patch 2 ok
Comment on attachment 145744 [details] Patch 2 Clearing flags on attachment: 145744 Committed r119540: <http://trac.webkit.org/changeset/119540>
All reviewed patches have been landed. Closing bug.
It appears that this patch increased the binary size significantly: http://build.chromium.org/f/chromium/perf/linux-release-64/sizes/report.html?history=150&rev=140809 Blame list: http://trac.webkit.org/log/?verbose=on&rev=119550&stop_rev=119536
Actually, this could be due to the libjingle change: http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&mode=html&range=140704:140708
(In reply to comment #9) > It appears that this patch increased the binary size significantly: > http://build.chromium.org/f/chromium/perf/linux-release-64/sizes/report.html?history=150&rev=140809 > > Blame list: http://trac.webkit.org/log/?verbose=on&rev=119550&stop_rev=119536 (In reply to comment #10) > Actually, this could be due to the libjingle change: http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&mode=html&range=140704:140708 I don't think the patch in this bug caused the significant size change.