[webkit-reviews] review granted: [Bug 214946] [macOS] Show picker for date and datetime-local input types : [Attachment 405541] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Aditya Keerthi <akeerthi at apple.com>'s
request for review:
Bug 214946: [macOS] Show picker for date and datetime-local input types
https://bugs.webkit.org/show_bug.cgi?id=214946

Attachment 405541: Patch

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




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 405541
  --> https://bugs.webkit.org/attachment.cgi?id=405541
Patch

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

> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:63
> +    if (Chrome* chrome = this->chrome()) {

I suggest auto.

> Source/WebCore/loader/EmptyClients.cpp:498
> +    return nullptr;

Why is this OK?

> Source/WebCore/page/Chrome.cpp:455
> +#if PLATFORM(IOS_FAMILY)
> +    UNUSED_PARAM(client);
> +    return nullptr;

Why is this correct?

> Source/WebCore/platform/DateTimeChooserParameters.h:31
> +#include "IntRect.h"
> +#include <wtf/text/WTFString.h>

I think we also need to include Vector.h.

> Source/WebKit/UIProcess/WebPageProxy.h:64
> +#include "WebDateTimePicker.h"

Would be nice to avoid having to add this include, and explore if we can do
this with only forward declaration in the header.

> Source/WebKit/UIProcess/WebPageProxy.h:1649
> +    void didChooseDate(const String&);

Same thought about StringView.

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:34
> +OBJC_CLASS WKDateTimePicker;

Since this is an ObjC only header, only included in Objective-C++ files, we can
and should write this:

@class WKDateTimePicker;

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:43
> +    void didChooseDate(const String&);

Suggest StringView.

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:46
> +    void endPicker() final;
> +    void showDateTimePicker(WebCore::DateTimeChooserParameters&&) final;

Consider making these functions private.

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.h:51
> +    NSView *m_view;

What guarantees this reference counted object, WebDateTimePickerMac, will not
outlive this other reference counted object, m_view?

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:38
> +static NSString * const kDateFormatString = @"yyyy-MM-dd";
> +static NSString * const kDateTimeFormatString = @"yyyy-MM-dd'T'HH:mm";
> +static NSString * const kDefaultLocaleIdentifier = @"en_US_POSIX";

Should consider constexpr for these too.

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:63
> +    if (m_picker) {
> +	   [m_picker invalidate];
> +	   m_picker = nil;
> +    }

Could just write:

    [m_picker invalidate];

No need to null out a pointer when the object is being destroyed. And no need
to check for null since Objective-C does nothing when passed null.

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:99
> +// TODO: Share this implementation with WKDataListSuggestionWindow.

WebKit project uses FIXME, not TODO.

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:118
> +    _backdropView = [[NSVisualEffectView alloc] initWithFrame:contentRect];

I believe this is going to leak because it’s missing adoptNS.

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:178
> +    RetainPtr<NSLocale> englishLocale = adoptNS([[NSLocale alloc]
initWithLocaleIdentifier:kDefaultLocaleIdentifier]);

I suggest using auto here

> Source/WebKit/WebProcess/WebCoreSupport/WebDateTimeChooser.h:45
> +    void didChooseDate(const String&);

Would be nicer if this took StringView, I think.