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

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.autoc
apitalizeType)];
> +	       [_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.