WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
rdar://78482517
Attachments
Patch
(3.39 KB, patch)
2021-05-28 11:42 PDT
,
Dana Estra
no flags
Details
Formatted Diff
Diff
Patch
(4.07 KB, patch)
2021-05-28 14:24 PDT
,
Dana Estra
no flags
Details
Formatted Diff
Diff
Patch
(5.38 KB, patch)
2021-05-28 15:46 PDT
,
Dana Estra
no flags
Details
Formatted Diff
Diff
Patch
(5.33 KB, patch)
2021-06-01 09:54 PDT
,
Dana Estra
no flags
Details
Formatted Diff
Diff
Patch
(941 bytes, patch)
2021-06-01 12:34 PDT
,
Dana Estra
no flags
Details
Formatted Diff
Diff
Patch
(5.37 KB, patch)
2021-06-01 13:14 PDT
,
Dana Estra
no flags
Details
Formatted Diff
Diff
Patch
(5.53 KB, patch)
2021-06-01 14:53 PDT
,
Dana Estra
no flags
Details
Formatted Diff
Diff
Patch
(26.15 KB, patch)
2021-06-14 13:09 PDT
,
Dana Estra
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-05-28 11:30:15 PDT
<
rdar://problem/78626007
>
Dana Estra
Comment 2
2021-05-28 11:42:13 PDT
Created
attachment 430034
[details]
Patch
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
Created
attachment 430051
[details]
Patch
Dana Estra
Comment 6
2021-05-28 15:46:18 PDT
Created
attachment 430063
[details]
Patch
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
Created
attachment 430273
[details]
Patch
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
Created
attachment 430281
[details]
Patch
Dana Estra
Comment 12
2021-06-01 13:14:26 PDT
Created
attachment 430284
[details]
Patch
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
Created
attachment 430292
[details]
Patch
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
Created
attachment 431358
[details]
Patch
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug