Summary: | [Forms] Move numeric related methods in HTMLInputElement class to another place | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yosin | ||||||||||||||||||||||
Component: | Forms | Assignee: | yosin | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, gustavo, tkent, webkit.review.bot, xan.lopez | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 85978, 86058 | ||||||||||||||||||||||||
Bug Blocks: | 80009 | ||||||||||||||||||||||||
Attachments: |
|
Description
yosin
2012-03-23 01:34:38 PDT
*** Bug 85959 has been marked as a duplicate of this bug. *** Following methods are used in renderer and/or validator. So, we keep them. * rangeUnderflow * rangeOverflow * minimum * maximum * getAllowedValueStep * stepMismatch * minimumString * maximumString * stepBBaseString * stepString Created attachment 141855 [details]
Patch 1
Comment on attachment 141855 [details]
Patch 1
Build with CR-Linux and CR-Mac.
Could you review my patch?
Thanks in advance.
P.S. decimal places related code will be removed when we introduce decimal arithmetic.
Comment on attachment 141855 [details] Patch 1 Attachment 141855 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12689619 Comment on attachment 141855 [details] Patch 1 Attachment 141855 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12685657 New failing tests: fast/css/pseudo-in-range-invalid-value.html Created attachment 141861 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 141871 [details]
Patch 2 - Nothing changed
Comment on attachment 141871 [details]
Patch 2 - Nothing changed
Noting changed since Patch1.
I'm not sure why Mac-EWS failed.
Anyway, could you review this patch?
Thanks in advance.
Comment on attachment 141871 [details] Patch 2 - Nothing changed Attachment 141871 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12703373 Comment on attachment 141871 [details] Patch 2 - Nothing changed View in context: https://bugs.webkit.org/attachment.cgi?id=141871&action=review > Source/WebCore/html/DateTimeInputType.cpp:79 > + const String& stepString = element()->fastGetAttribute(stepAttr); Holding a reference of a temporary object created by conversion from const AtomicString& looks dangerous. This should be just "String." > Source/WebCore/html/InputType.h:164 > + virtual void setupStepRange(AnyStepHandling, StepRange*) const; I think we had better make this function a factory. virtual StepRange createStepRange() const; > Source/WebCore/html/StepRange.cpp:119 > +double StepRange::step() const > +{ > + return m_step; > +} > + > +double StepRange::stepBase() const > +{ > + return m_stepBase; > +} > + > +unsigned StepRange::stepBaseDecimalPlaces() const > +{ > + return m_stepBaseDecimalPlaces; > } > > +unsigned StepRange::stepDecimalPlaces() const > +{ > + return m_stepDecimalPlaces; > +} > + > +int StepRange::stepScaleFactor() const > +{ > + return m_stepDescription.stepScaleFactor; > +} Such simple accessor functions should be defined in StepRange.h in order to make them inline. > Source/WebCore/html/StepRange.cpp:121 > +void StepRange::setHasStep(bool hasStep) Can we merge this to the constructor? > Source/WebCore/html/StepRange.cpp:174 > +void StepRange::setup(AnyStepHandling anyStepHandling, double stepBase, double minimum, double maximum, const String& stepString, const StepDescription& stepDescription, unsigned stepBaseDecimalPlaces) > +{ Can we merge this to the constructor? Created attachment 141892 [details]
Patch 3
Comment on attachment 141892 [details]
Patch 3
* Fix Mac build
* Changes by review comments: use createStepRange and StepRange ctor instead of setupStepRange and step method.
* CR-Linux failure tests aren't related to this patch.
Please review again.
Thanks in advance.
Comment on attachment 141892 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=141892&action=review Please make sure this patch is compilable with enabling all of ENABLE_INPUT_TYPE_*. > Source/WebCore/html/DateInputType.cpp:81 > + const String& stepString = element()->fastGetAttribute(stepAttr); should be String. > Source/WebCore/html/DateTimeLocalInputType.cpp:85 > + const String& stepString = element()->fastGetAttribute(stepAttr); ditto. > Source/WebCore/html/HTMLInputElement.h:69 > + StepRange createStepRange(AnyStepHandling) const; Do we need this? > Source/WebCore/html/InputType.cpp:258 > + StepRange stepRange(createStepRange(RejectAny)); > + return doubleValue < stepRange.minimum(); You don't need the "stepRange" object. return doubleValue < createStepRange(RejectAny).minimum(); is enough. > Source/WebCore/html/InputType.cpp:271 > + StepRange stepRange(createStepRange(RejectAny)); > + return doubleValue > stepRange.maximum(); ditto. > Source/WebCore/html/InputType.cpp:282 > + StepRange stepRange(createStepRange(RejectAny)); > + return stepRange.minimum(); ditto. > Source/WebCore/html/InputType.cpp:288 > + StepRange stepRange(createStepRange(RejectAny)); > + return stepRange.maximum(); ditto. > Source/WebCore/html/MonthInputType.cpp:109 > + const String& stepString = element()->fastGetAttribute(stepAttr); should be String. > Source/WebCore/html/NumberInputType.cpp:121 > + const String& stepString = element()->fastGetAttribute(stepAttr); should be String. > Source/WebCore/html/NumberInputType.h:51 > + virtual StepRange createStepRange(AnyStepHandling) const; should add OVERRIDE > Source/WebCore/html/RangeInputType.cpp:273 > + StepRange stepRange(createStepRange(RejectAny)); > + return serializeForNumberType(stepRange.defaultValue()); return serializeForNumberType(createStepRange(RejectAny).defaultValue()); is enough. > Source/WebCore/html/RangeInputType.h:51 > + virtual StepRange createStepRange(AnyStepHandling) const; should add OVERRIDE > Source/WebCore/html/StepRange.cpp:35 > +StepRange::StepRange() Is this used? > Source/WebCore/html/StepRange.cpp:54 > +StepRange::StepRange(const StepRange& stepRange) > { > - double clampedValue = max(minimum, min(value, maximum)); > - if (!hasStep) > - return clampedValue; > - // Rounds clampedValue to minimum + N * step. > - clampedValue = minimum + round((clampedValue - minimum) / step) * step; > - if (clampedValue > maximum) > - clampedValue -= step; > - ASSERT(clampedValue >= minimum); > - ASSERT(clampedValue <= maximum); > - return clampedValue; > + m_hasStep = stepRange.m_hasStep; > + m_maximum = stepRange.m_maximum; > + m_minimum = stepRange.m_minimum; > + m_step = stepRange.m_step; > + m_stepBase = stepRange.m_stepBase; > + m_stepBaseDecimalPlaces = stepRange.m_stepBaseDecimalPlaces; > + m_stepDecimalPlaces = stepRange.m_stepDecimalPlaces; we should use member initializers, and we can make some members const. > Source/WebCore/html/StepRange.cpp:66 > +StepRange::StepRange(AnyStepHandling anyStepHandling, double stepBase, double minimum, double maximum, const String& stepString, const StepDescription& stepDescription, unsigned stepBaseDecimalPlaces) > +{ > + ASSERT(isfinite(maximum)); > + ASSERT(isfinite(minimum)); > + ASSERT(isfinite(stepBase)); > + m_maximum = maximum; > + m_minimum = minimum; > + m_stepBase = stepBase; > + m_stepBaseDecimalPlaces = stepBaseDecimalPlaces; > + m_stepDescription = stepDescription; ditto. > Source/WebCore/html/TimeInputType.cpp:91 > + const String& stepString = element()->fastGetAttribute(stepAttr); should be String. > Source/WebCore/html/TimeInputType.h:49 > + virtual StepRange createStepRange(AnyStepHandling) const; should add OVERRIDE. > Source/WebCore/html/WeekInputType.cpp:75 > + const String& stepString = element()->fastGetAttribute(stepAttr); should be String. > Source/WebCore/html/WeekInputType.h:48 > + virtual StepRange createStepRange(AnyStepHandling) const; should add OVERRIDE (In reply to comment #14) > (From update of attachment 141892 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141892&action=review > > > Source/WebCore/html/HTMLInputElement.h:69 > > + StepRange createStepRange(AnyStepHandling) const; > > Do we need this? Static inline function sliderPosition in SliderThumbElement.cpp needs this. > > Source/WebCore/html/StepRange.cpp:35 > > +StepRange::StepRange() > > Is this used? Default method of createStepRange in InputType uses this. Created attachment 141906 [details]
Patch 4
Comment on attachment 141906 [details]
Patch 4
Please review again.
Thanks in advance.
I did:
* Build with ENABLE_INPUT_TYPE_*
* Build with Chromium-Mac
* Changes as review comments
** Use StepRange ctor instead of setup method.
** Fix const String& issue for fastGetAttribute
* HTMLInputElement::createStepRange is used by sliderPosition in SliderThumbElement.cpp
* Default ctor of StepRange is used by default method of InputType::createStepRange
Test failures of CR-Linux build aren't related to this patch. Comment on attachment 141906 [details] Patch 4 Attachment 141906 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12706310 New failing tests: media/media-document-audio-repaint.html media/audio-repaint.html media/controls-drag-timebar.html Created attachment 141969 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #18) > Test failures of CR-Linux build aren't related to this patch. How did you confirm it? Comment on attachment 141906 [details]
Patch 4
I'm changing StepRange now...
Created attachment 142143 [details]
Patch 5
(In reply to comment #21) > (In reply to comment #18) > > Test failures of CR-Linux build aren't related to this patch. > > How did you confirm it? I examined HTML files of failed tests and check they contains "input" element or not. (In reply to comment #24) > (In reply to comment #21) > > (In reply to comment #18) > > > Test failures of CR-Linux build aren't related to this patch. > > > > How did you confirm it? > > I examined HTML files of failed tests and check they contains "input" element or not. <audio> and <video> contain some <input type=range> in shadow trees. Failure of media test was caused by wrong handling of "precision" attribute with "float" value. Patch 4 marked it had step value instead of no step value. Comment on attachment 142143 [details]
Patch 5
* Introducing DoubleWithDecimalPlaces and DoubleWithDecimalPlacesOrMissing for making StepRange immutable.
* Local build with ENABLE_INPUT_TYPE_*
* Build with clang on CR-Mac
* Failure of CR-Linux on media tests was fixed.
Please review again.
Thanks in advance.
Comment on attachment 142143 [details]
Patch 5
This patch succeeded on CR-Linux EWS. Failed tests aren't related to this patch.
Comment on attachment 142143 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=142143&action=review > Source/WebCore/html/DateInputType.cpp:76 > + StepRange::StepDescription stepDescription; > + stepDescription.defaultStep = dateDefaultStep; > + stepDescription.parsedStepValueShouldBeInteger = true; > + stepDescription.stepScaleFactor = dateStepScaleFactor; StepDescription is constant. We don't need to create it every time when createStepRange() is called. We had better make a static object like the following, and pass it to the StepRange constructor. StepRange should hold a reference/pointer to a static StepDescription object. DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription, (dataDefaultStep, 0, true, false, dateStepScaleFactor)); > Source/WebCore/html/StepRange.cpp:43 > +StepRange::StepRange() > + : m_hasStep(false) > + , m_maximum(100) > + , m_minimum(0) > + , m_step(1) > + , m_stepBase(0) > + , m_stepBaseDecimalPlaces(0) > + , m_stepDecimalPlaces(0) > { Should add ASSERT_NOT_REACHED()? > Source/WebCore/html/StepRange.h:63 > + int defaultStep; > + int defaultStepBase; > + bool parsedStepValueShouldBeInteger; > + bool scaledStepValueShouldBeInteger; > + int stepScaleFactor; > + We had better reorder them like int,int,int,bool,bool in order to minimize the object size. > Source/WebCore/html/StepRange.h:127 > + const bool m_hasStep; > + const double m_maximum; // maximum must be >= minimum. > + const double m_minimum; > + const double m_step; > + const double m_stepBase; > + const unsigned m_stepBaseDecimalPlaces; > + const unsigned m_stepDecimalPlaces; > + const StepDescription m_stepDescription; ditto. > Source/WebCore/html/WeekInputType.cpp:47 > -static const double weekDefaultStepBase = -259200000.0; // The first day of 1970-W01. > -static const double weekDefaultStep = 1.0; > -static const double weekStepScaleFactor = 604800000.0; > +static const int weekDefaultStepBase = -259200000; // The first day of 1970-W01. > +static const int weekDefaultStep = 1; > +static const int weekStepScaleFactor = 604800000; Why did you change them to int though you didn't change such values for other types? Created attachment 142683 [details]
Patch 6
Comment on attachment 142683 [details] Patch 6 Attachment 142683 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12727329 Comment on attachment 142683 [details] Patch 6 Attachment 142683 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12723628 Comment on attachment 142683 [details] Patch 6 Attachment 142683 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12715864 Comment on attachment 142683 [details] Patch 6 Attachment 142683 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12718841 Comment on attachment 142683 [details] Patch 6 Attachment 142683 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12725476 Comment on attachment 142683 [details] Patch 6 Attachment 142683 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12719827 Comment on attachment 142683 [details] Patch 6 View in context: https://bugs.webkit.org/attachment.cgi?id=142683&action=review > Source/WebCore/html/HTMLInputElement.h:161 > > - void setEditingValue(const String&); > - > double valueAsDate() const; Do not edit unrelated part. > Source/WebCore/html/HTMLInputElement.h:294 > > - virtual void copyNonAttributePropertiesFromElement(const Element&); > + virtual void copyNonAttributeProperties(const Element* source); > ditto. Comment on attachment 142683 [details] Patch 6 Attachment 142683 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12716881 Comment on attachment 142683 [details] Patch 6 Attachment 142683 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12726446 Created attachment 142930 [details]
Patch 7
Comment on attachment 142930 [details]
Patch 7
* Make StepRange immutable
* CR-Linux test failures aren't related to this patch
** fast/text/international/thai-line-breaks.html
** platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html
Thanks in advance.
-yosi
Comment on attachment 142930 [details] Patch 7 View in context: https://bugs.webkit.org/attachment.cgi?id=142930&action=review > Source/WebCore/html/DateInputType.cpp:74 > + DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription, (dateDefaultStep, dateDefaultStepBase, true, false, dateStepScaleFactor)); "ture, false' is not good for code readability. How about the followings? const bool ParsedStepValueShouldBeInteger = true; const bool ScaledStepValueShouldBeInteger = true; DEFINE_STATIC_LOCAL(... , ParseStepValueShoudlBeInteger, !ScaledStepValeuShouldBeInteger, ...) We usually use enum in such cases. But I feel enum is overkill in this case because the number of callers of the StepDescription constructors is limited. > Source/WebCore/html/DateTimeInputType.cpp:72 > + DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription, (dateTimeDefaultStep, dateTimeDefaultStepBase, false, true, dateTimeStepScaleFactor)); ditto. > Source/WebCore/html/DateTimeLocalInputType.cpp:78 > + DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription, (dateTimeLocalDefaultStep, dateTimeLocalDefaultStepBase, false, true, dateTimeLocalStepScaleFactor)); ditto. > Source/WebCore/html/MonthInputType.cpp:102 > + DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription, (monthDefaultStep, monthDefaultStepBase, true, false, monthStepScaleFactor)); ditto. > Source/WebCore/html/NumberInputType.cpp:113 > + DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription, (numberDefaultStep, numberDefaultStepBase, false, false, numberStepScaleFactor)); ditto. > Source/WebCore/html/RangeInputType.cpp:95 > + DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription, (rangeDefaultStep, rangeDefaultStepBase, false, false, rangeStepScaleFactor)); ditto. > Source/WebCore/html/TimeInputType.cpp:84 > + DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription, (timeDefaultStep, timeDefaultStepBase, false, false, timeStepScaleFactor)); ditto. > Source/WebCore/html/WeekInputType.cpp:66 > + DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription, (weekDefaultStep, weekDefaultStepBase, false, true, weekStepScaleFactor)); ditto. Created attachment 142942 [details]
Patch 8
Comment on attachment 142942 [details]
Patch 8
* Build with ENABLE_INPUT_TYPE_*=1
* Build with CR-Linux w/ clang
* Introduce StepValueShouldBe enum to combine two Boolean values. We need three value rather than four value.
** There is no combination parsed and scaled should be integer.
Could you review again?
Thanks in advance.
Comment on attachment 142942 [details] Patch 8 Clearing flags on attachment: 142942 Committed r117738: <http://trac.webkit.org/changeset/117738> All reviewed patches have been landed. Closing bug. |