[webkit-reviews] review granted: [Bug 210611] [iOS] Add a way to focus a text input and place a caret : [Attachment 396673] Patch and tests

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 210611: [iOS] Add a way to focus a text input and place a caret
https://bugs.webkit.org/show_bug.cgi?id=210611

Attachment 396673: Patch and tests

https://bugs.webkit.org/attachment.cgi?id=396673&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 396673
  --> https://bugs.webkit.org/attachment.cgi?id=396673
Patch and tests

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5088
> +    // We ignore bounding rect changes as the bounding rect of the focused
element is not kept up-to-date.
> +    return self._hasFocusedElement && context &&
context._textInputContext.isSameElement(_focusedElementInformation.elementConte
xt);

Why does this method null-check context instead of asserting?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5093
> +    ASSERT(context);

Why does this method assert context instead of checking for null?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4291
> +    RefPtr<Element> target = elementForContext(elementContext);

I think that the modern approach for this would be to have elementForContext
return a RefPtr. Lacking that, then I suggest using auto x = makeRefPtr(y)
instead of RefPtr<ClassName> x = y so that the RefPtr gets the appropriate type
automatically instead of possible narrowing.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4292
> +    if (!target || !target->renderer()) {

Checking renderer for null without first updating layout is almost always a
programming mistake.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4298
> +    Ref<Frame> targetFrame = *target->document().frame();

Similarly here, newest style would be:

    auto targetFrame = makeRef(*target->document().frame());

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4305
> +    if (!didFocus || !target->renderer()) {

Checking renderer for null without first updating layout is almost always a
programming mistake.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4312
> +    VisiblePosition position =
visiblePositionInFocusedNodeForPoint(targetFrame, point, true /*
isInteractingWithFocusedElement */);

I suggest auto here.