Bug 211825 – Change Date/Time popovers to contextMenus.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Change Date/Time popovers to contextMenus.

[Megan Gardner]

Created attachment 399226 [details]
Patch

[Megan Gardner]

Created attachment 399235 [details]
Patch

[Megan Gardner]

Created attachment 399239 [details]
Patch

[Wenson Hsieh]

Comment on attachment 399239 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Removed all the unneded popover code.

Nit - unneeded

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:104
> +@class WKDateTimeInputControl;

Nit - let’s keep this sorted and move it above WKDrawingCoordinator.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:542
> +- (NSString*)inputLabelText;

Nit - space before *

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8628
> +- (WKDateTimeInputControl *)formInputControl

Should we also rename `-formInputControl` to `-dateTimeInputControl`?

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:44
> +}
> +- (id)init;

Nit - no need for the { }, as well as the -init declaration.

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:67
> +- (id)initWithView:(WKContentView *)view datePickerMode:(UIDatePickerMode)mode;

Let’s use instancetype for the return value.

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:87
> +- (id)init
> +{
> +    if (!(self = [super init]))
> +        return nil;
> +    return self;
> +}

Do we need to override this?

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:175
> +        [_viewController setToolbarItems:@[ [[UIBarButtonItem alloc] initWithBarButtonSystemItem:UIBarButtonSystemItemFlexibleSpace target:nil action:nil], [[UIBarButtonItem alloc] initWithTitle:resetString style:UIBarButtonItemStylePlain target:self action:@selector(reset:)] ]];

The two UIBarButtonItems leak here.

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:182
> +            UILabel *title = [[UILabel alloc] init];

The UILabel leaks here.

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:187
> +            _viewController.get().navigationItem.leftBarButtonItem = [[UIBarButtonItem alloc] initWithCustomView:title];

This leaks the UIBarButtonItem.

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:432
> +    RetainPtr<NSObject <WKFormControl>> control;
> +    control = adoptNS([[WKDateTimePicker alloc] initWithView:view datePickerMode:mode]);

Nit - I think you can just return [super initWithView:view control:adoptNS([[WKDateTimePicker alloc] initWithView:view datePickerMode:mode])];

[Wenson Hsieh]

Comment on attachment 399239 [details]
Patch

r=me, with fixes for the memory leaks and style nits

[Wenson Hsieh]

Comment on attachment 399239 [details]
Patch

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

It’s also probably worth running the tests in fast/forms/ios to make sure this doesn’t break any testing infrastructure.

> Source/WebKit/UIProcess/ios/forms/WKDateTimeInputControl.mm:58
> +    WKContentView* _view;

Nit - space before the *

[Megan Gardner]

Created attachment 399267 [details]
Patch

[Megan Gardner]

Created attachment 399272 [details]
Patch

[Megan Gardner]

<rdar://problem/63102524>

[Megan Gardner]

Created attachment 399301 [details]
Patch for landing

[EWS]

Committed r261658: <https://trac.webkit.org/changeset/261658>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399301 [details].