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

[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 225892: Patch4
https://bugs.webkit.org/attachment.cgi?id=225892&action=review

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


> Source/WebKit2/UIProcess/ios/forms/WKFormSelectControl.h:34
> +extern CGFloat adjustedFontSize(CGFloat textWidth, UIFont *font, CGFloat
initialFontSize, const Vector<WebKit::WKOptionItem>& items);

Does this need the 'extern'?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectControl.h:51
> + at interface WKSelectMultiplePicker : UIPickerView <WKFormControl,
UIPickerViewDataSource, UIPickerViewDelegate>

SelectMultiple or MultipleSelect?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:52
> +static NSString *stringByTrimmingWhitespaceAndNewlines(NSString *string)

Can this just use CFTrimWhitespace (which says it removes newlines too)?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:85
> +- (instancetype)initCommon;

Why not just -init?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:101
> +    [self.titleLabel setLineBreakMode:NSLineBreakByTruncatingMiddle];

Should not use property syntax in init and dealloc methods.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:114
> +    self.titleLabel.text = stringByTrimmingWhitespaceAndNewlines(item.text);

> +    self.checked = item.isSelected;
> +    self.disabled = item.disabled;
> +    if (self.disabled)
> +	   self.titleLabel.textColor = [UIColor colorWithWhite:0.0
alpha:DisabledOptionAlpha];

Ditto.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:136
> +    self.titleLabel.text = stringByTrimmingWhitespaceAndNewlines(item.text);

> +    self.checked = NO;
> +    self.titleLabel.textColor = [UIColor colorWithWhite:0.0 alpha:0.5];
> +    self.disabled = YES;

Ditto

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:163
> +    float _layoutWidth;

Why not a CGFloat?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:178
> +    self.autoresizingMask = UIViewAutoresizingFlexibleWidth |
UIViewAutoresizingFlexibleHeight;
> +    self.dataSource = self;
> +    self.delegate = self;

Property access.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:200
> +	   if (!_allowsMultipleSelection && item.isSelected) {
> +	       _singleSelectionIndex = currentIndex;
> +	   }

No braces.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:213
> +    self.dataSource = nil;
> +    self.delegate = nil;

property access