RESOLVED FIXED 226383
Adopt Reveal for phone numbers
https://bugs.webkit.org/show_bug.cgi?id=226383
Summary Adopt Reveal for phone numbers
Dana Estra
Reported 2021-05-28 11:29:47 PDT
Attachments
Patch (3.39 KB, patch)
2021-05-28 11:42 PDT, Dana Estra
no flags
Patch (4.07 KB, patch)
2021-05-28 14:24 PDT, Dana Estra
no flags
Patch (5.38 KB, patch)
2021-05-28 15:46 PDT, Dana Estra
no flags
Patch (5.33 KB, patch)
2021-06-01 09:54 PDT, Dana Estra
no flags
Patch (941 bytes, patch)
2021-06-01 12:34 PDT, Dana Estra
no flags
Patch (5.37 KB, patch)
2021-06-01 13:14 PDT, Dana Estra
no flags
Patch (5.53 KB, patch)
2021-06-01 14:53 PDT, Dana Estra
no flags
Patch (26.15 KB, patch)
2021-06-14 13:09 PDT, Dana Estra
no flags
Radar WebKit Bug Importer
Comment 1 2021-05-28 11:30:15 PDT
Dana Estra
Comment 2 2021-05-28 11:42:13 PDT
Aditya Keerthi
Comment 3 2021-05-28 11:53:23 PDT
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 4 2021-05-28 12:25:46 PDT
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
Comment 5 2021-05-28 14:24:13 PDT
Dana Estra
Comment 6 2021-05-28 15:46:18 PDT
Aditya Keerthi
Comment 7 2021-05-28 16:51:34 PDT
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 8 2021-05-28 16:56:26 PDT
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
Comment 9 2021-06-01 09:54:52 PDT
Aditya Keerthi
Comment 10 2021-06-01 11:53:24 PDT
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
Comment 11 2021-06-01 12:34:48 PDT
Dana Estra
Comment 12 2021-06-01 13:14:26 PDT
Tim Horton
Comment 13 2021-06-01 13:57:37 PDT
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 14 2021-06-01 13:58:10 PDT
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
Comment 15 2021-06-01 14:53:59 PDT
EWS
Comment 16 2021-06-01 15:36:21 PDT
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
Comment 17 2021-06-14 13:09:56 PDT
Reopening to attach new patch.
Dana Estra
Comment 18 2021-06-14 13:09:58 PDT
Note You need to log in before you can comment on or make changes to this bug.