This patch provides a default storage access API prompt at the WebKit level, rather than relying on a custom version in the WebKit embedder.
<rdar://problem/45150009>
Created attachment 364982 [details] Patch
Comment on attachment 364982 [details] Patch Attachment 364982 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11544704 New failing tests: jquery/offset.html
Created attachment 364991 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 365032 [details] Patch
Created attachment 365071 [details] Patch
Created attachment 365121 [details] Patch
Comment on attachment 365121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365121&action=review > Source/WebCore/ChangeLog:3 > + Add default prompt implementation for Storage Access API Since you used to ask me, I'll do the same – please add a "the." :) > Source/WebKit/ChangeLog:3 > + Add default prompt implementation for Storage Access API Ditto. > Source/WebCore/en.lproj/Localizable.strings:106 > +/* Allow Storage Access API */ This sounds like allowing the API. Could "for the" help here? Or reverse it to "Storage Access API allow access" to match the comment for the deny case further down. > Source/WebCore/en.lproj/Localizable.strings:109 > +/* Title for Allow button label in Storage Access API request dialog */ the Storage Access API … > Source/WebCore/en.lproj/Localizable.strings:262 > +/* Title for Don't Allow button label in Storage Access API request dialog */ the > Source/WebCore/en.lproj/Localizable.strings:263 > +"Donât Allow (cross-site cookie and website data access)" = "Donât Allow"; I assume it's just Bugzilla messing up the apostrophe character. > Source/WebKit/UIProcess/Cocoa/UIDelegate.h:90 > + void presentStorageAccessConfirmDialog(const WTF::String& requestingDomain, const WTF::String& currentDomain, CompletionHandler<void(bool)>&&); Could WebCore::RegistrableDomain be used here? > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:339 > +void UIDelegate::UIClient::presentStorageAccessConfirmDialog(const WTF::String& requestingDomain, const WTF::String& currentDomain, CompletionHandler<void(bool)>&& completionHandler) We should use WebCore::RegistrableDomain here, if at all possible. > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:342 > + NSString *alertMessageString = [NSString stringWithFormat:WEB_UI_NSSTRING(@"Do you want to allow â%@â to use cookies and website data while browsing â%@â?", @"Message for requesting cross-site cookie and website data access."), requestingDomain.createCFString().get(), currentDomain.createCFString().get()]; I saw the labels and strings went into some localized infrastructure. So these will be localized, right? > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:343 > + NSString *informativeText = [NSString stringWithFormat:WEB_UI_NSSTRING(@"This will allow â%@â to track your activity.", @"Informative text for requesting cross-site cookie and website data access."), requestingDomain.createCFString().get()]; I'm no expert on CF APIs but I know we use adoptCF() in cpp files. Not the case in mm? I see instances where we use RetainPtr<CFStringRef>. > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:353 > + bool shouldAllow = returnCode == NSAlertFirstButtonReturn; auto? > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:382 > + [alert addAction:allowAction]; Is either of these are highlighted as the default action? > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:396 > + if (!m_uiDelegate.m_delegateMethods.webViewRequestStorageAccessPanelForTopPrivatelyControlledDomainUnderFirstPartyTopPrivatelyControlledDomainCompletionHandler) { Maybe not part of this patch, but we should change the name of this method to webViewRequestStorageAccessPanelForRegistrableDomainUnderFirstPartyRegistrableDomainCompletionHandler.
Comment on attachment 365121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365121&action=review >> Source/WebCore/en.lproj/Localizable.strings:263 >> +"Donât Allow (cross-site cookie and website data access)" = "Donât Allow"; > > I assume it's just Bugzilla messing up the apostrophe character. It would not if it were a regular ascii apostrophe. This must be some kind of unicode one. I'd suggest switching to the ascii '
Comment on attachment 365121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365121&action=review > Source/WebCore/en.lproj/Localizable.strings:821 > +"This will allow â%@â to track your activity." = "This will allow â%@â to track your activity."; ditto: %@â
Comment on attachment 365121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365121&action=review >> Source/WebCore/ChangeLog:3 >> + Add default prompt implementation for Storage Access API > > Since you used to ask me, I'll do the same – please add a "the." :) Done. >> Source/WebKit/ChangeLog:3 >> + Add default prompt implementation for Storage Access API > > Ditto. Done. >> Source/WebCore/en.lproj/Localizable.strings:109 >> +/* Title for Allow button label in Storage Access API request dialog */ > > the Storage Access API … Done. >> Source/WebCore/en.lproj/Localizable.strings:262 >> +/* Title for Don't Allow button label in Storage Access API request dialog */ > > the Done. >>> Source/WebCore/en.lproj/Localizable.strings:263 >>> +"Donât Allow (cross-site cookie and website data access)" = "Donât Allow"; >> >> I assume it's just Bugzilla messing up the apostrophe character. > > It would not if it were a regular ascii apostrophe. This must be some kind of unicode one. I'd suggest switching to the ascii ' Done. >> Source/WebCore/en.lproj/Localizable.strings:821 >> +"This will allow â%@â to track your activity." = "This will allow â%@â to track your activity."; > > ditto: %@â Okay. I made the same change on line 257. >> Source/WebKit/UIProcess/Cocoa/UIDelegate.h:90 >> + void presentStorageAccessConfirmDialog(const WTF::String& requestingDomain, const WTF::String& currentDomain, CompletionHandler<void(bool)>&&); > > Could WebCore::RegistrableDomain be used here? Sounds like a great follow-up fix. We would need to thread RegistrableDomain through several layers, which I don't want to do in this patch. (See new Bug 195957). >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:339 >> +void UIDelegate::UIClient::presentStorageAccessConfirmDialog(const WTF::String& requestingDomain, const WTF::String& currentDomain, CompletionHandler<void(bool)>&& completionHandler) > > We should use WebCore::RegistrableDomain here, if at all possible. Filed 195957 to do so. >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:342 >> + NSString *alertMessageString = [NSString stringWithFormat:WEB_UI_NSSTRING(@"Do you want to allow â%@â to use cookies and website data while browsing â%@â?", @"Message for requesting cross-site cookie and website data access."), requestingDomain.createCFString().get(), currentDomain.createCFString().get()]; > > I saw the labels and strings went into some localized infrastructure. So these will be localized, right? Right. >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:353 >> + bool shouldAllow = returnCode == NSAlertFirstButtonReturn; > > auto? Sure! >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:382 >> + [alert addAction:allowAction]; > > Is either of these are highlighted as the default action? Yes. Allow, just like in Safari. >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:396 >> + if (!m_uiDelegate.m_delegateMethods.webViewRequestStorageAccessPanelForTopPrivatelyControlledDomainUnderFirstPartyTopPrivatelyControlledDomainCompletionHandler) { > > Maybe not part of this patch, but we should change the name of this method to webViewRequestStorageAccessPanelForRegistrableDomainUnderFirstPartyRegistrableDomainCompletionHandler. We should definitely change the name, but preferably to something shorter!
(In reply to Brent Fulgham from comment #11) > Comment on attachment 365121 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=365121&action=review > > >> Source/WebCore/ChangeLog:3 > >> + Add default prompt implementation for Storage Access API > > > > Since you used to ask me, I'll do the same – please add a "the." :) > > Done. > > >> Source/WebKit/ChangeLog:3 > >> + Add default prompt implementation for Storage Access API > > > > Ditto. > > Done. > > >> Source/WebCore/en.lproj/Localizable.strings:109 > >> +/* Title for Allow button label in Storage Access API request dialog */ > > > > the Storage Access API … > > Done. > > >> Source/WebCore/en.lproj/Localizable.strings:262 > >> +/* Title for Don't Allow button label in Storage Access API request dialog */ > > > > the > > Done. > > >>> Source/WebCore/en.lproj/Localizable.strings:263 > >>> +"Donât Allow (cross-site cookie and website data access)" = "Donât Allow"; > >> > >> I assume it's just Bugzilla messing up the apostrophe character. > > > > It would not if it were a regular ascii apostrophe. This must be some kind of unicode one. I'd suggest switching to the ascii ' > > Done. > > >> Source/WebCore/en.lproj/Localizable.strings:821 > >> +"This will allow â%@â to track your activity." = "This will allow â%@â to track your activity."; > > > > ditto: %@â > > Okay. I made the same change on line 257. > > >> Source/WebKit/UIProcess/Cocoa/UIDelegate.h:90 > >> + void presentStorageAccessConfirmDialog(const WTF::String& requestingDomain, const WTF::String& currentDomain, CompletionHandler<void(bool)>&&); > > > > Could WebCore::RegistrableDomain be used here? > > Sounds like a great follow-up fix. We would need to thread RegistrableDomain > through several layers, which I don't want to do in this patch. (See new Bug > 195957). > > >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:339 > >> +void UIDelegate::UIClient::presentStorageAccessConfirmDialog(const WTF::String& requestingDomain, const WTF::String& currentDomain, CompletionHandler<void(bool)>&& completionHandler) > > > > We should use WebCore::RegistrableDomain here, if at all possible. > > Filed 195957 to do so. > > >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:342 > >> + NSString *alertMessageString = [NSString stringWithFormat:WEB_UI_NSSTRING(@"Do you want to allow â%@â to use cookies and website data while browsing â%@â?", @"Message for requesting cross-site cookie and website data access."), requestingDomain.createCFString().get(), currentDomain.createCFString().get()]; > > > > I saw the labels and strings went into some localized infrastructure. So these will be localized, right? > > Right. > > >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:353 > >> + bool shouldAllow = returnCode == NSAlertFirstButtonReturn; > > > > auto? > > Sure! > > >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:382 > >> + [alert addAction:allowAction]; > > > > Is either of these are highlighted as the default action? > > Yes. Allow, just like in Safari. I believe Safari does not have a default button for this prompt. > >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:396 > >> + if (!m_uiDelegate.m_delegateMethods.webViewRequestStorageAccessPanelForTopPrivatelyControlledDomainUnderFirstPartyTopPrivatelyControlledDomainCompletionHandler) { > > > > Maybe not part of this patch, but we should change the name of this method to webViewRequestStorageAccessPanelForRegistrableDomainUnderFirstPartyRegistrableDomainCompletionHandler. > > We should definitely change the name, but preferably to something shorter! How about webViewRequestStorageAccessPanelUnderFirstPartyCompletionHandler? Once we have RegistrableDomain as parameter types we don't need to mention it in function names.
Created attachment 365190 [details] Patch
Comment on attachment 365190 [details] Patch Attachment 365190 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11568886 New failing tests: http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Created attachment 365210 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
I'm looking at the two crashes now. Hard to see how my string changes would have broken anything, but ... <shrug>.
I don't believe the crashes are related to this patch: Crash 1: http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 WTFCrashWithInfo(int, char const*, char const*, int) + 19 (Assertions.h:566) 1 initAVEncoderBitRateKey() + 128 (MediaRecorderPrivateWriterCocoa.mm:55) 2 WebCore::MediaRecorderPrivateWriter::setAudioInput() + 38 (MediaRecorderPrivateWriterCocoa.mm:179) 3 WebCore::MediaRecorderPrivateWriter::create(WebCore::MediaStreamTrackPrivate const*, WebCore::MediaStreamTrackPrivate const*) + 574 (MediaRecorderPrivateWriterCocoa.mm:107) 4 WebCore::MediaRecorderPrivateAVFImpl::create(WebCore::MediaStreamPrivate const&) + 643 (RefPtr.h:81) 5 WebCore::MediaRecorder::create(WebCore::Document&, WTF::Ref<WebCore::MediaStream, WTF::DumbPtrTraits<WebCore::MediaStream> >&&, WebCore::MediaRecorder::Options&&) + 128 (memory:2588) 6 WebCore::JSDOMConstructor<WebCore::JSMediaRecorder>::construct(JSC::ExecState*) + 412 (Ref.h:59) Crash 2: imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 WTFCrashWithInfo(int, char const*, char const*, int) + 19 (Assertions.h:566) 1 initAVEncoderBitRateKey() + 128 (MediaRecorderPrivateWriterCocoa.mm:55) 2 WebCore::MediaRecorderPrivateWriter::setAudioInput() + 38 (MediaRecorderPrivateWriterCocoa.mm:179) 3 WebCore::MediaRecorderPrivateWriter::create(WebCore::MediaStreamTrackPrivate const*, WebCore::MediaStreamTrackPrivate const*) + 574 (MediaRecorderPrivateWriterCocoa.mm:107) 4 WebCore::MediaRecorderPrivateAVFImpl::create(WebCore::MediaStreamPrivate const&) + 643 (RefPtr.h:81) 5 WebCore::MediaRecorder::create(WebCore::Document&, WTF::Ref<WebCore::MediaStream, WTF::DumbPtrTraits<WebCore::MediaStream> >&&, WebCore::MediaRecorder::Options&&) + 128 (memory:2588) 6 WebCore::JSDOMConstructor<WebCore::JSMediaRecorder>::construct(JSC::ExecState*) + 412 (Ref.h:59)
Comment on attachment 365190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365190&action=review > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:339 > +void UIDelegate::UIClient::presentStorageAccessConfirmDialog(const WTF::String& requestingDomain, const WTF::String& currentDomain, CompletionHandler<void(bool)>&& completionHandler) Could we move this to a separate file / class to avoid crowding UIDelegate.mm? UIDelegate normally only acts as a proxy and contains as little code as possible. > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:357 > + struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> { Could this be replaced by a BlockPtr? auto completionBlock = makeBlockPtr([completionHandler = WTFMove(completionHandler)](bool shouldAllow) { completionHandler(shouldAllow); }); > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:373 > + UIAlertAction* doNotAllowAction = [UIAlertAction actionWithTitle:WEB_UI_STRING_KEY(@"Don't Allow", "Don't Allow (cross-site cookie and website data access)", @"Button title in Storage Access API prompt") style:UIAlertActionStyleCancel handler:makeBlockPtr([callbackAggregator = callbackAggregator.copyRef()](UIAlertAction *action) mutable { UIAlertAction* doNotAllowAction = [UIAlertAction actionWithTitle:WEB_UI_STRING_KEY(@"Don't Allow", "Don't Allow (cross-site cookie and website data access)", @"Button title in Storage Access API prompt") style:UIAlertActionStyleCancel handler:[completionBlock](UIAlertAction *action) { completionBlock(false); }); > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:384 > + [m_uiDelegate.m_webView.window.rootViewController presentViewController:alert animated:YES completion:nil]; I see some code if relying on [UIViewController _viewControllerForFullScreenPresentationFromView:view] to get the presenting view controller. I personally do not know which one is best. It'd be good to confirm with thorton / Wenson / Andy.
Comment on attachment 365190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365190&action=review >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:357 >> + struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> { > > Could this be replaced by a BlockPtr? > > auto completionBlock = makeBlockPtr([completionHandler = WTFMove(completionHandler)](bool shouldAllow) { > completionHandler(shouldAllow); > }); Oh! Seems like it. >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:373 >> + UIAlertAction* doNotAllowAction = [UIAlertAction actionWithTitle:WEB_UI_STRING_KEY(@"Don't Allow", "Don't Allow (cross-site cookie and website data access)", @"Button title in Storage Access API prompt") style:UIAlertActionStyleCancel handler:makeBlockPtr([callbackAggregator = callbackAggregator.copyRef()](UIAlertAction *action) mutable { > > UIAlertAction* doNotAllowAction = [UIAlertAction actionWithTitle:WEB_UI_STRING_KEY(@"Don't Allow", "Don't Allow (cross-site cookie and website data access)", @"Button title in Storage Access API prompt") style:UIAlertActionStyleCancel handler:[completionBlock](UIAlertAction *action) { > completionBlock(false); > }); Will do. >> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:384 >> + [m_uiDelegate.m_webView.window.rootViewController presentViewController:alert animated:YES completion:nil]; > > I see some code if relying on [UIViewController _viewControllerForFullScreenPresentationFromView:view] to get the presenting view controller. I personally do not know which one is best. It'd be good to confirm with thorton / Wenson / Andy. Wenson says [UIViewController _viewControllerForFullScreenPresentationFromView:view]
Created attachment 365228 [details] Patch
Comment on attachment 365228 [details] Patch r=me
Comment on attachment 365228 [details] Patch Clearing flags on attachment: 365228 Committed r243173: <https://trac.webkit.org/changeset/243173>
All reviewed patches have been landed. Closing bug.
Follow-up build fix: https://trac.webkit.org/r243178.
*** Bug 195424 has been marked as a duplicate of this bug. ***