[webkit-reviews] review granted: [Bug 129344] [iOS WebKit2] Form controls handling : [Attachment 225311] Patch2

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 129344: [iOS WebKit2] Form controls handling
https://bugs.webkit.org/show_bug.cgi?id=129344

Attachment 225311: Patch2
https://bugs.webkit.org/attachment.cgi?id=225311&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225311&action=review


Seems OK but I would like to see some naming improvements, and preferably file
moves.

> Source/WebKit2/ChangeLog:25
> +	   * UIProcess/ios/WKFormInputControl.h: Added.
> +	   * UIProcess/ios/WKFormInputControl.mm: Added.

Maybe the files should be called WKFormInputControls?

> Source/WebKit2/ChangeLog:54
> +	   * UIProcess/ios/WKFormPeripheral.h: Added.
> +	   * UIProcess/ios/WKFormPopover.h: Added.
> +	   * UIProcess/ios/WKFormPopover.mm: Added.

Lots of new form-related files. Maybe make UIProcess/ios/forms ?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:104
> +    RetainPtr<NSObject<WKFormPeripheral> > _inputPeripheral;

Can this have a better name? What is a peripheral?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:139
> +- (void)accessoryDone;

Is this a protocol method or can it have a better name (e.g.
formAccessoryDone)?

> Source/WebKit2/UIProcess/ios/WKFormInputControl.mm:50
> +// WKDateTimePicker

Comment is not useful.

> Source/WebKit2/UIProcess/ios/WKFormInputControl.mm:53
> +    NSString *_formatString;

What's the ownership of this string?

> Source/WebKit2/UIProcess/ios/WKFormInputControl.mm:62
> +// WKDateTimePopover

Ditto.

> Source/WebKit2/UIProcess/ios/WKFormInputControl.mm:87
> +    self = [super init];
> +    if (!self)
> +	   return nil;

We usually do if ((self == [super init])) {...}

> Source/WebKit2/UIProcess/ios/WKFormInputControl.mm:92
> +    _formatString = nil;

No need.

> Source/WebKit2/UIProcess/ios/WKFormInputControl.mm:206
> +	   RetainPtr<NSLocale> englishLocale = adoptNS([[NSLocale alloc]
initWithLocaleIdentifier:@"en_US_POSIX"]);
> +	   RetainPtr<NSDateFormatter> dateFormatter = adoptNS([[NSDateFormatter
alloc] init]);
> +	   [dateFormatter setTimeZone:_datePicker.get().timeZone];
> +	   [dateFormatter setDateFormat:_formatString];
> +	   [dateFormatter setLocale:englishLocale.get()];

Maybe share with the previous copy of this code?

> Source/WebKit2/UIProcess/ios/WKFormPopover.h:36
> +    id <WKRotatingPopoverDelegate> _dismissDelegate;

"_dismissDelegate" sounds like a command. dismissionDelegate?

> Source/WebKit2/UIProcess/ios/WKFormPopover.h:54
> + at interface WKFormRotatingAccessoryPopover : WKRotatingPopover
<WKRotatingPopoverDelegate>

"form rotating accessory popover" is hard to parse. I can't tell what this
does. Is it related to device rotation?