[Webkit-unassigned] [Bug 214946] [macOS] Show picker for date and datetime-local input types

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=214946

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com
 Attachment #405541|review?                     |review+
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200804/7275c242/attachment.htm>