[webkit-reviews] review granted: [Bug 218050] [Contact Picker API] Implement ContactsManager.select() : [Attachment 412116] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Aditya Keerthi
<akeerthi at apple.com>'s request for review:
Bug 218050: [Contact Picker API] Implement ContactsManager.select()
https://bugs.webkit.org/show_bug.cgi?id=218050

Attachment 412116: Patch

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




--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 412116
  --> https://bugs.webkit.org/attachment.cgi?id=412116
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)?