[webkit-reviews] review denied: [Bug 92960] [Forms] Introduce shadow elements for multiple fields time input UI : [Attachment 156309] Patch 3

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 92960: [Forms] Introduce shadow elements for multiple fields time input UI
https://bugs.webkit.org/show_bug.cgi?id=92960

Attachment 156309: Patch 3
https://bugs.webkit.org/attachment.cgi?id=156309&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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()?