Bug 226383 – Adopt Reveal for phone numbers

• Component: Platform
• Status: RESOLVED FIXED

[Dana Estra]

rdar://78482517

[Radar WebKit Bug Importer]

<rdar://problem/78626007>

[Dana Estra]

Created attachment 430034 [details]
Patch

[Aditya Keerthi]

Comment on attachment 430034 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Covered by existing tests.

I don't think we have tests for this :)

You can explain how you manually tested instead.

> Source/WebKit/Platform/mac/MenuUtilities.mm:109
> +
> +

Remove this whitespace.

> Source/WebKit/Platform/mac/MenuUtilities.mm:119
> +    auto viewForPresenter = [[NSView alloc] init];

These should all use `adoptNS()` to avoid leaks.

[Wenson Hsieh]

Comment on attachment 430034 [details]
Patch

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

It looks like we'll need to add a declaration for `-initWithURL:rangeInContext:` in PAL's RevealSPI.h as well, to fix the non-internal macOS build.

> Source/WebKit/Platform/mac/MenuUtilities.mm:112
>      if (!DataDetectorsLibrary())

It's probably a good idea to change this to `!PAL::isRevealFrameworkAvailable() || !PAL::isRevealCoreFrameworkAvailable()` instead, since the new code uses Reveal instead of DataDetectors.

[Dana Estra]

Created attachment 430051 [details]
Patch

[Dana Estra]

Created attachment 430063 [details]
Patch

[Aditya Keerthi]

Comment on attachment 430063 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Adopted reveal for phone numbers

Nit: "Adopt reveal for phone numbers".

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=226383

You can include the radar URL below this line.

> Source/WebKit/ChangeLog:6
> +        Reviewed by Aditya Keerthi and Wenson Hsieh.

This should be left as NOBODY (OOPS!), until you've received an "r+".

> Source/WebKit/Platform/mac/MenuUtilities.mm:121
> +    auto context = adoptNS([PAL::allocRVPresentingContextInstance() initWithPointerLocationInView:NSZeroPoint inView:(NSView *)viewForPresenter highlightDelegate:(id<RVPresenterHighlightDelegate>)delegate]);

This should be `viewForPresenter.get()` and `delegate.get()`, not casted.

> Source/WebKit/Platform/mac/MenuUtilities.mm:122
> +    NSArray *proposedMenuItems = [presenter menuItemsForItem:(RVItem *)item documentContext:nil presentingContext:(RVPresentingContext *)context options:nil];

`.get()` instead of casting.

[Tim Horton]

Comment on attachment 430063 [details]
Patch

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

>> Source/WebKit/ChangeLog:3
>> +        Adopted reveal for phone numbers
> 
> Nit: "Adopt reveal for phone numbers".

also probably capital-R Reveal since it is the name of a framework.

[Dana Estra]

Created attachment 430273 [details]
Patch

[Aditya Keerthi]

Comment on attachment 430273 [details]
Patch

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

Looks good overall! Perhaps Wenson or Tim can r+.

> Source/WebCore/PAL/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=226383

Nit: Missing radar.

> Source/WebKit/ChangeLog:3
> +        Adopted Reveal for phone numbers

Nit: "Adopt".

[Dana Estra]

Created attachment 430281 [details]
Patch

[Dana Estra]

Created attachment 430284 [details]
Patch

[Tim Horton]

Comment on attachment 430284 [details]
Patch

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

> Source/WebKit/Platform/mac/MenuUtilities.mm:121
> +    auto context = adoptNS([PAL::allocRVPresentingContextInstance() initWithPointerLocationInView:NSZeroPoint inView:viewForPresenter.get() highlightDelegate:delegate.get()]);

I wonder if we should ask Reveal folks (separately from this bug) for API that doesn't take a view/point (or, if we do so and they tell us why that's a bad idea, we'll understand why faking it like this is bad).

[Tim Horton]

Comment on attachment 430284 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Manually tested by selecting text containing phone numbers in Safari and viewing dropdown menu.

Some more words about the /motivation/ would be good

[Dana Estra]

Created attachment 430292 [details]
Patch

[EWS]

Committed r278330 (238363@main): <https://commits.webkit.org/238363@main>

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

[Dana Estra]

Reopening to attach new patch.

[Dana Estra]

Created attachment 431358 [details]
Patch