Summary: | [Contact Picker API] Add support for picker UI on iOS | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aditya Keerthi <akeerthi> | ||||||||||||||||
Component: | New Bugs | Assignee: | Aditya Keerthi <akeerthi> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, ews-watchlist, fred.wang, hi, peng.liu6, sam, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | Other | ||||||||||||||||||
Hardware: | iPhone / iPad | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=218547 | ||||||||||||||||||
Attachments: |
|
Description
Aditya Keerthi
2020-10-26 09:27:09 PDT
Created attachment 412321 [details]
Patch
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]` Created attachment 412554 [details]
Patch
(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 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. Created attachment 412560 [details]
Patch
(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 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. (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. Please try to clean this up by introducing the appropriate ENABLE/HAVE/USE macros rather than sprinkling #if PLATFORM() everywhere. Created attachment 412748 [details]
Patch
The new patch introduces HAVE macros and goes back to soft linking the new frameworks to avoid dependency cycles. Created attachment 412760 [details]
Patch
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; }); ``` Created attachment 413168 [details]
Patch
(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 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". (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. (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&&); ``` ChangeLog entry in Tools/ChangeLog contains OOPS!. Created attachment 413181 [details]
Patch for landing
Committed r269394: <https://trac.webkit.org/changeset/269394> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413181 [details]. @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. (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. |