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
Summary: [Forms] Empty visible value of AM/PM field of multiple fields time input UI s...
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: 95542
Blocks:
  Show dependency tree / graph
 
Reported: 2012-09-02 19:23 PDT by yosin
Modified: 2012-09-02 23:03 PDT (History)
1 user (show)

See Also:

Attachments
Patch 1 (2.64 KB, patch)
2012-09-02 21:15 PDT, yosin 		no flags Details | Formatted Diff | Diff
Patch 2 (3.94 KB, patch)
2012-09-02 22:02 PDT, yosin 		no flags Details | Formatted Diff | Diff
Patch 3 (4.06 KB, patch)
2012-09-02 22:23 PDT, yosin 		no flags Details | Formatted Diff | Diff
Patch 4 (4.05 KB, patch)
2012-09-02 22:30 PDT, yosin 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-09-02 19:23:33 PDT 
* 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
Comment 1 yosin 2012-09-02 21:15:25 PDT 
Created attachment 161857 [details]
Patch 1
Comment 2 yosin 2012-09-02 21:16:23 PDT 
Comment on attachment 161857 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 3 Kent Tamura 2012-09-02 21:31:59 PDT 
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);.
Comment 4 yosin 2012-09-02 22:02:57 PDT 
Created attachment 161860 [details]
Patch 2
Comment 5 yosin 2012-09-02 22:05:01 PDT 
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()
Comment 6 Kent Tamura 2012-09-02 22:11:06 PDT 
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?
Comment 7 yosin 2012-09-02 22:13:05 PDT 
(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?
Comment 8 yosin 2012-09-02 22:23:47 PDT 
Created attachment 161862 [details]
Patch 3
Comment 9 yosin 2012-09-02 22:25:05 PDT 
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.
Comment 10 Kent Tamura 2012-09-02 22:26:45 PDT 
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().
Comment 11 yosin 2012-09-02 22:30:29 PDT 
Created attachment 161863 [details]
Patch 4
Comment 12 yosin 2012-09-02 22:32:00 PDT 
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
Comment 13 Kent Tamura 2012-09-02 22:59:26 PDT 
Comment on attachment 161863 [details]
Patch 4

ok
Comment 14 yosin 2012-09-02 23:03:06 PDT 
Comment on attachment 161863 [details]
Patch 4

Clearing flags on attachment: 161863

Committed r127400: <http://trac.webkit.org/changeset/127400>
Comment 15 yosin 2012-09-02 23:03:10 PDT 
All reviewed patches have been landed.  Closing bug.