Bug 236478 – Implement Reveal methods

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Implement Reveal methods

[Megan Gardner]

Created attachment 451629 [details]
Patch

[Megan Gardner]

Created attachment 451634 [details]
Patch

[Megan Gardner]

Created attachment 451636 [details]
Patch

[Wenson Hsieh]

Comment on attachment 451636 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/RevealItem.h:27
> +#ifndef RevealItem_h
> +#define RevealItem_h

Nit - `#pragma once` in new code.

> Source/WebKit/Shared/Cocoa/RevealItem.h:44
> +    RetainPtr<RVItem> m_item;

This should be a private member.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:38
> +#import <wtf/Span.h>

Is this needed now that we're including `Span.h` in ArgumentCoders.h?

> Source/WebKit/UIProcess/WebPageProxy.h:880
> +    void requestRVItemInCurrentSelectedRange(CompletionHandler<void(const WebKit::RevealItem&)>&&);

Nit - you can omit the WebKit:: here.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7382
> +- (void)requestRVItemInSelectedRangeWithCompletionHandler:(void(^)(RVItem * item))completionHandler

Nit - extra space before "item"

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7384
> +    _page->requestRVItemInCurrentSelectedRange([revealItemSelectionHandler = makeBlockPtr(completionHandler)](const WebKit::RevealItem& item) {

Nit - you can omit the WebKit:: here too.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2342
> +        NSRange selectionRange = NSMakeRange(0, 0);

Nit - it seems like you can remove this line (and just do `auto selectionRange = NSMakeRange(characterCount(*makeSimpleRange(fullCharacterRange->start, selectionStart)), characterCount(*makeSimpleRange(selectionStart, selectionEnd)));` below)

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2354
> +                selectionRange = NSMakeRange(characterCount(*makeSimpleRange(fullCharacterRange->start, selectionStart)),
> +                characterCount(*makeSimpleRange(selectionStart, selectionEnd)));

This looks a bit confusing (I thought it was two separate lines at first). Perhaps something like this instead?

```
auto selectionRange = NSMakeRange(
    characterCount(*makeSimpleRange(fullCharacterRange->start, selectionStart)),
    characterCount(*makeSimpleRange(selectionStart, selectionEnd))
);
```

[Megan Gardner]

Created attachment 451755 [details]
Patch

[Megan Gardner]

Created attachment 451760 [details]
Patch for landing

[EWS]

Committed r289685 (247171@main): <https://commits.webkit.org/247171@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/88844376>