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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 8 02:56:23 PDT 2012


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

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


More information about the webkit-reviews mailing list