Bug 236478 - Implement Reveal methods
Summary: Implement Reveal methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-10 17:49 PST by Megan Gardner
Modified: 2022-02-11 17:48 PST (History)
3 users (show)

See Also:


Attachments
Patch (21.18 KB, patch)
2022-02-10 19:11 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (21.31 KB, patch)
2022-02-10 19:52 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.33 KB, patch)
2022-02-10 20:09 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (21.68 KB, patch)
2022-02-11 15:53 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (21.68 KB, patch)
2022-02-11 17:10 PST, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2022-02-10 17:49:24 PST
Implement Reveal methods
Comment 1 Megan Gardner 2022-02-10 19:11:20 PST
Created attachment 451629 [details]
Patch
Comment 2 Megan Gardner 2022-02-10 19:52:58 PST
Created attachment 451634 [details]
Patch
Comment 3 Megan Gardner 2022-02-10 20:09:58 PST
Created attachment 451636 [details]
Patch
Comment 4 Wenson Hsieh 2022-02-11 09:29:55 PST
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))
);
```
Comment 5 Megan Gardner 2022-02-11 15:53:13 PST
Created attachment 451755 [details]
Patch
Comment 6 Megan Gardner 2022-02-11 17:10:03 PST
Created attachment 451760 [details]
Patch for landing
Comment 7 EWS 2022-02-11 17:47:46 PST
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].
Comment 8 Radar WebKit Bug Importer 2022-02-11 17:48:19 PST
<rdar://problem/88844376>