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

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

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


> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:120
> +- (Vector<WebKit::WKOptionItem>&) assistedNodeSelectOptions;

Why not make this a readonly property?

If you want it as a method, please move it lower down.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1812
> +- (Vector<WKOptionItem>&) assistedNodeSelectOptions

No space after the )
Should this return a non-const reference?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectControl.h:40
> +- (id)initWithView:(WKContentView *)view hasGroups:(BOOL)hasGroups;

Return instancetype?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectControl.h:45
> +- (id)initWithView:(WKContentView *)view;

instancetype?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectControl.mm:73
> ++ (WKFormSelectControl *)createPeripheralWithView:(WKContentView *)view

It would be more idiomatic to call this +peripheralWithView and have it return
an autoreleased object (don't forget to fix the callers).

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:49
> +    WKOptionItem* _optionToSelectWhenDone;

I don't really like holding a pointer to an object in an array that's held by
some other class. Can we store its index instead?

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

Is the dataSource retained? Is this creating a retain cycle?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:68
> +    for (size_t i = 0; i <
_view.assistedNodeInformation.selectOptions.size(); ++i) {
> +	   WKOptionItem* item = &[_view assistedNodeSelectOptions][i];

Really confusing that the first line uses
_view.assistedNodeInformation.selectOptions and the second [_view
assistedNodeSelectOptions] which I think are the same?

Use const WKOptionItem&

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:144
> +	   return [string substringWithRange: r];

No space after :

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:149
> +- (NSInteger)pickerView:(UIPickerView *)aPickerView
numberOfRowsInComponent:(NSInteger)aColumnIndex

aPickerView -> pickerView, aColumnIndex -> columnIndex

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:156
> +    const WKOptionItem& option = [_view assistedNodeSelectOptions][row];

I think you should check that 'row' is in bounds here.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:168
> +    const WKOptionItem& newSelectedOption = [_view
assistedNodeSelectOptions][row];

Ditto.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:200
> +    _optionToSelectWhenDone = &[_view assistedNodeSelectOptions][row];

Nasty.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:50
> +static NSString *WKPopoverTableViewCellReuseIdentifier  =
@"WKPopoverTableViewCellReuseIdentifier";

static NSString* const Foo

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:77
> +    unichar directionalFormattingCharacter = (writingDirection ==
UITextWritingDirectionLeftToRight) ? (override ? leftToRightOverride :
leftToRightEmbedding)
> +    : (override ? rightToLeftOverride : rightToLeftEmbedding);

Hard to understand.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:112
> +    WKSelectPopover *_popover;

What's the ownership model?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:260
> +	   cell = [[[UITableViewCell alloc]
initWithStyle:UITableViewCellStyleDefault
reuseIdentifier:WKPopoverTableViewCellReuseIdentifier] autorelease];

This is leaked.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:407
> +    CGFloat titleHeight = 0.0;

= 0

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:439
> +    _tableViewController.get().popover = nil;

[_tableViewController.get setPopover:nil] etc

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:443
> +    _tableViewController = nil;

No point doing this.