[webkit-reviews] review granted: [Bug 127015] Support WebSelections in WK2 on iOS : [Attachment 221300] Patch2

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 127015: Support WebSelections in WK2 on iOS
https://bugs.webkit.org/show_bug.cgi?id=127015

Attachment 221300: Patch2
https://bugs.webkit.org/attachment.cgi?id=221300&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=221300&action=review


> Source/WebKit2/ChangeLog:4
> +	   Support WebSelections in WK2 on iOS.
> +	   https://bugs.webkit.org/show_bug.cgi?id=127015

Awesome changelog!

> Source/WebKit2/ChangeLog:11
> +	   gesture recognizers are enabled and empty stubs for the gestures

and add empty stubs?

> Source/WebKit2/ChangeLog:37
> +	       We are then able tp make an informed decision about whether

Typo: tp -> to

> Source/WebKit2/Shared/PositionInformation.h:37
> +struct PositionInformation {

InteractionInformationAtPosition maybe?

> Source/WebKit2/Shared/PositionInformation.h:40
> +	   , wantsHighlight(true)

Shouldn't this be false by default?

Is it still needed?

> Source/WebKit2/Shared/PositionInformation.h:44
> +    WebCore::IntPoint point;

Maybe we should pass a floating point and round in the WebProcess?
I have the dream some day we will handle input on floating points position :)

(Not important for landing this).

> Source/WebKit2/Shared/PositionInformation.h:45
> +    bool isSameAssistedNode;

Maybe "nodeAtPositionIsSameAssistedNode"

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:39
> +#import <UIKit/UIGestureRecognizer_Private.h>
>  #import <UIKit/UIFont_Private.h>

Include order.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:424
> +    if (_positionInformation.clickableElementName ==
String(ASCIILiteral("IMG")))
> +	   return @selector(_showImageSheet);
> +    else if (_positionInformation.clickableElementName ==
String(ASCIILiteral("A")))
> +	   return @selector(_showLinkSheet);

Did you need the String() with ASCIILiteral()? If so, I should fix operator==.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:817
> +	   const HTMLElement* element = toHTMLElement(hitNode);
> +	   if (!element)

Is this right? IIRC, toHTMLElement will just static_cast. On the other hand,
hitNode is already check for nullity above, and could be a SVN element.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:834
> +	   if (element->hasLocalName(HTMLNames::appletTag) ||
element->hasLocalName(HTMLNames::embedTag) ||
element->hasLocalName(HTMLNames::objectTag))
> +	       info.wantsHighlight = false;
> +	   else if (element->renderer() && element->renderer()->isImage()) {
> +	       URL url =
toRenderImage(element->renderer())->cachedImage()->url();
> +	       if (!url.string().isNull()) {
> +		   info.wantsHighlight = true;
> +		   info.url = url.string();
> +	       }
> +	   } else if (element->isLink()) {
> +	       info.wantsHighlight = true;
> +	       info.url = element->getAttribute(HTMLNames::hrefAttr).string();
> +	   } else if (isHTMLSelectElement(hitNode))
> +	       info.wantsHighlight = true;
> +	   else
> +	       info.wantsHighlight = hitNode->isContentEditable();

Why not set wantsHighlight to yet in all of those cases?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:853
> +

You can remove the empty line.