rdar://78482517
<rdar://problem/78626007>
Created attachment 430034 [details] Patch
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.
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.
Created attachment 430051 [details] Patch
Created attachment 430063 [details] Patch
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.
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.
Created attachment 430273 [details] Patch
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".
Created attachment 430281 [details] Patch
Created attachment 430284 [details] Patch
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).
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
Created attachment 430292 [details] Patch
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].
Reopening to attach new patch.
Created attachment 431358 [details] Patch