Bug 233890

Summary: Add support for `navigator.requestCookieConsent()` behind a disabled feature flag
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bdakin, cdumez, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, japhet, katherine_cheney, kondapallykalyan, mitz, ryuan.choi, sergio, thorton, webkit-bug-importer, wenson_hsieh, wilander
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Fix macOS SPI availability
ews-feeder: commit-queue-
Fix Windows build
none
Adjust ChangeLog
none
For EWS
ews-feeder: commit-queue-
Rebase again
ews-feeder: commit-queue-
Fix Windows build
none
Followup (for EWS)
none
Patch none

Description Wenson Hsieh 2021-12-06 12:22:32 PST 
.
Comment 1 Radar WebKit Bug Importer 2021-12-06 12:23:10 PST 
<rdar://problem/86117718>
Comment 2 Wenson Hsieh 2021-12-06 12:49:06 PST Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-12-06 12:53:55 PST Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2021-12-06 13:23:20 PST Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2021-12-06 14:18:00 PST 
Created attachment 446083 [details]
Adjust ChangeLog
Comment 6 Darin Adler 2021-12-07 14:27:49 PST 
Comment on attachment 446083 [details]
Adjust ChangeLog

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

> Source/WebCore/Modules/cookie-consent/NavigatorCookieConsent.h:37
> +class DeferredPromise;
> +class Navigator;
> +struct RequestCookieConsentOptions;

I typically put a blank line between class and struct.

> Source/WebCore/Modules/cookie-consent/RequestCookieConsentOptions.h:28
> +#include "VoidCallback.h"

Can we forward-declare VoidCallback instead of including the header? We want to resist including headers in headers as much as we can.

> Source/WebCore/page/ChromeClient.h:27
> +#include "CookieConsentDecisionResult.h"

Can’t we use this instead?

enum class CookieConsentDecisionResult : uint8_t;

> Source/WebKit/UIProcess/API/APIUIClient.h:33
> +#include <WebCore/CookieConsentDecisionResult.h>

Can’t we use this instead?

enum class CookieConsentDecisionResult : uint8_t;
Comment 7 Wenson Hsieh 2021-12-07 15:48:44 PST 
Comment on attachment 446083 [details]
Adjust ChangeLog

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

Thanks for the review!

>> Source/WebCore/Modules/cookie-consent/NavigatorCookieConsent.h:37
>> +struct RequestCookieConsentOptions;
> 
> I typically put a blank line between class and struct.

Added!

>> Source/WebCore/Modules/cookie-consent/RequestCookieConsentOptions.h:28
>> +#include "VoidCallback.h"
> 
> Can we forward-declare VoidCallback instead of including the header? We want to resist including headers in headers as much as we can.

Good point — changed this to a forward declaration. (Note that I did need to forward declare several ctors and the dtor (and add a RequestCookieConsentOptions.cpp) though, since the ctor/dtor require VoidCallback to not be a forward declaration).

>> Source/WebCore/page/ChromeClient.h:27
>> +#include "CookieConsentDecisionResult.h"
> 
> Can’t we use this instead?
> 
> enum class CookieConsentDecisionResult : uint8_t;

Ah, so I needed to include the header here because I'm doing `CookieConsentDecisionResult::NotSupported` below.

However, I can turn this into a forward enum declaration here by making `requestCookieConsent` a pure virtual function (and then adding implementation stubs elsewhere, as needed). Will do that!

>> Source/WebKit/UIProcess/API/APIUIClient.h:33
>> +#include <WebCore/CookieConsentDecisionResult.h>
> 
> Can’t we use this instead?
> 
> enum class CookieConsentDecisionResult : uint8_t;

This one might be tricky to avoid, since we need to invoke the completion handler below with the value `CookieConsentDecisionResult::NotSupported`... I'll leave this one as-is.
Comment 8 Wenson Hsieh 2021-12-07 16:30:17 PST 
Created attachment 446252 [details]
For EWS
Comment 9 Wenson Hsieh 2021-12-07 17:04:26 PST 
Created attachment 446258 [details]
Rebase again
Comment 10 Wenson Hsieh 2021-12-07 17:40:51 PST 
Comment on attachment 446083 [details]
Adjust ChangeLog

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

>>> Source/WebCore/Modules/cookie-consent/RequestCookieConsentOptions.h:28
>>> +#include "VoidCallback.h"
>> 
>> Can we forward-declare VoidCallback instead of including the header? We want to resist including headers in headers as much as we can.
> 
> Good point — changed this to a forward declaration. (Note that I did need to forward declare several ctors and the dtor (and add a RequestCookieConsentOptions.cpp) though, since the ctor/dtor require VoidCallback to not be a forward declaration).

> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\Modules\cookie-consent\RequestCookieConsentOptions.h(39,34): error C2079: 'WebCore::RequestCookieConsentOptions::moreInfo' uses undefined class 'WTF::RefPtr<WebCore::VoidCallback,WTF::RawPtrTraits<T>,WTF::DefaultRefDerefTraits<T>>' [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\Modules\cookie-consent\RequestCookieConsentOptions.h(39,34): error C2079:         with [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\Modules\cookie-consent\RequestCookieConsentOptions.h(39,34): error C2079:         [ [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\Modules\cookie-consent\RequestCookieConsentOptions.h(39,34): error C2079:             T=WebCore::VoidCallback [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\Modules\cookie-consent\RequestCookieConsentOptions.h(39,34): error C2079:         ] (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-cbdfe323-7.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

Sadly, it seems like this forward declaration breaks the windows build :(

I'll revert it to #include "VoidCallback.h" for now.
Comment 11 Wenson Hsieh 2021-12-07 17:57:42 PST 
Created attachment 446271 [details]
Fix Windows build
Comment 12 EWS 2021-12-07 20:25:54 PST 
Committed r286640 (244953@main): <https://commits.webkit.org/244953@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446271 [details].
Comment 13 mitz 2021-12-07 20:29:04 PST 
Comment on attachment 446271 [details]
Fix Windows build

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

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:658
> +    [(id <WKUIDelegatePrivate>)delegate _webView:m_uiDelegate->m_webView.get().get() requestCookieConsentWithMoreInfoHandler:nil decisionHandler:makeBlockPtr([completion = WTFMove(completion)] (BOOL decision) mutable {

I thought it was customary to wrap a CompletionHandlerCallChecker around calls to delegate methods that take a completion handler.
Comment 14 mitz 2021-12-07 20:30:47 PST 
(In reply to mitz from comment #13)
> Comment on attachment 446271 [details]
> Fix Windows build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446271&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:658
> > +    [(id <WKUIDelegatePrivate>)delegate _webView:m_uiDelegate->m_webView.get().get() requestCookieConsentWithMoreInfoHandler:nil decisionHandler:makeBlockPtr([completion = WTFMove(completion)] (BOOL decision) mutable {
> 
> I thought it was customary to wrap a CompletionHandlerCallChecker around
> calls to delegate methods that take a completion handler.

(more precisely: “around a completion handler being passed to a delegate method”.)
Comment 15 Wenson Hsieh 2021-12-07 20:46:18 PST 
Reopening to attach new patch.
Comment 16 Wenson Hsieh 2021-12-07 20:46:21 PST 
Created attachment 446285 [details]
Followup (for EWS)
Comment 17 Wenson Hsieh 2021-12-07 20:46:51 PST 
(In reply to mitz from comment #14)
> (In reply to mitz from comment #13)
> > Comment on attachment 446271 [details]
> > Fix Windows build
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=446271&action=review
> > 
> > > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:658
> > > +    [(id <WKUIDelegatePrivate>)delegate _webView:m_uiDelegate->m_webView.get().get() requestCookieConsentWithMoreInfoHandler:nil decisionHandler:makeBlockPtr([completion = WTFMove(completion)] (BOOL decision) mutable {
> > 
> > I thought it was customary to wrap a CompletionHandlerCallChecker around
> > calls to delegate methods that take a completion handler.
> 
> (more precisely: “around a completion handler being passed to a delegate
> method”.)

Good catch!
Comment 18 Wenson Hsieh 2021-12-08 08:09:31 PST 
Created attachment 446369 [details]
Patch
Comment 19 EWS 2021-12-08 09:05:39 PST 
Committed r286658 (244971@main): <https://commits.webkit.org/244971@main>

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