[webkit-reviews] review granted: [Bug 215155] [macOS] Date inputs should contain editable components : [Attachment 407499] 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 215155: [macOS] Date inputs should contain editable components
https://bugs.webkit.org/show_bug.cgi?id=215155

Attachment 407499: Patch

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




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

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

r=me, nice work!

One general comment is that there's a lot of places where you could use `auto`
(or `auto&`).  Up to you whether to keep the explicitly written type or not,
but I personally like to use `auto` for most things that are obvious from
reading surrounding code :)

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:81
> +    constexpr int countForAbbreviatedMonth = 3;
> +    constexpr int countForFullMonth = 4;
> +    constexpr int countForNarrowMonth = 5;

NIT: should we move these inside the `case` for months?

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:108
> +	   return;

NIT: why not replace the `break`s above with `return`?

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:135
> +    , m_editControlOwner(makeWeakPtr(&editControlOwner))

I don't think you need the `&`.  You should be able to `makeWeakPtr` with a
reference.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:204
> +    for (size_t fieldIndex = 0; fieldIndex < m_fields.size(); ++fieldIndex)
> +	   m_fields[fieldIndex]->setValueAsDate(date);

any reason not to use a range-for?
```
    for (auto& field : m_fields)
```

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:211
> +    for (size_t fieldIndex = 0; fieldIndex < m_fields.size(); ++fieldIndex)
> +	   m_fields[fieldIndex]->setEmptyValue();

ditto (:203)

> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:49
> +    , m_fieldOwner(makeWeakPtr(&fieldOwner))

I don't think you need the `&`.  You should be able to `makeWeakPtr` with a
reference.

> Source/WebCore/html/shadow/DateTimeFieldElement.h:66
> +    WeakPtr<FieldOwner> m_fieldOwner;

Is it possible/expected for `m_fieldOwner` to be destroyed before this object
is destroyed?  If not, can we just make this a `FieldOwner&` instead?

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:73
> +    setValueAsInteger(date.month() + 1);

Does this get "transformed" back to 0-based after the user edits it?  If not,
should it?

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.h:40
> +    DateTimeNumericFieldElement(Document&, FieldOwner&, const String&);

NIT: the `const String&` should also have parameter name here as it's not clear
what it's used for

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:44
> +    for (unsigned index = 0; index < symbols.size(); ++index)
> +	   maximumLength = std::max(maximumLength,
numGraphemeClusters(symbols[index]));

any reason not to use a range-for?
```
    for (auto& symbol : symbols)
```

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:48
> +    for (unsigned length = 0; length < maximumLength; ++length)
> +	   builder.append('-');

NIT: can we use `pad` to simplify this?