Bug 95660 – [Forms] Empty visible value of AM/PM field of multiple fields time input UI should display variable number of "-" based on maximum number of labels

• Component: Forms
• Status: RESOLVED FIXED

* On Arabic language, AM/PM are represented in one character[1].
* Windows Visit, 7, or later, Users can set arbitrary string to AM/PM fields.
  * There are symbols for "a.m." (U+33C2 = "㏂") and "p.m." (U+33D8 = "㏘") in Unicode[2]

[1] http://unicode.org/cldr/trac/browser/tags/release-21-0-1/common/main/ar.xml
  <dayPeriods>
    <dayPeriodContext type="format">
      <dayPeriodWidth type="wide">
        <dayPeriod type="am">ص</dayPeriod>
        <dayPeriod type="pm">م</dayPeriod>
      </dayPeriodWidth">
    </dayPeriodContext>
  </dayPeriods>
[2] http://en.wikipedia.org/wiki/12-hour_clock#Typography

Created attachment 161857 [details]
Patch 1

Comment on attachment 161857 [details]
Patch 1

Could you review this patch?
Thanks in advance.

[Kent Tamura]

Comment on attachment 161857 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=161857&action=review

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:119
>  String DateTimeSymbolicFieldElement::visibleEmptyValue() const

Building the string every time is not efficient though this function is not so hot.

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:123
> +        maximumLength = std::max(maximumLength, m_symbols[index].length());

I think numGraphemeClusters(m_symbols[index]) is better than m_symbols[index].length().

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:124
> +    StringBuilder builder;

should have builder.reserveCapacity(maximumLength);.

Created attachment 161860 [details]
Patch 2

Comment on attachment 161860 [details]
Patch 2

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Introduce m_visibleEmptyValue for making visible empty value at instance construction time.
* Use numGraphemeClusters instead of String::length()
* Add to call StringBuilder::reserveCapacity()

[Kent Tamura]

Comment on attachment 161860 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=161860&action=review

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.h:60
> +    const AtomicString m_visibleEmptyValue;

Why AtomicString?

(In reply to comment #6)
> (From update of attachment 161860 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161860&action=review
> 
> > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.h:60
> > +    const AtomicString m_visibleEmptyValue;
> 
> Why AtomicString?

To share "--" among input elements for input type "time".
Do you think it overkill?

Created attachment 161862 [details]
Patch 3

Comment on attachment 161862 [details]
Patch 3

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Add comment about using AtomcString for m_visibleEmptyValue.

[Kent Tamura]

Comment on attachment 161862 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=161862&action=review

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:46
> +     builder.reserveCapacity(maximumLength);

wrong indentation

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:49
> +    return AtomicString(builder.toString());

should use builder.toAtomicString().

Created attachment 161863 [details]
Patch 4

Comment on attachment 161863 [details]
Patch 4

Could you review this patch?
Thanks in advance.

= Changes since the last patch =
* Use StringBuilder::toAtomicString() instead of AtomicString constructor with StringBuilder::toString()
* Fix indendation

[Kent Tamura]

Comment on attachment 161863 [details]
Patch 4

ok

Comment on attachment 161863 [details]
Patch 4

Clearing flags on attachment: 161863

Committed r127400: <http://trac.webkit.org/changeset/127400>

All reviewed patches have been landed.  Closing bug.