[webkit-reviews] review granted: [Bug 236478] Implement Reveal methods : [Attachment 451636] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 236478: Implement Reveal methods
https://bugs.webkit.org/show_bug.cgi?id=236478

Attachment 451636: Patch

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




--- Comment #4 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 451636
  --> https://bugs.webkit.org/attachment.cgi?id=451636
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))
);
```