Bug 236627 – Implement additional Reveal methods.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Implement additional Reveeal methods.

[Megan Gardner]

Created attachment 451977 [details]
Patch

[Megan Gardner]

Created attachment 451978 [details]
Patch

[Megan Gardner]

Created attachment 451986 [details]
Patch

[Megan Gardner]

Created attachment 451988 [details]
Patch

[Tim Horton]

Comment on attachment 451988 [details]
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::roundedIntPoint(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!

[Tim Horton]

Comment on attachment 451988 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Implement additional Reveeal methods.

"Reveeal"

[Megan Gardner]

Created attachment 452033 [details]
Patch for landing

[EWS]

Committed r289818 (247279@main): <https://commits.webkit.org/247279@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452033 [details].

[Radar WebKit Bug Importer]

<rdar://problem/88972256>