Bug 92960 - [Forms] Introduce shadow elements for multiple fields time input UI
Summary: [Forms] Introduce shadow elements for multiple fields time input UI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on: 93428
Blocks: 88970
  Show dependency treegraph
 
Reported: 2012-08-02 02:07 PDT by yosin
Modified: 2012-08-10 02:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch 1 (63.61 KB, patch)
2012-08-02 21:56 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (63.31 KB, patch)
2012-08-02 23:22 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (63.62 KB, patch)
2012-08-03 03:39 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (66.09 KB, patch)
2012-08-08 01:40 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 5 (63.87 KB, patch)
2012-08-09 18:05 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 6 (63.88 KB, patch)
2012-08-09 18:22 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 7 (64.04 KB, patch)
2012-08-09 22:35 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 8 (65.09 KB, patch)
2012-08-10 01:24 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 9 (65.08 KB, patch)
2012-08-10 01:30 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 11 (65.23 KB, patch)
2012-08-10 01:53 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 12 (65.22 KB, patch)
2012-08-10 02:06 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 13 (65.20 KB, patch)
2012-08-10 02:23 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-08-02 02:07:51 PDT
This bug is a port of multiple fields time UI, bug 88970.
Comment 1 yosin 2012-08-02 21:56:22 PDT
Created attachment 156257 [details]
Patch 1
Comment 2 yosin 2012-08-02 21:57:33 PDT
Comment on attachment 156257 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 3 WebKit Review Bot 2012-08-02 22:14:05 PDT
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 4 Kentaro Hara 2012-08-02 22:19:33 PDT
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.
Comment 5 yosin 2012-08-02 22:54:37 PDT
(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.
Comment 6 Kentaro Hara 2012-08-02 23:06:44 PDT
(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.)
Comment 7 yosin 2012-08-02 23:13:09 PDT
(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.
Comment 8 yosin 2012-08-02 23:22:24 PDT
Created attachment 156265 [details]
Patch 2
Comment 9 yosin 2012-08-02 23:23:59 PDT
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
Comment 10 Kentaro Hara 2012-08-03 02:19:44 PDT
(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?
Comment 11 yosin 2012-08-03 02:31:09 PDT
(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:--".
Comment 12 Kentaro Hara 2012-08-03 02:48:31 PDT
(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.
Comment 13 yosin 2012-08-03 03:39:42 PDT
Created attachment 156309 [details]
Patch 3
Comment 14 yosin 2012-08-03 03:41:02 PDT
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.
Comment 15 Kentaro Hara 2012-08-03 03:45:52 PDT
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 16 Kent Tamura 2012-08-07 01:41:09 PDT
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()?
Comment 17 Kent Tamura 2012-08-07 02:05:24 PDT
(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
Comment 18 yosin 2012-08-08 01:40:35 PDT
Created attachment 157149 [details]
Patch 4
Comment 19 yosin 2012-08-08 01:48:43 PDT
(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 20 yosin 2012-08-08 01:51:13 PDT
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
Comment 21 WebKit Review Bot 2012-08-08 01:53:54 PDT
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 22 Kent Tamura 2012-08-08 02:56:48 PDT
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 23 Kent Tamura 2012-08-08 03:21:33 PDT
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.
Comment 24 yosin 2012-08-09 18:05:32 PDT
Created attachment 157601 [details]
Patch 5
Comment 25 yosin 2012-08-09 18:22:27 PDT
Created attachment 157604 [details]
Patch 6
Comment 26 yosin 2012-08-09 18:24:40 PDT
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 27 Kent Tamura 2012-08-09 19:37:54 PDT
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
Comment 28 yosin 2012-08-09 22:35:08 PDT
Created attachment 157640 [details]
Patch 7
Comment 29 yosin 2012-08-09 22:37:17 PDT
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 30 Kent Tamura 2012-08-09 23:26:47 PDT
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?
Comment 31 yosin 2012-08-10 01:17:48 PDT
(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.
Comment 32 yosin 2012-08-10 01:24:18 PDT
Created attachment 157669 [details]
Patch 8
Comment 33 yosin 2012-08-10 01:27:17 PDT
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.
Comment 34 Kent Tamura 2012-08-10 01:30:45 PDT
(In reply to comment #33)
> (From update of attachment 157669 [details])
> Could you review this patch?

Please set r? flag.
Comment 35 yosin 2012-08-10 01:30:47 PDT
Created attachment 157671 [details]
Patch 9
Comment 36 yosin 2012-08-10 01:31:34 PDT
Comment on attachment 157671 [details]
Patch 9

Could you review this patch?
Thanks in advance.

= Changes since the last patch =
* Updates ChangeLog
Comment 37 Kent Tamura 2012-08-10 01:37:36 PDT
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.
Comment 38 yosin 2012-08-10 01:53:13 PDT
Created attachment 157676 [details]
Patch 11
Comment 39 yosin 2012-08-10 01:55:44 PDT
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 40 Kent Tamura 2012-08-10 02:01:27 PDT
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.
Comment 41 yosin 2012-08-10 02:06:46 PDT
Created attachment 157680 [details]
Patch 12
Comment 42 yosin 2012-08-10 02:23:30 PDT
Created attachment 157687 [details]
Patch 13
Comment 43 yosin 2012-08-10 02:25:05 PDT
Comment on attachment 157687 [details]
Patch 13

Clearing flags on attachment: 157687

Committed r125263: <http://trac.webkit.org/changeset/125263>
Comment 44 yosin 2012-08-10 02:25:14 PDT
All reviewed patches have been landed.  Closing bug.