Bug 129344 – [iOS WebKit2] Form controls handling

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

We need to implement the logic to show the appropriate keyboard for the selected input or to show the pickers and popover.

<rdar://problem/16053643>

[Enrica Casucci]

Created attachment 225197 [details]
Patch1

[WebKit Commit Bot]

Attachment 225197 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:753:  The parameter name "information" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/Shared/AssistedNodeInformation.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 225197 [details]
Patch1

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

> Source/WebKit2/Shared/AssistedNodeInformation.h:72
> +    WebCore::IntRect elementRect;

In what coordinate space? What about iframes?

> Source/WebKit2/Shared/AssistedNodeInformation.h:76
> +    bool hasNextNode;
> +    bool hasPreviousNode;
> +    bool isPasswordField;
> +    bool isAutocorrect;

Please group all the bools together (some lower down). for better packing.

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:352
> +    [m_contentView _startAssistingNode:information];

"information" seems a bit vague here.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:102
>      WebKit::WKAutoCorrectionData _autocorrectionData;
>      WebKit::InteractionInformationAtPosition _positionInformation;
> +    WebKit::AssistedNodeInformation _assistedNodeInformation;

I wish we didn't store all these by value. We should only allocate AssistedNodeInformation when a node is being assisted.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1447
> +    _traits.get().secureTextEntry = _assistedNodeInformation.isPasswordField;

I think our preferred style is [_traits.get setSecureTextEntry:...]

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1457
> +    _traits.get().shortcutConversionType = _assistedNodeInformation.isPasswordField ? UITextShortcutConversionTypeNo : UITextShortcutConversionTypeDefault;
> +    if (!_assistedNodeInformation.formAction.isEmpty())
> +        [_traits setReturnKeyType:(_assistedNodeInformation.elementType == WebKit::WKTypeSearch) ? UIReturnKeySearch : UIReturnKeyGo];
> +    if (_assistedNodeInformation.isPasswordField || _assistedNodeInformation.elementType == WKTypeEmail || _assistedNodeInformation.elementType == WKTypeURL || _assistedNodeInformation.formAction.contains("login")) {
> +        [_traits setAutocapitalizationType:UITextAutocapitalizationTypeNone];
> +        [_traits setAutocorrectionType:UITextAutocorrectionTypeNo];
> +    } else {
> +            [_traits setAutocapitalizationType:toUITextAutocapitalize(_assistedNodeInformation.autocapitalizeType)];
> +            [_traits setAutocorrectionType:_assistedNodeInformation.isAutocorrect ? UITextAutocorrectionTypeYes : UITextAutocorrectionTypeNo];
> +    }

Can we subclass UITextInputTraits and push some of this logic into it?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1500
> +    RefPtr<KeyboardEvent> key = KeyboardEvent::create();

Seems weird to have to create a fake key event to call nextFocusableElement() etc.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1511
> +    information.elementRect = m_assistedNode->renderer()->absoluteBoundingBoxRect();

Wrong for subframes?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1512
> +    Page* page = m_assistedNode->document().page();

We should get the Page from |this|

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1515
> +    if (!page) {
> +        information.hasNextNode = false;
> +        information.hasPreviousNode = false;

Does this ever happen?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1532
> +        information.isMultiSelect = element->multiple();
> +    } else if (isHTMLTextAreaElement(m_assistedNode.get())) {

Maybe early returns here.

[Simon Fraser (smfr)]

> I think our preferred style is [_traits.get setSecureTextEntry:...]

Sorry, I meant [_traits setSecureTextEntry:...]

[Joseph Pecoraro]

Comment on attachment 225197 [details]
Patch1

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

Looks good to me. I am not a WK2 Owner.

> Source/WebKit2/ChangeLog:11
> +        select the appropriate keybaord type based on the

Typo: "keybaord" => "keyboard"

> Source/WebKit2/Shared/AssistedNodeInformation.cpp:101
> +
> +

Nit: extra blank line

> Source/WebKit2/Shared/AssistedNodeInformation.h:31
> +#include <WebCore/WebAutocapitalize.h>
> +#include <WebCore/IntRect.h>

Nit: sort order

> Source/WebKit2/Shared/AssistedNodeInformation.h:39
> +    WKTypeText,

Should Password be a specific type instead of an "isPasswordField" boolean?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:370
> +    case WebKit::WKTypeDate:
> +    case WebKit::WKTypeDateTime:
> +    case WebKit::WKTypeDateTimeLocal:
> +    case WebKit::WKTypeTime:
> +        return !UICurrentUserInterfaceIdiomIsPad();

Needs Month type.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1449
> +    if (!_assistedNodeInformation.formAction.isEmpty())

Some newlines would really make this code easier to read!

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1456
> +            [_traits setAutocapitalizationType:toUITextAutocapitalize(_assistedNodeInformation.autocapitalizeType)];
> +            [_traits setAutocorrectionType:_assistedNodeInformation.isAutocorrect ? UITextAutocorrectionTypeYes : UITextAutocorrectionTypeNo];

Style: double indent.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1505
> +    Node* nextNode = isForward ? page->focusController().nextFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), startNode, key.get())
> +        : page->focusController().previousFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), startNode, key.get());
> +    while (nextNode && !isAssistableNode(startNode))
> +        nextNode = isForward ? page->focusController().nextFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), nextNode, key.get())
> +        : page->focusController().previousFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), nextNode, key.get());

I think this is supposed to be isAssistableNode(nextNode) not startNode.

How reducing the duplicated logic with a do/while.

    Node* nextNode = startNode;
    do {
        nextNode = isForward
            ? page->focusController().nextFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), nextNode, key.get())
            : page->focusController().previousFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), nextNode, key.get());
    } while (nextNode && !isAssistableNode(startNode)

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1554
> +            information.elementType = element->getAttribute("pattern") == "\\d*" || element->getAttribute("pattern") == "[0-9]*" ? WKTypeNumberPad : WKTypeNumber;

I think this should also happen for text inputs. If we get <input type="text" pattern="[0-9]*"> we should show a number pad. Not only for <input type="number">.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1574
> +        information.valueAsNumber = element->valueAsNumber();

Will we need to be careful about only encoding valueAsNumber when it is valid?

    document.createElement("input").valueAsNumber => NaN.

[Joseph Pecoraro]

I think I may have accidentally erased smfr's r+ submitting the review form.

[Enrica Casucci]

(In reply to comment #3)
> 
> In what coordinate space? What about frames?
The idea is to have everything in view coordinates.
> 
> I wish we didn't store all these by value. We should only allocate AssistedNodeInformation when a node is being assisted.

It is possible. There is only one per view though.

> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1447
> > +    _traits.get().secureTextEntry = _assistedNodeInformation.isPasswordField;
> 
> I think our preferred style is [_traits.get setSecureTextEntry:...]

I'll change it.
 
> 
> Can we subclass UITextInputTraits and push some of this logic into it?
Not sure how to do this. UITextInputTraits is a protocol that the view needs to implement.

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1500
> > +    RefPtr<KeyboardEvent> key = KeyboardEvent::create();
> 
> Seems weird to have to create a fake key event to call nextFocusableElement() etc.

FocusController::nextFocusableElement and FocusController::previousFocusableElement internally dereference the KeyboardEvent* parameter. Don't see any other way to do it.

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1511
> > +    information.elementRect = m_assistedNode->renderer()->absoluteBoundingBoxRect();
> 
> Wrong for subframes?
> 
I'll add the mapping. The rect needs to be in view coordinates.
> Does this ever happen?

No I'll remove it.

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1532
> > +        information.isMultiSelect = element->multiple();
> > +    } else if (isHTMLTextAreaElement(m_assistedNode.get())) {
> 
> Maybe early returns here.

[Enrica Casucci]

Thanks for the review!!
> 
> > Source/WebKit2/Shared/AssistedNodeInformation.h:39
> > +    WKTypeText,
> 
> Should Password be a specific type instead of an "isPasswordField" boolean?
Yes, I'll change it.
> 
> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:370
> > +    case WebKit::WKTypeDate:
> > +    case WebKit::WKTypeDateTime:
> > +    case WebKit::WKTypeDateTimeLocal:
> > +    case WebKit::WKTypeTime:
> > +        return !UICurrentUserInterfaceIdiomIsPad();
> 
> Needs Month type.
I'll double check WK1. I didn't think we were using month, but I might be wrong.
> 
> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1505
> > +    Node* nextNode = isForward ? page->focusController().nextFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), startNode, key.get())
> > +        : page->focusController().previousFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), startNode, key.get());
> > +    while (nextNode && !isAssistableNode(startNode))
> > +        nextNode = isForward ? page->focusController().nextFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), nextNode, key.get())
> > +        : page->focusController().previousFocusableElement(FocusNavigationScope::focusNavigationScopeOf(&startNode->document()), nextNode, key.get());
> 
> I think this is supposed to be isAssistableNode(nextNode) not startNode.
> 
You are absolutely right. I had already fixed it.

> How reducing the duplicated logic with a do/while.
> 
Much better!

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1554
> > +            information.elementType = element->getAttribute("pattern") == "\\d*" || element->getAttribute("pattern") == "[0-9]*" ? WKTypeNumberPad : WKTypeNumber;
> 
> I think this should also happen for text inputs. If we get <input type="text" pattern="[0-9]*"> we should show a number pad. Not only for <input type="number">.
> 
I'll fix it.

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1574
> > +        information.valueAsNumber = element->valueAsNumber();
> 
> Will we need to be careful about only encoding valueAsNumber when it is valid?
> 
>     document.createElement("input").valueAsNumber => NaN.
I'll double check what we do for WK1.

[Sam Weinig]

Comment on attachment 225197 [details]
Patch1

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

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:50
> +struct AssistedNodeInformation;

I don't think you need to forward declare this since you are #importing it.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1484
> +static inline bool isAssistableNode(Node* node)

Pass the node as a Node&

[Enrica Casucci]

First patch landed.
Committed revision 164690.

[Enrica Casucci]

Created attachment 225311 [details]
Patch2

[WebKit Commit Bot]

Attachment 225311 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKFormPopover.h:32:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKFormPopover.h:44:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKFormPopover.h:45:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKFormPopover.h:46:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 4 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 225311 [details]
Patch2

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
> +@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?

[Joseph Pecoraro]

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

Looks good to me.

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

Style: No need for "> >" it can be ">>".

> Source/WebKit2/UIProcess/ios/WKFormInputControl.mm:29
> +#import "config.h"
> +#import "WKContentView.h"
> +#import "WKContentViewInteraction.h"
> +#import "WKFormInputControl.h"

Isn't the usual style:

    #import "config.h"
    #import "WKFormInputControl.h"

    #import ...

> Source/WebKit2/UIProcess/ios/WKFormInputControl.mm:244
> +        case WebKit::WKTypeDate:
> +            mode = UIDatePickerModeDate;
> +            break;
> +        case WebKit::WKTypeDateTimeLocal:
> +            mode = UIDatePickerModeDateAndTime;
> +            break;

Nit: newline between these cases like the other cases?

> Source/WebKit2/UIProcess/ios/WKFormInputControl.mm:345
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> +    UIBarButtonItem *clearButton = [[[UIBarButtonItem alloc] initWithTitle:clearString style:UIBarButtonItemStyleBordered target:self action:@selector(clear:)] autorelease];
> +#pragma clang diagnostic pop

Gross =(.

[Enrica Casucci]

Thanks for the review.
> Maybe the files should be called WKFormInputControls?
Not sure I agree. It handles HTMLInput control with different types.
> 
> > 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 ?
I can do that.
> 
> Can this have a better name? What is a peripheral?
Peripheral is the UIKit terminology for input device.

> 
> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:139
> > +- (void)accessoryDone;
> 
> Is this a protocol method or can it have a better name (e.g. formAccessoryDone)?
I prefer to keep the name the same as the WebKit1 version. It makes it easier to find the corresponding implementation.
> 
> 
> What's the ownership of this string?

It only points to constant strings.

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

Good idea.
 
> > Source/WebKit2/UIProcess/ios/WKFormPopover.h:36
> > +    id <WKRotatingPopoverDelegate> _dismissDelegate;
> 
> "_dismissDelegate" sounds like a command. dismissionDelegate?
Sure.

> 
> > Source/WebKit2/UIProcess/ios/WKFormPopover.h:54
> > +@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?
Yes. for iPad the date/time pickers are inside a popover. We need to handle the device rotation to hide the popover when rotation begins and show it at the new position and potentially with a different arrow direction when done.

[Enrica Casucci]

Committed revision 164760.

[Enrica Casucci]

Created attachment 225579 [details]
Patch3

Adds support for select elements on iOS.

[Tim Horton]

Comment on attachment 225579 [details]
Patch3

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

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

I (think) we prefer [_object setPopover:self]; in the smartptr message passing case.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:55
> +#import <WebCore/HTMLOptGroupElement.h>
> +#import <WebCore/HTMLOptionElement.h>
>  #import <WebCore/HTMLInputElement.h>

these don't look sorted correctly

[Simon Fraser (smfr)]

Comment on attachment 225579 [details]
Patch3

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.

[Darin Adler]

Comment on attachment 225579 [details]
Patch3

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

> Source/WebCore/WebCore.exp.in:262
>  __ZNK7WebCore11HTMLElement5titleEv
> +__ZN7WebCore9HTMLNames11optgroupTagE
> +__ZNK7WebCore19HTMLOptGroupElement14groupLabelTextEv
> +__ZN7WebCore17HTMLOptionElement8selectedEv
> +__ZN7WebCore9HTMLNames12disabledAttrE
> +__ZN7WebCore17HTMLSelectElement20optionSelectedByUserEibb
>  __ZNK7WebCore17HTMLOptionElement4textEv

To make merges of this file easier, it’s much better if we keep these lists of symbols sorted in the order that the command-line sort tool does, rather than just putting new symbols in a block together. The existing symbols seem to be sorted. What I do is add my symbols, then select the text and copy, then "pbpaste | sort | pbcopy", then paste the symbols back in.

[Enrica Casucci]

Comment on attachment 225579 [details]
Patch3

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

Thanks for the review! I added some replies to your comments.

>> Source/WebCore/WebCore.exp.in:262
>>  __ZNK7WebCore17HTMLOptionElement4textEv
> 
> To make merges of this file easier, it’s much better if we keep these lists of symbols sorted in the order that the command-line sort tool does, rather than just putting new symbols in a block together. The existing symbols seem to be sorted. What I do is add my symbols, then select the text and copy, then "pbpaste | sort | pbcopy", then paste the symbols back in.

Thanks! I'll do that.

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

I need this to be readwrite because, if we have a multi-select, we want to update the information on the UIProcess side, without querying a new state from the WebProcess.
I preferred to leave the existing assistedNodeInformation as readonly and expose only the select option part as writable.
I'll move it down, as you suggested.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1812
>> +- (Vector<WKOptionItem>&) assistedNodeSelectOptions
> 
> No space after the )
> Should this return a non-const reference?

Yes, see the comment above.

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

This is consistent with the way we handle the input controls. Why did you find it ok for those and not this one?

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

I agree, I'll change it

>> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:60
>> +    self.dataSource = self;
> 
> Is the dataSource retained? Is this creating a retain cycle?

This is the same model used by the WebKit1 implementation. I believe this is ok.

>> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:68
>> +        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&

Sure, I forgot.

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

I'll use the index here too.

>> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:112
>> +    WKSelectPopover *_popover;
> 
> What's the ownership model?

WKSelectPopover has a RetainPtr to WKSelectTableViewController which in turn holds a weak reference to WKSelectPopover.

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

Why? It is autoreleased.

[Enrica Casucci]

Comment on attachment 225579 [details]
Patch3

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

>>> 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?
> 
> I agree, I'll change it

Actually this is leftover of me trying to implement the multi select picker. I only need _selectedIndex here.

[Enrica Casucci]

Committed revision 164955.

[Simon Fraser (smfr)]

> >> 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).
> 
> This is consistent with the way we handle the input controls. Why did you find it ok for those and not this one?

Maybe I didn't notice! Use of "create" is fairly rare in Obj-C, and doing it this way is more likely to result in mistakes I think.

> >> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:60
> >> +    self.dataSource = self;
> > 
> > Is the dataSource retained? Is this creating a retain cycle?

Could you check?

> >> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:112
> >> +    WKSelectPopover *_popover;
> > 
> > What's the ownership model?
> 
> WKSelectPopover has a RetainPtr to WKSelectTableViewController which in turn holds a weak reference to WKSelectPopover.

I think it's good to document weak refs with comments (or the 'weak' keyword) to make ownership clear.

> >> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:260
> >> +        cell = [[[UITableViewCell alloc] initWithStyle:UITableViewCellStyleDefault reuseIdentifier:WKPopoverTableViewCellReuseIdentifier] autorelease];
> > 
> > This is leaked.
> 
> Why? It is autoreleased.

Sorry, I misread.

[Joseph Pecoraro]

Comment on attachment 225579 [details]
Patch3

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

>>> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:60
>>> +    self.dataSource = self;
>> 
>> Is the dataSource retained? Is this creating a retain cycle?
> 
> This is the same model used by the WebKit1 implementation. I believe this is ok.

A dataSource is just like a delegate. They are both (assign) UIPickerView properties.

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPicker.mm:131
> +    //  First strip all white space off the end of the range

Weird double space in comments here.

[Enrica Casucci]

Created attachment 225892 [details]
Patch4

[WebKit Commit Bot]

Attachment 225892 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/forms/WKFormSelectControl.h:34:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 225892 [details]
Patch4

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
> +@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

[Enrica Casucci]

The work is now complete
Committed revision 165116.