Bug 92960

Created attachment 156257 [details] Patch 1

Comment on attachment 156257 [details] Patch 1 Could you review this patch? Thanks in advance.

Comment on attachment 156257 [details] Patch 1 Attachment 156257 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13417774

Comment on attachment 156257 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=156257&action=review > Source/WebCore/html/shadow/DateTimeEditElement.h:116 > + RefPtr<DateTimeFieldElement> m_AMPMField; > + RefPtr<DateTimeFieldElement> m_hour11Field; > + RefPtr<DateTimeFieldElement> m_hour12Field; > + RefPtr<DateTimeFieldElement> m_hour24Field; > + RefPtr<DateTimeFieldElement> m_hour23Field; > + RefPtr<DateTimeFieldElement> m_millisecondField; > + RefPtr<DateTimeFieldElement> m_minuteField; > + RefPtr<DateTimeFieldElement> m_secondField; > + RefPtr<SpinButtonElement> m_spinButton; Do these need to be RefPtrs? As far as I read your implementation, these RefPtrs are guaranteed to point to Elements in the (shadow) subtree of this object. If that's true, please use raw pointers. The lifetime of these Elements should be managed by the DOM tree (i.e. the DOM lifetime semantics guarantees that "if X is alive, all the nodes under X are kept alive"). Context: RefPtr<*(Element|Node)> in Element|Node objects can generate reference cycles, which is causing memory leak. I'm removing those unnecessary RefPtrs.

(In reply to comment #4) > (From update of attachment 156257 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156257&action=review > > > Source/WebCore/html/shadow/DateTimeEditElement.h:116 > > + RefPtr<DateTimeFieldElement> m_AMPMField; > > + RefPtr<DateTimeFieldElement> m_hour11Field; > > + RefPtr<DateTimeFieldElement> m_hour12Field; > > + RefPtr<DateTimeFieldElement> m_hour24Field; > > + RefPtr<DateTimeFieldElement> m_hour23Field; > > + RefPtr<DateTimeFieldElement> m_millisecondField; > > + RefPtr<DateTimeFieldElement> m_minuteField; > > + RefPtr<DateTimeFieldElement> m_secondField; > > + RefPtr<SpinButtonElement> m_spinButton; > > Do these need to be RefPtrs? As far as I read your implementation, these RefPtrs are guaranteed to point to Elements in the (shadow) subtree of this object. If that's true, please use raw pointers. The lifetime of these Elements should be managed by the DOM tree (i.e. the DOM lifetime semantics guarantees that "if X is alive, all the nodes under X are kept alive"). > > Context: RefPtr<*(Element|Node)> in Element|Node objects can generate reference cycles, which is causing memory leak. I'm removing those unnecessary RefPtrs. Yes, they should be ref counted. Here is scenario: 1. Create shadow elements for <input type=time step=1 value="12:34:56"> hour, minute, second fields are created and inserted into DOM. 2. Change step to 60 second field is removed from DOM 3. GC DateTimeEditElement keeps second field 4. Change step to 1 second field is inserted again into DOM.

(In reply to comment #5) > Yes, they should be ref counted. > > Here is scenario: > (1) Create shadow elements for <input type=time step=1 value="12:34:56"> > hour, minute, second fields are created and inserted into DOM. > (2) Change step to 60 > second field is removed from DOM > (3) GC > DateTimeEditElement keeps second field > (4) Change step to 1 > second field is inserted again into DOM. (Although I don't know much about the semantics of DateTimeEditElement,) what happens if GC reclaims the second field in (3) and DateTimeEditElement recreates the second field in (4). (You might need to retain the value of the second field somewhere.)

(In reply to comment #6) > (In reply to comment #5) > > Yes, they should be ref counted. > > > > Here is scenario: > > (1) Create shadow elements for <input type=time step=1 value="12:34:56"> > > hour, minute, second fields are created and inserted into DOM. > > (2) Change step to 60 > > second field is removed from DOM > > (3) GC > > DateTimeEditElement keeps second field > > (4) Change step to 1 > > second field is inserted again into DOM. > > (Although I don't know much about the semantics of DateTimeEditElement,) what happens if GC reclaims the second field in (3) and DateTimeEditElement recreates the second field in (4). (You might need to retain the value of the second field somewhere.) Each field keeps numeric/symbolic value or empty value of time component rather than DateTimeEditElement keeps value for representing empty field, e.g. "12:--:34", where "--" represents empty value.

Created attachment 156265 [details] Patch 2

Comment on attachment 156265 [details] Patch 2 Could you review this patch? Thanks in advance. = Changes since the last patch = * Remove displaySizeOfNumber() from DateTimeFieldElement.cpp ** Failed on CR-Linux build

(In reply to comment #7) > > > Here is scenario: > > > (1) Create shadow elements for <input type=time step=1 value="12:34:56"> > > > hour, minute, second fields are created and inserted into DOM. > > > (2) Change step to 60 > > > second field is removed from DOM > > > (3) GC > > > DateTimeEditElement keeps second field > > > (4) Change step to 1 > > > second field is inserted again into DOM. > > > > what happens if GC reclaims the second field in (3) and DateTimeEditElement recreates the second field in (4). > > Each field keeps numeric/symbolic value or empty value of time component rather than DateTimeEditElement keeps value for representing empty field, e.g. "12:--:34", where "--" represents empty value. What is your concern about recreating Elements before insertion? Would you elaborate on why you need to keep once-removed Elements?

(In reply to comment #10) > (In reply to comment #7) > > > > Here is scenario: > > > > (1) Create shadow elements for <input type=time step=1 value="12:34:56"> > > > > hour, minute, second fields are created and inserted into DOM. > > > > (2) Change step to 60 > > > > second field is removed from DOM > > > > (3) GC > > > > DateTimeEditElement keeps second field > > > > (4) Change step to 1 > > > > second field is inserted again into DOM. > > > > > > what happens if GC reclaims the second field in (3) and DateTimeEditElement recreates the second field in (4). > > > > Each field keeps numeric/symbolic value or empty value of time component rather than DateTimeEditElement keeps value for representing empty field, e.g. "12:--:34", where "--" represents empty value. > > What is your concern about recreating Elements before insertion? Would you elaborate on why you need to keep once-removed Elements? I'm trying to keep value before/after step change, e.g. (1) step=1 display=12:34:56 (2) step=60 display=12:34 (3) step=1 display=12:34:56 Although, it may be acceptable that (3) displaying "12:34:--".

(In reply to comment #11) > > What is your concern about recreating Elements before insertion? Would you elaborate on why you need to keep once-removed Elements? > > I'm trying to keep value before/after step change, e.g. > (1) step=1 display=12:34:56 > (2) step=60 display=12:34 > (3) step=1 display=12:34:56 > > Although, it may be acceptable that (3) displaying "12:34:--". If you want to keep the value, I guess that keeping the value on a DateTimeFieldElement object would be reasonable. Shadow DOM Elements are just for displaying values managed by the DateTimeFieldElement object.

Created attachment 156309 [details] Patch 3

Comment on attachment 156309 [details] Patch 3 Could you review this patch? Thanks in advance. = Changes since the last patch = * Use raw pointer for fields in DateTimeEdit class.

Thanks for the update. (I'm not familiar with the area, and I'd like to delegate the rest of review to other reviewers.)

Comment on attachment 156309 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=156309&action=review > Source/WebCore/html/shadow/DateTimeEditElement.cpp:29 > + > +#if ENABLE(INPUT_TYPE_TIME_MULTIPLE_FIELDS) > + nit: These blank lines are unnecessary. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:52 > +static const int hourInMillisecond = 3600 * 1000; > +static const int minuteInMillisecond = 60 * 1000; > +static const int secondInMillisecond = 1000; Please remove them. We have WTF::msPerHour, WTF::msPerMinute, WTF::msPerSecond. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:62 > + bool build(const String&); This class is 'Layouter.' So this function should be 'layout()'. Or you had better rename the class to *Builder. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:159 > + m_editElement->m_hour11Field = DateTimeHourFieldElement::create(m_editElement->document(), *m_editElement, hourPsuedoId, 0, 11).leakRef(); nit: Please add a local copy of m_editElement->document() to clean the code. This .leakRef() is a real memory leak bug. The codes should be: RefPtr<DateTimeFieldElement> prpElement = DateTimeHourFieldElement::create(document, ...); m_editElement->m_hour11Field = prpElement.get(); addField(prpElement.release()); addField() should take PassRefPtr<DateTimeFieldElement>. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:201 > + if (needSecondField() || m_formatDetail.hasSecondField) { Is m_formatDetail.hasSecondField needed? Here is a case block for DateTimeFormat::FieldTypeSecond. So m_formatDetail.hasSecondField is always true, right? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:260 > + m_spinButton = SpinButtonElement::create(document, *this).leakRef(); No one does deref() for m_spinButton. You should create a SpinButtonElement in setLayout(). > Source/WebCore/html/shadow/DateTimeEditElement.cpp:287 > + if (m_hour11Field) > + m_hour11Field->removeEventHandler(); > + > + if (m_hour12Field) > + m_hour12Field->removeEventHandler(); > + > + if (m_hour23Field) > + m_hour23Field->removeEventHandler(); > + > + if (m_hour24Field) > + m_hour24Field->removeEventHandler(); > + > + if (m_millisecondField) > + m_millisecondField->removeEventHandler(); > + > + if (m_minuteField) > + m_minuteField->removeEventHandler(); > + > + if (m_AMPMField) > + m_AMPMField->removeEventHandler(); > + > + if (m_secondField) > + m_secondField->removeEventHandler(); for (int i = 0; i < m_numberOfFields; ++i) { m_fields[i]->removeEventHandler(); > Source/WebCore/html/shadow/DateTimeEditElement.cpp:296 > + return container; should be container.release() > Source/WebCore/html/shadow/DateTimeEditElement.cpp:316 > + if (keyboardEvent->type() == eventNames().keydownEvent && fieldAt(m_focusFieldIndex) && !disabled() & !readOnly()) { We prefer early-return > Source/WebCore/html/shadow/DateTimeEditElement.cpp:335 > + if (keyIdentifier == "Left") { > + keyboardEvent->setDefaultHandled(); > + const int fieldIndex = previousFieldIndex(); > + if (fieldAt(fieldIndex)) > + setFocusField(fieldIndex); > + return; > + } > + > + if (keyIdentifier == "Right") { > + keyboardEvent->setDefaultHandled(); > + const int fieldIndex = nextFieldIndex(); > + if (fieldAt(fieldIndex)) > + setFocusField(fieldIndex); > + return; > + } > + > + if (keyIdentifier == "U+0009") { What's the expectation for key operations with m_focusFieldIndex == InvalidFieldIndex? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:354 > + if (mouseEvent->type() == eventNames().mousedownEvent && mouseEvent->button() == LeftButton && !disabled() && !readOnly()) { We prefer early-return. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:368 > +void DateTimeEditElement::moveToNextField() Move what? Probably focusOnNextField() is better. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:392 > +bool DateTimeEditElement::readOnly() const should be isReadOnly() > Source/WebCore/html/shadow/DateTimeEditElement.cpp:405 > + while (Node* node = firstChild()) > + removeChild(node); You can use ContainerNode::removeChildren() or removeAllChildren(). > Source/WebCore/html/shadow/DateTimeEditElement.cpp:415 > +} m_focusFieldIndex is not cleared. Why? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:417 > +void DateTimeEditElement::setFocusField(int newFocusFieldIndex) The function name should be focusFieldAt() or setFocusFieldIndex(). I think the former is good because this function is not a simple setter. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:440 > +void DateTimeEditElement::setLayout(const StepRange& stepRange) The function name sounds a setter for 'layout' field. probably just 'layout()' is ok. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:514 > + if (m_AMPMField) > + m_AMPMField->setValueAsInteger(date.hour() >= 12 ? 1 : 0); > + > + if (m_hour11Field) > + m_hour11Field->setValueAsInteger(date.hour()); > + > + if (m_hour12Field) > + m_hour12Field->setValueAsInteger(date.hour()); > + > + if (m_hour23Field) > + m_hour23Field->setValueAsInteger(date.hour()); > + > + if (m_hour24Field) > + m_hour24Field->setValueAsInteger(date.hour()); > + > + if (m_millisecondField) > + m_millisecondField->setValueAsInteger(date.millisecond()); > + > + if (m_minuteField) > + m_minuteField->setValueAsInteger(date.minute()); > + > + if (m_secondField) > + m_secondField->setValueAsInteger(date.second()); This is not a good design. Each of fields should know what it wants, and it should be handled by themselves. They should have setValueAsDate(const DateComponents&). > Source/WebCore/html/shadow/DateTimeEditElement.cpp:540 > + setFocusField(InvalidFieldIndex); wrong indentation > Source/WebCore/html/shadow/DateTimeEditElement.h:69 > + void removeEventHandler() { m_editControlOwner = 0; } The function name should be removeEditControlOwner. > Source/WebCore/html/shadow/DateTimeEditElement.h:76 > + static const int InvalidFieldIndex = -1; should be start with a lowercase letter. > Source/WebCore/html/shadow/DateTimeEditElement.h:83 > + // Time can be represent at most five fields, such as > + // 1. hour > + // 2. minute > + // 3. second > + // 4. millisecond > + // 5. AM/PM You assume this class is for type=time? If so, the class name should be TimeEditElement? > Source/WebCore/html/shadow/DateTimeEditElement.h:88 > + virtual bool disabled() const OVERRIDE; Why do you need to override disabled()?

(In reply to comment #16) > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:159 > > + m_editElement->m_hour11Field = DateTimeHourFieldElement::create(m_editElement->document(), *m_editElement, hourPsuedoId, 0, 11).leakRef(); > > nit: Please add a local copy of m_editElement->document() to clean the code. > > This .leakRef() is a real memory leak bug. The codes should be: > > RefPtr<DateTimeFieldElement> prpElement = DateTimeHourFieldElement::create(document, ...); > m_editElement->m_hour11Field = prpElement.get(); > addField(prpElement.release()); Ah, prpElement is a wrong name. prp prefix is for PassRefPtr variables. http://www.webkit.org/coding/RefPtr.html

Created attachment 157149 [details] Patch 4

(In reply to comment #16) > (From update of attachment 156309 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156309&action=review > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:29 > > + > > +#if ENABLE(INPUT_TYPE_TIME_MULTIPLE_FIELDS) > > + > > nit: These blank lines are unnecessary. > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:52 > > +static const int hourInMillisecond = 3600 * 1000; > > +static const int minuteInMillisecond = 60 * 1000; > > +static const int secondInMillisecond = 1000; > > Please remove them. We have WTF::msPerHour, WTF::msPerMinute, WTF::msPerSecond. Removed and replaced to msPerXXX. > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:62 > > + bool build(const String&); > > This class is 'Layouter.' So this function should be 'layout()'. > Or you had better rename the class to *Builder. Rename to DateTimeEditBuilder. > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:159 > > + m_editElement->m_hour11Field = DateTimeHourFieldElement::create(m_editElement->document(), *m_editElement, hourPsuedoId, 0, 11).leakRef(); > > nit: Please add a local copy of m_editElement->document() to clean the code. > > This .leakRef() is a real memory leak bug. The codes should be: * Rewrite to pass PassRef<DateTimeField> to addField. * Remove m_*Field fields. > > RefPtr<DateTimeFieldElement> prpElement = DateTimeHourFieldElement::create(document, ...); > m_editElement->m_hour11Field = prpElement.get(); > addField(prpElement.release()); > > addField() should take PassRefPtr<DateTimeFieldElement>. Done. > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:201 > > + if (needSecondField() || m_formatDetail.hasSecondField) { > > Is m_formatDetail.hasSecondField needed? Here is a case block for DateTimeFormat::FieldTypeSecond. So m_formatDetail.hasSecondField is always true, right? Yes. Removed m_formatDetail and hasSecondField. > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:260 > > + m_spinButton = SpinButtonElement::create(document, *this).leakRef(); > > No one does deref() for m_spinButton. > You should create a SpinButtonElement in setLayout(). Done > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:287 > > + if (m_hour11Field) > > + m_hour11Field->removeEventHandler(); > > + > > + if (m_hour12Field) > > + m_hour12Field->removeEventHandler(); > > + > > + if (m_hour23Field) > > + m_hour23Field->removeEventHandler(); > > + > > + if (m_hour24Field) > > + m_hour24Field->removeEventHandler(); > > + > > + if (m_millisecondField) > > + m_millisecondField->removeEventHandler(); > > + > > + if (m_minuteField) > > + m_minuteField->removeEventHandler(); > > + > > + if (m_AMPMField) > > + m_AMPMField->removeEventHandler(); > > + > > + if (m_secondField) > > + m_secondField->removeEventHandler(); > Introduced DateTimeField::setValueAsDate() and DateTimeMillisedondField, DateTImeSecondField, and DateTimeAMPMField. > for (int i = 0; i < m_numberOfFields; ++i) { > m_fields[i]->removeEventHandler(); > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:296 > > + return container; > > should be container.release() Done > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:316 > > + if (keyboardEvent->type() == eventNames().keydownEvent && fieldAt(m_focusFieldIndex) && !disabled() & !readOnly()) { > > We prefer early-return > Done > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:335 > > + if (keyIdentifier == "Left") { > > + keyboardEvent->setDefaultHandled(); > > + const int fieldIndex = previousFieldIndex(); > > + if (fieldAt(fieldIndex)) > > + setFocusField(fieldIndex); > > + return; > > + } > > + > > + if (keyIdentifier == "Right") { > > + keyboardEvent->setDefaultHandled(); > > + const int fieldIndex = nextFieldIndex(); > > + if (fieldAt(fieldIndex)) > > + setFocusField(fieldIndex); > > + return; > > + } > > + > > + if (keyIdentifier == "U+0009") { > > What's the expectation for key operations with m_focusFieldIndex == InvalidFieldIndex? > This function doe nothing if m_focusFieldIndex == invliadFieldIndex. It is checked by !fieldAt(m_folcusFieldIndex) Note: Tab key is handled by base class event handler to move focus to next/previous focus-sable element. > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:354 > > + if (mouseEvent->type() == eventNames().mousedownEvent && mouseEvent->button() == LeftButton && !disabled() && !readOnly()) { > > We prefer early-return. > Done > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:368 > > +void DateTimeEditElement::moveToNextField() > > Move what? > Probably focusOnNextField() is better. Renamed. > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:392 > > +bool DateTimeEditElement::readOnly() const > > should be isReadOnly() Done. > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:405 > > + while (Node* node = firstChild()) > > + removeChild(node); > > You can use ContainerNode::removeChildren() or removeAllChildren(). > Done. > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:415 > > +} > > m_focusFieldIndex is not cleared. Why? Reset m_focusFieldIndex to invalidFocusIndex. > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:417 > > +void DateTimeEditElement::setFocusField(int newFocusFieldIndex) > > The function name should be focusFieldAt() or setFocusFieldIndex(). I think the former is good because this function is not a simple setter. > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:440 > > +void DateTimeEditElement::setLayout(const StepRange& stepRange) > > The function name sounds a setter for 'layout' field. probably just 'layout()' is ok. > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:514 > > + if (m_AMPMField) > > + m_AMPMField->setValueAsInteger(date.hour() >= 12 ? 1 : 0); > > + > > + if (m_hour11Field) > > + m_hour11Field->setValueAsInteger(date.hour()); > > + > > + if (m_hour12Field) > > + m_hour12Field->setValueAsInteger(date.hour()); > > + > > + if (m_hour23Field) > > + m_hour23Field->setValueAsInteger(date.hour()); > > + > > + if (m_hour24Field) > > + m_hour24Field->setValueAsInteger(date.hour()); > > + > > + if (m_millisecondField) > > + m_millisecondField->setValueAsInteger(date.millisecond()); > > + > > + if (m_minuteField) > > + m_minuteField->setValueAsInteger(date.minute()); > > + > > + if (m_secondField) > > + m_secondField->setValueAsInteger(date.second()); > > This is not a good design. Each of fields should know what it wants, and it should be handled by themselves. They should have setValueAsDate(const DateComponents&). > Done. > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:540 > > + setFocusField(InvalidFieldIndex); > > wrong indentation > Fixed. > > Source/WebCore/html/shadow/DateTimeEditElement.h:69 > > + void removeEventHandler() { m_editControlOwner = 0; } > > The function name should be removeEditControlOwner. > Done. > > Source/WebCore/html/shadow/DateTimeEditElement.h:76 > > + static const int InvalidFieldIndex = -1; > > should be start with a lowercase letter. > Fixed. > > Source/WebCore/html/shadow/DateTimeEditElement.h:83 > > + // Time can be represent at most five fields, such as > > + // 1. hour > > + // 2. minute > > + // 3. second > > + // 4. millisecond > > + // 5. AM/PM > > You assume this class is for type=time? If so, the class name should be TimeEditElement? > I assume this class will handle datetime. Update comment. > > Source/WebCore/html/shadow/DateTimeEditElement.h:88 > > + virtual bool disabled() const OVERRIDE; > > Why do you need to override disabled()? Renamed to isDisabled().

Comment on attachment 157149 [details] Patch 4 Could you review this patch? Thanks in advance. = Changes since the last review = * Introduce DateTimeAMPMField, DateTimeMillisecondField, DateTimeSecondField in DateTimeFiledElements.h ** for setValueAsDate to remove m_XXXField and switch-like if statements. ** for simplify construction

Attachment 157149 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/html/shadow/DateTimeEditElement.cpp:84: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 157149 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=157149&action=review > Source/WebCore/html/shadow/DateTimeEditElement.cpp:67 > + class DateTimeFormatAnalyzer : private DateTimeFormat::TokenHandler { The class name DateTimeFormatAnalyzer sounds overkill. It is MilliSecondFieldChecker or something. We have no much benefit to make it an inner class of DateTimeEditBuilder. I think moving this class to the top level improves the code readability. BTW, do we really need to respect FieldTypeFractionalSecond in the format? Ignoring it and checking StepRange would reduce the code size and the code complexity. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:69 > + public: > + DateTimeFormatAnalyzer() The constructor should be private. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:139 > + DEFINE_STATIC_LOCAL(AtomicString, hourPsuedoId, ("-webkit-datetime-edit-hour-field")); > + DEFINE_STATIC_LOCAL(AtomicString, millisecondPsuedoId, ("-webkit-datetime-edit-millisecond-field")); > + DEFINE_STATIC_LOCAL(AtomicString, minutePsuedoId, ("-webkit-datetime-edit-minute-field")); > + DEFINE_STATIC_LOCAL(AtomicString, ampmPsuedoId, ("-webkit-datetime-edit-ampm-field")); > + DEFINE_STATIC_LOCAL(AtomicString, secondPsuedoId, ("-webkit-datetime-edit-second-field")); Now we have dedicated classes for these types. The constructor of each types can set its pseudo ID and we don't need to pass them in this function. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:208 > +} > + > +DateTimeEditElement::EditControlOwner::~EditControlOwner() > +{ > +} > + > +DateTimeEditElement::DateTimeEditElement(Document* document, EditControlOwner& editControlOwner) nit: This file has multiple classes. Adding delimiter comments improves code readability. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:375 > + for (int fieldIndex = m_focusFieldIndex + 1; fieldIndex < m_numberOfFields; ++fieldIndex) { Need ASSERT(m_focusFieldIndex != invalidFieldIndex) ? > Source/WebCore/html/shadow/DateTimeEditElement.cpp:384 > + for (int fieldIndex = m_focusFieldIndex - 1; fieldIndex >= 0; --fieldIndex) { ditto. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:440 > +void DateTimeEditElement::setValueAsEmpty() The function name looks strange. It should be setEmptyValue() or clearValue(). setValueAs* has been used for types such as setValueAsNumber and setValueAsDate(). > Source/WebCore/html/shadow/DateTimeEditElement.cpp:473 > + return std::numeric_limits<double>::quiet_NaN(); wrong indentation > Source/WebCore/html/shadow/DateTimeEditElement.h:113 > + DateTimeFieldElement* m_fields[maximumNumberOfFields]; > + int m_focusFieldIndex; > + int m_numberOfFields; Why don't you use Vector<DateTimeFieldElement, maximumNumberOfFields>? > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:51 > + if (event->isKeyboardEvent() && event->type() == eventNames().keydownEvent) { We prefer early-return > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:92 > + setAttribute(readonlyAttr, emptyString()); should use setBooleanAttribute() > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:99 > + removeAttribute(readonlyAttr); should use setBooleanAttribute() > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:113 > + ExceptionCode ec; > + textNode->replaceWholeText(newVisibleValue, ec); should be textNode->replaceWholeText(newVisibleValue, ASSERT_NO_EXCEPTION); > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:77 > + return static_cast<KeyboardEvent*>(event)->keyCode() - '0'; How to support localized digits? > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:154 > + return String::number(m_value); > + > + if (m_range.maximum > 99) > + return String::format("%03d", m_value); > + > + return String::format("%02d", m_value); Need number localization.

Comment on attachment 157149 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=157149&action=review >> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:92 >> + setAttribute(readonlyAttr, emptyString()); > > should use setBooleanAttribute() This is a real bug. Setting emptyString() doesn't remove the attribute. fastHasAttribute(readonlyAttr) still returns true.

Created attachment 157601 [details] Patch 5

Created attachment 157604 [details] Patch 6

Comment on attachment 157604 [details] Patch 6 Could you review this patch? Thanks in advance. = Changes since the last review = * Use convert{From,To}LocalizedNumber for localized digit input and localized number display * Use Vector<DateTimeFieldElement*> * Changes follow comments

Comment on attachment 157604 [details] Patch 6 View in context: https://bugs.webkit.org/attachment.cgi?id=157604&action=review > Source/WebCore/html/shadow/DateTimeEditElement.cpp:81 > +void DateTimeEditBuilder::addField(PassRefPtr<DateTimeFieldElement> field) const > +{ > + if (m_editElement->m_fields.size() == m_editElement->m_fields.capacity()) > + return; > + m_editElement->m_fields.append(field.get()); > + m_editElement->appendChild(field); > +} This should be a member function of DateTimeEditElement, and we can remove a "friend" declaration of DateTimeEditElement. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:160 > +bool DateTimeEditBuilder::needSecondField() const This function is similar to other needFooField() functions. Let's move this definition to the next to them. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:369 > + m_fields.clear(); Use shrink(0) instead of clear(), which deallocates the buffer. > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:130 > + return hasValue() ? static_cast<double>(valueAsInteger()) * unitInMillisecond() : std::numeric_limits<double>::quiet_NaN(); static_cast is not needed beucase unitInMillisecond() returns double. > Source/WebCore/html/shadow/DateTimeFieldElement.h:56 > + virtual void defaultEventHandler(Event*); OVERRIDE > Source/WebCore/html/shadow/DateTimeFieldElement.h:81 > + FieldEventHandler* m_fieldEventHandler; nit: add a blank line before the first data member. > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:61 > +int DateTimeNumericFieldElement::Range::ensureInRange(int value) const We use the word 'clamp' for such operation. > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:111 > + m_hasValue = isReadOnly(); Why? > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:54 > + if (WTF::Unicode::toLower(m_symbols[index][0]) == charCode) { We had better check m_symbols[index].length() > 0. > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.h:48 > + virtual bool hasValue() const; OVERRIDE

Created attachment 157640 [details] Patch 7

Comment on attachment 157640 [details] Patch 7 Could you review this patch? Thanks in advance. = Changes since the last review = * Move DateTimeEditBuilder::addField to DateTimeEditElement. * Changes for comments

Comment on attachment 157640 [details] Patch 7 View in context: https://bugs.webkit.org/attachment.cgi?id=157640&action=review > Source/WebCore/html/shadow/DateTimeEditElement.cpp:212 > + return fieldIndex >= 0 && static_cast<size_t>(fieldIndex) < m_fields.size() ? m_fields[fieldIndex] : 0; nit: If we define invalidFieldIndex as static_cast<size_t>(-1), and make m_focusFieldIndex size_t, this code would be much simpler. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:360 > + return -1; return invalidFieldIndex; ? > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:61 > +} nit: We had better support wheel events. It's ok to address it in a separated patch. > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:111 > + // For read-only field, empty value means value 0. > + m_hasValue = isReadOnly(); I see. However, we should make a second field read-only with value=30 if <input type=time min="00:00:30" step="60">, right?

(In reply to comment #30) > (From update of attachment 157640 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157640&action=review > > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:212 > > + return fieldIndex >= 0 && static_cast<size_t>(fieldIndex) < m_fields.size() ? m_fields[fieldIndex] : 0; > > nit: > If we define invalidFieldIndex as static_cast<size_t>(-1), and make m_focusFieldIndex size_t, this code would be much simpler. > Done. > > Source/WebCore/html/shadow/DateTimeEditElement.cpp:360 > > + return -1; > > return invalidFieldIndex; ? > Done. > > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:61 > > +} > > nit: We had better support wheel events. It's ok to address it in a separated patch. > I'll have another patch. > > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:111 > > + // For read-only field, empty value means value 0. > > + m_hasValue = isReadOnly(); > > I see. > However, we should make a second field read-only with value=30 if <input type=time min="00:00:30" step="60">, right? Yes, you're right. Fixed.

Created attachment 157669 [details] Patch 8

Comment on attachment 157669 [details] Patch 8 Could you review this patch? Thanks in advance. = Changes since the last review = * Make DateTimeEditElement::m_focusFieldIndex to size_t * Add DateComponents parameter to setEmptyValue for read-only fields.

(In reply to comment #33) > (From update of attachment 157669 [details]) > Could you review this patch? Please set r? flag.

Created attachment 157671 [details] Patch 9

Comment on attachment 157671 [details] Patch 9 Could you review this patch? Thanks in advance. = Changes since the last patch = * Updates ChangeLog

Comment on attachment 157671 [details] Patch 9 View in context: https://bugs.webkit.org/attachment.cgi?id=157671&action=review > Source/WebCore/html/shadow/DateTimeEditElement.cpp:376 > + --fieldIndex; wrong indentation > Source/WebCore/html/shadow/DateTimeEditElement.h:70 > + void setEmptyValue(const DateComponents&); it's unclear that what is passed by this DateComponents.

Created attachment 157676 [details] Patch 11

Comment on attachment 157676 [details] Patch 11 Could you review this patch? Thanks in advance. = Changes since the last review = * Rename a parameter of setEmptyValue to dateForReadOnlyField and put it into function declarations. * Fix indentation

Comment on attachment 157676 [details] Patch 11 View in context: https://bugs.webkit.org/attachment.cgi?id=157676&action=review ok. > Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:45 > +public: > + struct Range { should be protected.

Created attachment 157680 [details] Patch 12

Created attachment 157687 [details] Patch 13

Comment on attachment 157687 [details] Patch 13 Clearing flags on attachment: 157687 Committed r125263: <http://trac.webkit.org/changeset/125263>

All reviewed patches have been landed. Closing bug.