...
<rdar://problem/69862186>
Created attachment 412026 [details] Patch
Created attachment 412031 [details] Patch
Created attachment 412116 [details] Patch
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)?
Created attachment 412132 [details] Patch
(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.
Created attachment 412134 [details] Patch
Added a missing header to Headers.cmake.
Committed r268901: <https://trac.webkit.org/changeset/268901> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412134 [details].