Bug 217354 - [Contact Picker API] Add support for ContactsManager.getProperties()
Summary: [Contact Picker API] Add support for ContactsManager.getProperties()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-05 17:04 PDT by Aditya Keerthi
Modified: 2020-10-07 13:10 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.50 KB, patch)
2020-10-05 17:07 PDT, 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-05 17:04:34 PDT
...
Comment 1 Aditya Keerthi 2020-10-05 17:04:58 PDT
<rdar://problem/69862099>
Comment 2 Aditya Keerthi 2020-10-05 17:07:09 PDT
Created attachment 410593 [details]
Patch
Comment 3 youenn fablet 2020-10-06 00:57:24 PDT
Comment on attachment 410593 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        by the API. These currently include name, email and tel.

Is there a spec available?

> Source/WebCore/Modules/contact-picker/ContactsManager.cpp:59
> +    Vector<ContactProperty> properties = { ContactProperty::Email, ContactProperty::Name, ContactProperty::Tel };

Is the order important? Should getProperties return a Set instead?

> LayoutTests/contact-picker/contacts-manager-get-properties.html:18
> +            shouldBeTrue("properties.includes('tel')");

If the order is important, we might want to validate email is first, then name, then tel.
If this test can be shared with other browsers as WPT, we could use resources/testharness.js.
This would be written as:

promise_test(async () => {
 ...
}, "getProperties");
Comment 4 Aditya Keerthi 2020-10-06 08:32:50 PDT
(In reply to youenn fablet from comment #3)
> Comment on attachment 410593 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410593&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        by the API. These currently include name, email and tel.
> 
> Is there a spec available?

The spec can be found here: https://wicg.github.io/contact-api/spec/#dom-contactsmanager-getproperties.
 
> > Source/WebCore/Modules/contact-picker/ContactsManager.cpp:59
> > +    Vector<ContactProperty> properties = { ContactProperty::Email, ContactProperty::Name, ContactProperty::Tel };
> 
> Is the order important? Should getProperties return a Set instead?

The spec does not place any restrictions on the order, but this implementation matches the Blink implementation.
 
> > LayoutTests/contact-picker/contacts-manager-get-properties.html:18
> > +            shouldBeTrue("properties.includes('tel')");
> 
> If the order is important, we might want to validate email is first, then
> name, then tel.
> If this test can be shared with other browsers as WPT, we could use
> resources/testharness.js.
> This would be written as:
> 
> promise_test(async () => {
>  ...
> }, "getProperties");

These is a WPT for this method here: https://github.com/web-platform-tests/wpt/blob/master/contacts/contacts-select.https.window.js.

However that test simply verifies that properties.length > 0, since it is up to the user agent to decide which properties are supported.
Comment 5 EWS 2020-10-07 13:10:41 PDT
Committed r268144: <https://trac.webkit.org/changeset/268144>

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