Bug 218050 – [Contact Picker API] Add skeleton implementation of ContactsManager.select()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/69862186>

[Aditya Keerthi]

Created attachment 412026 [details]
Patch

[Aditya Keerthi]

Created attachment 412031 [details]
Patch

[Aditya Keerthi]

Created attachment 412116 [details]
Patch

[Devin Rousso]

Comment on attachment 412116 [details]
Patch

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

r=me, assuming you fix the build issues for GTK/WPE/Win/etc :)

> Source/WebCore/Modules/contact-picker/ContactsManager.cpp:82
> +    auto frame = makeRefPtr(this->frame());

Do we actually need to `makeRefPtr` here?  You properly null-check it and don't give it to the lambda, so is it necessary to increase the ref count?

> Source/WebCore/Modules/contact-picker/ContactsManager.cpp:84
> +        promise->reject(Exception { InvalidStateError });

NIT: do we actually need the `Exception { ... }` instead of just `InvalidStateError`?

> Source/WebCore/Modules/contact-picker/ContactsManager.cpp:89
> +        promise->reject(Exception { SecurityError });

ditto (:84)

> Source/WebCore/Modules/contact-picker/ContactsManager.cpp:94
> +        promise->reject(Exception { InvalidStateError });

ditto (:84)

> Source/WebCore/Modules/contact-picker/ContactsManager.cpp:113
> +        if (info) {

Is it actually necessary to wrap the `Vector` in an `Optional`?  Can we just have the `Vector` be the response, which resolves the `promise` with an empty array in that case?  Or do we explicitly want/need to differentiate between "the user did not pick any contacts" and "WebKit was unable to show the picker UI"?

> Source/WebCore/Modules/contact-picker/ContactsManager.cpp:118
> +        promise->reject(Exception { UnknownError });

ditto (:84)

> Source/WebCore/Modules/contact-picker/ContactsManager.h:58
> +    bool m_contactPickerIsShowing { false };

I'd put this below for (potentially) better packing

> Source/WebKit/UIProcess/PageClient.h:258
> +    virtual void showContactPicker(const WebCore::ContactsRequestData&, WTF::CompletionHandler<void(Optional<Vector<WebCore::ContactInfo>>&&)>&& completionHandler) { completionHandler(WTF::nullopt); }

So this patch doesn't hook this up to `PageClientImplCocoa`/`PageClientImplMac`/`PageClientImplIOS`/?  If so, could we maybe retitle this bug so it's clear that we're only implementing the skeleton/structure of this functionality (and add a related/blocked bug for the rest of it)?

[Aditya Keerthi]

Created attachment 412132 [details]
Patch

[Aditya Keerthi]

(In reply to Devin Rousso from comment #5)
> Comment on attachment 412116 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412116&action=review
> 
> r=me, assuming you fix the build issues for GTK/WPE/Win/etc :)

Thank you for the review :)

> > Source/WebCore/Modules/contact-picker/ContactsManager.cpp:82
> > +    auto frame = makeRefPtr(this->frame());
> 
> Do we actually need to `makeRefPtr` here?  You properly null-check it and
> don't give it to the lambda, so is it necessary to increase the ref count?

Removed `makeRefPtr`.

> > Source/WebCore/Modules/contact-picker/ContactsManager.cpp:84
> > +        promise->reject(Exception { InvalidStateError });
> 
> NIT: do we actually need the `Exception { ... }` instead of just
> `InvalidStateError`?

No need for the `Exception { ... }`. Corrected this instance and the others below.

> > Source/WebCore/Modules/contact-picker/ContactsManager.cpp:113
> > +        if (info) {
> 
> Is it actually necessary to wrap the `Vector` in an `Optional`?  Can we just
> have the `Vector` be the response, which resolves the `promise` with an
> empty array in that case?  Or do we explicitly want/need to differentiate
> between "the user did not pick any contacts" and "WebKit was unable to show
> the picker UI"?

We need to differentiate between the two.

The spec states "If presenting a user interface fails or accessing the contacts source's available contacts fails, then return failure." This is distinct from the empty list case: "The UI MUST provide an option to cancel/return without sharing any contacts, in which case remove the UI and return an empty list."
 
> > Source/WebCore/Modules/contact-picker/ContactsManager.h:58
> > +    bool m_contactPickerIsShowing { false };
> 
> I'd put this below for (potentially) better packing

Done.

> > Source/WebKit/UIProcess/PageClient.h:258
> > +    virtual void showContactPicker(const WebCore::ContactsRequestData&, WTF::CompletionHandler<void(Optional<Vector<WebCore::ContactInfo>>&&)>&& completionHandler) { completionHandler(WTF::nullopt); }
> 
> So this patch doesn't hook this up to
> `PageClientImplCocoa`/`PageClientImplMac`/`PageClientImplIOS`/?  If so,
> could we maybe retitle this bug so it's clear that we're only implementing
> the skeleton/structure of this functionality (and add a related/blocked bug
> for the rest of it)?

Correct. I've renamed the bug.

[Aditya Keerthi]

Created attachment 412134 [details]
Patch

[Aditya Keerthi]

Added a missing header to Headers.cmake.

[EWS]

Committed r268901: <https://trac.webkit.org/changeset/268901>

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