Bug 218189 – [Contact Picker API] Add support for picker UI on iOS

• Component: New Bugs
• Status: RESOLVED FIXED

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/69862277>

[Aditya Keerthi]

Created attachment 412321 [details]
Patch

[Devin Rousso]

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]`

[Aditya Keerthi]

Created attachment 412554 [details]
Patch

[Aditya Keerthi]

(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.

[Darin Adler]

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.

[Aditya Keerthi]

Created attachment 412560 [details]
Patch

[Aditya Keerthi]

(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.

[Peng Liu]

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.

[Tim Horton]

(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.

[Sam Weinig]

Please try to clean this up by introducing the appropriate ENABLE/HAVE/USE macros rather than sprinkling #if PLATFORM() everywhere.

[Aditya Keerthi]

Created attachment 412748 [details]
Patch

[Aditya Keerthi]

The new patch introduces HAVE macros and goes back to soft linking the new frameworks to avoid dependency cycles.

[Aditya Keerthi]

Created attachment 412760 [details]
Patch

[Devin Rousso]

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; });
```

[Aditya Keerthi]

Created attachment 413168 [details]
Patch

[Aditya Keerthi]

(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.

[Darin Adler]

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".

[Darin Adler]

(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.

[Aditya Keerthi]

(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&&);
```

[EWS]

ChangeLog entry in Tools/ChangeLog contains OOPS!.

[Aditya Keerthi]

Created attachment 413181 [details]
Patch for landing

[EWS]

Committed r269394: <https://trac.webkit.org/changeset/269394>

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

[Frédéric Wang (:fredw)]

@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.

[Aditya Keerthi]

(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.