[webkit-reviews] review granted: [Bug 236289] WKHoverPlatter should scale up the platter content to make it easier to see : [Attachment 451223] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 236289: WKHoverPlatter should scale up the platter content to make it
easier to see
https://bugs.webkit.org/show_bug.cgi?id=236289

Attachment 451223: Patch

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




--- Comment #3 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 451223
  --> https://bugs.webkit.org/attachment.cgi?id=451223
Patch

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

r=me with tvOS/watchOS builds fixed!

> Source/WebKit/UIProcess/WebPageProxy.cpp:11159
> +void WebPageProxy::interactableRegionsInRect(WebCore::FloatRect rect,
CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler)

Maybe give this a name that better describes the coordinate space? Given the
usage, it looks like it's in root view space.

(Also, you can remove the WebCore:: here)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3107
> +    if (WKHoverPlatterDomain.rootSettings.enabled)
> +	   return [_hoverPlatter adjustedPointForPoint:location];

Nit - maybe it would it be cleaner if we folded these
`WKHoverPlatterDomain.rootSettings.enabled` checks into
-adjustedPointForPoint:?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3117
> +    if (WKHoverPlatterDomain.rootSettings.enabled)
> +	   return [_hoverPlatter adjustedPointForPoint:location];

(Ditto)

> Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:122
> +    if (!boundingRect.contains(point) && point != WebCore::FloatPoint { -1 ,
-1 })

Nit - extra space before the ,

(It's also not super obvious where the (-1, -1) is coming from..)

> Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:300
> +    delta.scale(1.f / WKHoverPlatterDomain.rootSettings.platterScale);

`WKHoverPlatterDomain.rootSettings.platterScale` is always guaranteed to be
nonzero, right?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6268
> +void WebPage::interactableRegionsInRect(WebCore::FloatRect rect,
CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler)

Nit - I think you can omit the WebCore:: here

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6275
> +    auto result = HitTestResult { WebCore::LayoutRect(rect) };

(here too)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6289
> +    HitTestResult::NodeSet linkNodes;

Nit - maybe `interactableNodes` or `hitTestedNodes`? (linkNodes makes it sounds
a bit like it's only about links)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6301
> +	   rects.append(renderer->absoluteBoundingBoxRect());

(Just to double check — we don't need to convert to root view coordinates here
because all the hit-tested nodes are in the main frame?)