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

[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 157604: Patch 6
https://bugs.webkit.org/attachment.cgi?id=157604&action=review

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