Bug 218189 - [Contact Picker API] Add support for picker UI on iOS
Summary: [Contact Picker API] Add support for picker UI on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-26 09:27 PDT by Aditya Keerthi
Modified: 2020-11-06 08:02 PST (History)
12 users (show)

See Also:


Attachments
Patch (64.63 KB, patch)
2020-10-26 09:31 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (69.12 KB, patch)
2020-10-28 11:43 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (65.11 KB, patch)
2020-10-28 12:28 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (65.84 KB, patch)
2020-10-30 08:53 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (65.84 KB, patch)
2020-10-30 09:51 PDT, Aditya Keerthi
drousso: review+
Details | Formatted Diff | Diff
Patch (65.77 KB, patch)
2020-11-04 08:54 PST, Aditya Keerthi
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (65.77 KB, patch)
2020-11-04 10:57 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2020-10-26 09:27:09 PDT
...
Comment 1 Aditya Keerthi 2020-10-26 09:27:32 PDT
<rdar://problem/69862277>
Comment 2 Aditya Keerthi 2020-10-26 09:31:07 PDT
Created attachment 412321 [details]
Patch
Comment 3 Devin Rousso 2020-10-26 11:53:28 PDT
Comment on attachment 412321 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WKContactPicker.h:28
> +#if PLATFORM(COCOA) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

Is it necessary to `PLATFORM(COCOA)` given that this is inside `Source/WebKit/UIProcess/Cocoa`?

> Source/WebKit/UIProcess/Cocoa/WKContactPicker.h:51
> +- (void)contactPickerDidPresent:(WKContactPicker *)contactPicker;

NIT: do we normally order alphabetically ("D" before "P") or chronologically ("Present" before "Dismiss")?

> Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:43
> +SOFT_LINK_FRAMEWORK(Contacts)

Why do we need to soft link?

> Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:220
> +        NSString *contactName = [getCNContactFormatterClass() stringFromContact:contact style:CNContactFormatterStyleFullName];
> +        contactInfo.name = { contactName };

If the soft linking fails, what would `contactName` (and/or `CNContactFormatterStyleFullName`) end up being?

I haven't really dealt with soft linking before tho, so I dunno if this is something we realistically need to worry about.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9129
> +    if (!_contactPicker)
> +        return;

Is this check necessary?

> LayoutTests/contact-picker/contacts-select-after-dismissing-picker.html:45
> +                contacts = await request;

NIT: you can use destructuring to make this slightly nicer
```
   [contact1, contact2, contact3] = await request;
```
alternatively, you could just `contacts[0]` instead of `contact1` so that the index matches `addressBook[0]`
Comment 4 Aditya Keerthi 2020-10-28 11:43:44 PDT
Created attachment 412554 [details]
Patch
Comment 5 Aditya Keerthi 2020-10-28 11:50:27 PDT
(In reply to Devin Rousso from comment #3)
> Comment on attachment 412321 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412321&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WKContactPicker.h:28
> > +#if PLATFORM(COCOA) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> 
> Is it necessary to `PLATFORM(COCOA)` given that this is inside
> `Source/WebKit/UIProcess/Cocoa`?

Removed `PLATFORM(COCOA)` since this file is only included from WKContentViewInteraction, and the implementation source file is only in SourcesCocoa.txt.
 
> > Source/WebKit/UIProcess/Cocoa/WKContactPicker.h:51
> > +- (void)contactPickerDidPresent:(WKContactPicker *)contactPicker;
> 
> NIT: do we normally order alphabetically ("D" before "P") or chronologically
> ("Present" before "Dismiss")?

WebKit and platform (UIKit) convention looks chronological. See WKUIDelegatePrivate.h (didPresentFocusedElementViewController, didDismissFocusedElementViewController) and UIViewController.h (viewWillAppear, viewDidAppear).

Updated to match convention.

> > Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:43
> > +SOFT_LINK_FRAMEWORK(Contacts)
> 
> Why do we need to soft link?

I soft linked because the framework was previously soft linked in another source file. However, I've since removed the soft link in the other location, and have updated this file accordingly.

> > Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:220
> > +        NSString *contactName = [getCNContactFormatterClass() stringFromContact:contact style:CNContactFormatterStyleFullName];
> > +        contactInfo.name = { contactName };
> 
> If the soft linking fails, what would `contactName` (and/or
> `CNContactFormatterStyleFullName`) end up being?
> 
> I haven't really dealt with soft linking before tho, so I dunno if this is
> something we realistically need to worry about.

Not relevant anymore but a RELEASE_ASSERT would have been hit if the soft linking failed. (Since I used the SOFT_LINK_FRAMEWORK macro and not SOFT_LINK_OPTIONAL_FRAMEWORK).
 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9129
> > +    if (!_contactPicker)
> > +        return;
> 
> Is this check necessary?

Removed the check as sending a message to nil is safe.
 
> > LayoutTests/contact-picker/contacts-select-after-dismissing-picker.html:45
> > +                contacts = await request;
> 
> NIT: you can use destructuring to make this slightly nicer
> ```
>    [contact1, contact2, contact3] = await request;
> ```
> alternatively, you could just `contacts[0]` instead of `contact1` so that
> the index matches `addressBook[0]`

Updated to use destructuring.
Comment 6 Darin Adler 2020-10-28 12:11:03 PDT
Comment on attachment 412554 [details]
Patch

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

One small comment; didn’t get to review the whole patch (yet), an likely won}t get to it until after someone else does.

> Source/WebKit/Configurations/WebKit.xcconfig:65
> +WK_CONTACTS_UI_LDFLAGS = $(WK_CONTACTS_UI_LDFLAGS_$(WK_PLATFORM_NAME));
> +WK_CONTACTS_UI_LDFLAGS_iphoneos = -framework ContactsUI;
> +WK_CONTACTS_UI_LDFLAGS_iphonesimulator = -framework ContactsUI;
> +WK_CONTACTS_UI_LDFLAGS_maccatalyst = -framework ContactsUI;

Can we cut down on the proliferation of these variables? This gets to be a bigger and bigger liability over time. I’m not sure we need a separate variable for each framework; the variable’s job is to handle the per-platform differences. For now, can this one join in with WK_CONTACTS_LDFLAGS, or is there a place where we need to use WK_CONTACTS_LDFLAGS and must not use WK_CONTACTS_UI_LDFLAGS?

I think we should move in the direction of replacing these with variables named things more like WK_PLATFORM_DEPENDENT_LDFLAGS, containing list of framework options. Doing that we could combine at least 9 currently-separate variables that depend on the platform in a consistent way.
Comment 7 Aditya Keerthi 2020-10-28 12:28:50 PDT
Created attachment 412560 [details]
Patch
Comment 8 Aditya Keerthi 2020-10-28 12:30:07 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 412554 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412554&action=review
> 
> One small comment; didn’t get to review the whole patch (yet), an likely
> won}t get to it until after someone else does.
> 
> > Source/WebKit/Configurations/WebKit.xcconfig:65
> > +WK_CONTACTS_UI_LDFLAGS = $(WK_CONTACTS_UI_LDFLAGS_$(WK_PLATFORM_NAME));
> > +WK_CONTACTS_UI_LDFLAGS_iphoneos = -framework ContactsUI;
> > +WK_CONTACTS_UI_LDFLAGS_iphonesimulator = -framework ContactsUI;
> > +WK_CONTACTS_UI_LDFLAGS_maccatalyst = -framework ContactsUI;
> 
> Can we cut down on the proliferation of these variables? This gets to be a
> bigger and bigger liability over time. I’m not sure we need a separate
> variable for each framework; the variable’s job is to handle the
> per-platform differences. For now, can this one join in with
> WK_CONTACTS_LDFLAGS, or is there a place where we need to use
> WK_CONTACTS_LDFLAGS and must not use WK_CONTACTS_UI_LDFLAGS?
> 
> I think we should move in the direction of replacing these with variables
> named things more like WK_PLATFORM_DEPENDENT_LDFLAGS, containing list of
> framework options. Doing that we could combine at least 9 currently-separate
> variables that depend on the platform in a consistent way.

I've merged WK_CONTACTS_UI_LDFLAGS with WK_CONTACTS_LDFLAGS.
Comment 9 Peng Liu 2020-10-28 14:10:11 PDT
Comment on attachment 412560 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:311
> +#if PLATFORM(IOS_FAMILY) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

I guess you mean PLATFORM(IOS).

> Source/WebKit/UIProcess/Cocoa/WKContactPicker.h:28
> +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

Ditto.

> Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:29
> +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

Ditto.

> Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:38
> +#if PLATFORM(IOS_FAMILY)

Ditto. And the same for other macros in this patch.
Comment 10 Tim Horton 2020-10-28 14:16:20 PDT
(In reply to Peng Liu from comment #9)
> Comment on attachment 412560 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412560&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:311
> > +#if PLATFORM(IOS_FAMILY) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> 
> I guess you mean PLATFORM(IOS).
> 
> > Source/WebKit/UIProcess/Cocoa/WKContactPicker.h:28
> > +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> 
> Ditto.
> 
> > Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:29
> > +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> 
> Ditto.
> 
> > Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:38
> > +#if PLATFORM(IOS_FAMILY)
> 
> Ditto. And the same for other macros in this patch.

That would exclude macCatalyst.
Comment 11 Sam Weinig 2020-10-28 14:30:53 PDT
Please try to clean this up by introducing the appropriate ENABLE/HAVE/USE macros rather than sprinkling #if PLATFORM() everywhere.
Comment 12 Aditya Keerthi 2020-10-30 08:53:44 PDT
Created attachment 412748 [details]
Patch
Comment 13 Aditya Keerthi 2020-10-30 08:54:52 PDT
The new patch introduces HAVE macros and goes back to soft linking the new frameworks to avoid dependency cycles.
Comment 14 Aditya Keerthi 2020-10-30 09:51:38 PDT
Created attachment 412760 [details]
Patch
Comment 15 Devin Rousso 2020-11-03 13:11:17 PST
Comment on attachment 412760 [details]
Patch

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

r=me, nice work :)

> Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:199
> +    Vector<WebCore::ContactInfo> info;
> +    for (CNContact *contact in contacts)
> +        info.append([self _contactInfoFromCNContact:contact]);

Is it possible to use `makeVector` instead?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6856
> +    [_contactPicker presentWithRequestData:requestData completionHandler:WTFMove(completionHandler)];

the type of `completionHandler` doesn't match what's expected by `WKContactPicker` (i'm surprised this even compiles)

> LayoutTests/contact-picker/contacts-select-while-presenting-picker.html:27
> +                const promise1 = new Promise((resolve, reject) => {
> +                  navigator.contacts.select(['name']).then(resolve)
> +                                                     .catch(e => reject(e.message));
> +                });

NIT: you could simplify this
```
    let promise1 = navigator.contacts.select(['name']).catch((error) => { throw error.message; });
```
Comment 16 Aditya Keerthi 2020-11-04 08:54:29 PST
Created attachment 413168 [details]
Patch
Comment 17 Aditya Keerthi 2020-11-04 09:00:54 PST
(In reply to Devin Rousso from comment #15)
> Comment on attachment 412760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412760&action=review
> 
> r=me, nice work :)

Thank you for the review :)
 
> > Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:199
> > +    Vector<WebCore::ContactInfo> info;
> > +    for (CNContact *contact in contacts)
> > +        info.append([self _contactInfoFromCNContact:contact]);
> 
> Is it possible to use `makeVector` instead?

The variant of `makeVector` that takes a mapping function currently expects the mapping function to return something that has a `value_type` (see VectorCocoa.h). For that reason, I can't use any currently available version of `makeVector`.

I'm not sure if I should add another implementation of `makeVector` for my purpose, but for the time being I've updated my code to use `reserveInitialCapacity` and `uncheckedAppend`.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6856
> > +    [_contactPicker presentWithRequestData:requestData completionHandler:WTFMove(completionHandler)];
> 
> the type of `completionHandler` doesn't match what's expected by
> `WKContactPicker` (i'm surprised this even compiles)

Fixed.
 
> > LayoutTests/contact-picker/contacts-select-while-presenting-picker.html:27
> > +                const promise1 = new Promise((resolve, reject) => {
> > +                  navigator.contacts.select(['name']).then(resolve)
> > +                                                     .catch(e => reject(e.message));
> > +                });
> 
> NIT: you could simplify this
> ```
>     let promise1 = navigator.contacts.select(['name']).catch((error) => {
> throw error.message; });
> ```

Done.
Comment 18 Darin Adler 2020-11-04 10:01:51 PST
Comment on attachment 412760 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:199
>>> +        info.append([self _contactInfoFromCNContact:contact]);
>> 
>> Is it possible to use `makeVector` instead?
> 
> The variant of `makeVector` that takes a mapping function currently expects the mapping function to return something that has a `value_type` (see VectorCocoa.h). For that reason, I can't use any currently available version of `makeVector`.
> 
> I'm not sure if I should add another implementation of `makeVector` for my purpose, but for the time being I've updated my code to use `reserveInitialCapacity` and `uncheckedAppend`.

makeVector should work here. I don’t understand what you mean by "a value_type".
Comment 19 Darin Adler 2020-11-04 10:02:17 PST
(In reply to Darin Adler from comment #18)
> makeVector should work here. I don’t understand what you mean by "a
> value_type".

I’ll follow up after you land with a patch that moves to use makeVector.
Comment 20 Aditya Keerthi 2020-11-04 10:52:51 PST
(In reply to Darin Adler from comment #18)
> Comment on attachment 412760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412760&action=review
> 
> >>> Source/WebKit/UIProcess/Cocoa/WKContactPicker.mm:199
> >>> +        info.append([self _contactInfoFromCNContact:contact]);
> >> 
> >> Is it possible to use `makeVector` instead?
> > 
> > The variant of `makeVector` that takes a mapping function currently expects the mapping function to return something that has a `value_type` (see VectorCocoa.h). For that reason, I can't use any currently available version of `makeVector`.
> > 
> > I'm not sure if I should add another implementation of `makeVector` for my purpose, but for the time being I've updated my code to use `reserveInitialCapacity` and `uncheckedAppend`.
> 
> makeVector should work here. I don’t understand what you mean by "a
> value_type".

Sorry, I should have been more specific. `makeVector` with a mapping function currently requires the mapping function to return an Optional.

```
// This overload of makeVector takes a function to map each Objective-C object to a vector element.
// Currently, the map function needs to return an Optional.
template<typename MapFunctionType> Vector<typename std::invoke_result_t<MapFunctionType, id>::value_type> makeVector(NSArray *, MapFunctionType&&);
```
Comment 21 EWS 2020-11-04 10:55:53 PST
ChangeLog entry in Tools/ChangeLog contains OOPS!.
Comment 22 Aditya Keerthi 2020-11-04 10:57:59 PST
Created attachment 413181 [details]
Patch for landing
Comment 23 EWS 2020-11-04 14:36:52 PST
Committed r269394: <https://trac.webkit.org/changeset/269394>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413181 [details].
Comment 24 Frédéric Wang (:fredw) 2020-11-05 11:14:38 PST
@Aditya Keerthi FYI, It seems bug 218547 happens landed more or less at the same as your patch. So your tests should not use experimental: anymore.
Comment 25 Aditya Keerthi 2020-11-06 08:02:08 PST
(In reply to Frédéric Wang (:fredw) from comment #24)
> @Aditya Keerthi FYI, It seems bug 218547 happens landed more or less at the
> same as your patch. So your tests should not use experimental: anymore.

Thanks for the heads up. I've put up a patch to remove them here: https://bugs.webkit.org/show_bug.cgi?id=218658.