[webkit-reviews] review granted: [Bug 236627] Implement additional Reveeal methods. : [Attachment 451988] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 236627: Implement additional Reveeal methods.
https://bugs.webkit.org/show_bug.cgi?id=236627

Attachment 451988: Patch

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




--- Comment #5 from Tim Horton <thorton at apple.com> ---
Comment on attachment 451988
  --> https://bugs.webkit.org/attachment.cgi?id=451988
Patch

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

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:430
> -    if (!canCreateRevealItems() || !PAL::getRVPresenterClass())
> +    if (!canCreateRevealItems() || !PAL::getRVPresenterClass() ||
!PAL::isRevealFrameworkAvailable())

I think you want to put the Reveal framework short circuit /before/ you go
looking up RVPresenter, because RVPresenter's softlink macro has a
RELEASE_ASSERT in it.

> Source/WebCore/page/cocoa/EventHandlerCocoa.mm:54
> +    return VisibleSelection();

Perhaps `{ }` like above?

> Source/WebCore/page/cocoa/EventHandlerCocoa.mm:61
> +#endif // PLATFORM(IOS_FAMILY)

Comment seems to disagree with the condition.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7560
> +   
_page->prepareSelectionForContextMenuWithLocationInView(WebCore::roundedIntPoin
t(locationInView), [prepareSelectionForContextMenuHandler =
makeBlockPtr(completionHandler)](bool shouldPresentMenu, const
WebKit::RevealItem& item) {

`prepareSelectionForContextMenuHandler` seems like a slightly confusing name
for the captured completion handler, since it's not "handling"
`prepareSelectionForContextMenu`, but rather the completion callback. If you
search for `makeBlockPtr(completionHandler)` you'll see much prior art, ranging
from folks who literally use `completionHandler =
makeBlockPtr(completionHandler)`, shadowing the name, to folks who use slightly
adjusted names like `completionBlock` or `protectedCompletionHandler` or etc.
etc.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2347
> +RetainPtr<RVItem> WebPage::rvItemForCurrentSelection()

I know it's not new in this patch, but since you're renaming it, can we expand
"rv" to "reveal"? (Also I kind of wonder if it should just return a RevealItem,
but this is fine).

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2386
> +    eventHander.selectClosestContextualWordOrLinkFromHitTestResult(result,
WebCore::DontAppendTrailingWhitespace);
>      
> -    completionHandler(RevealItem(WTFMove(item)));
> +    completionHandler(true, RevealItem(rvItemForCurrentSelection()));

This all worked out handily!