Bug 233890 - Add support for `navigator.requestCookieConsent()` behind a disabled feature flag
Summary: Add support for `navigator.requestCookieConsent()` behind a disabled feature ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2021-12-06 12:22 PST by Wenson Hsieh
Modified: 2021-12-08 09:05 PST (History)
17 users (show)

See Also:


Attachments
Patch (55.08 KB, patch)
2021-12-06 12:49 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix macOS SPI availability (55.08 KB, patch)
2021-12-06 12:53 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix Windows build (56.49 KB, patch)
2021-12-06 13:23 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Adjust ChangeLog (58.45 KB, patch)
2021-12-06 14:18 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (67.63 KB, patch)
2021-12-07 16:30 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Rebase again (66.00 KB, patch)
2021-12-07 17:04 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix Windows build (63.61 KB, patch)
2021-12-07 17:57 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Followup (for EWS) (5.01 KB, patch)
2021-12-07 20:46 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (2.35 KB, patch)
2021-12-08 08:09 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].