.
<rdar://problem/86117718>
Created attachment 446069 [details] Patch
Created attachment 446072 [details] Fix macOS SPI availability
Created attachment 446077 [details] Fix Windows build
Created attachment 446083 [details] Adjust ChangeLog
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 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.
Created attachment 446252 [details] For EWS
Created attachment 446258 [details] Rebase again
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.
Created attachment 446271 [details] Fix Windows build
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”.)
Reopening to attach new patch.
Created attachment 446285 [details] Followup (for EWS)
(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!
Created attachment 446369 [details] Patch
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].