Bug 233890 – Add support for `navigator.requestCookieConsent()` behind a disabled feature flag

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

.

[Radar WebKit Bug Importer]

<rdar://problem/86117718>

[Wenson Hsieh]

Created attachment 446069 [details]
Patch

[Wenson Hsieh]

Created attachment 446072 [details]
Fix macOS SPI availability

[Wenson Hsieh]

Created attachment 446077 [details]
Fix Windows build

[Wenson Hsieh]

Created attachment 446083 [details]
Adjust ChangeLog

[Darin Adler]

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;

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 446252 [details]
For EWS

[Wenson Hsieh]

Created attachment 446258 [details]
Rebase again

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 446271 [details]
Fix Windows build

[EWS]

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 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.

(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”.)

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 446285 [details]
Followup (for EWS)

[Wenson Hsieh]

(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!

[Wenson Hsieh]

Created attachment 446369 [details]
Patch

[EWS]

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].