[webkit-reviews] review granted: [Bug 216188] [macOS] Add editability to input type=time : [Attachment 408017] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Aditya Keerthi
<akeerthi at apple.com>'s request for review:
Bug 216188: [macOS] Add editability to input type=time
https://bugs.webkit.org/show_bug.cgi?id=216188

Attachment 408017: Patch

https://bugs.webkit.org/attachment.cgi?id=408017&action=review




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 408017
  --> https://bugs.webkit.org/attachment.cgi?id=408017
Patch

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

r=me, nice work!

> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:313
> +    } else if (name == stepAttr && m_dateTimeEditElement) {
> +	   updateInnerTextValue();
>      }

Style: no `{` `}` if the body is a single line

> Source/WebCore/html/DateTimeFieldsState.h:48
> +    Optional<AMPMValue> ampm;

IMO it would be nicer to read as `meridiem` (e.g.
`-webkit-datetime-edit-meridiem-field`) than as `ampm`

> Source/WebCore/html/TimeInputType.cpp:121
> +    if (!state.hour || !state.minute || !state.ampm)

is it not possible (perhaps undesirable) to infer the `minute` and/or
`meridiem` based on the `hour`?  e.g. if the `hour` is `20`, then `minute` is
`0` and `meridien` is `PM`

> Source/WebCore/html/TimeInputType.cpp:140
> +    auto millisecondsPerSecond = Decimal::fromDouble(msPerSecond);
> +    auto millisecondsPerMinute = Decimal::fromDouble(msPerMinute);

<unrelated to this patch> would be neat if `Decimal` were `constexpr` :P

> Source/WebCore/html/TimeInputType.cpp:153
> +	   layoutParameters.fallbackDateTimeFormat = "HH:mm:ss";

`_s`

> Source/WebCore/html/TimeInputType.cpp:156
> +	   layoutParameters.fallbackDateTimeFormat = "HH:mm";

`_s`

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:110
> +    case 11: {

NIT: none of these `case` actually need the `{` `}` since they don't declare
any variables

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:111
> +	   state.hour = value ? value : 12;

<after reading below> Could we use `value ?: 12` here too?

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:119
> +	   state.hour = (value % 12) ?: 12;

`?:` ��

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:129
> +	   state.hour = value == 12 ? 12 : value % 12;

Why not also do `(value % 12) ?: 12` here too?

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:141
> +    case 11: {

ditto (:110)