Bug 195866 – Add default prompt implementation for Storage Access API

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

This patch provides a default storage access API prompt at the WebKit level, rather than relying on a custom version in the WebKit embedder.

[Brent Fulgham]

<rdar://problem/45150009>

[Brent Fulgham]

Created attachment 364982 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

Created attachment 365032 [details]
Patch

[Brent Fulgham]

Created attachment 365071 [details]
Patch

[Brent Fulgham]

Created attachment 365121 [details]
Patch

[John Wilander]

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.

[Chris Dumez]

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 '

[Chris Dumez]

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: %@â

[Brent Fulgham]

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!

[John Wilander]

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

[Brent Fulgham]

Created attachment 365190 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

I'm looking at the two crashes now. Hard to see how my string changes would have broken anything, but ... <shrug>.

[Brent Fulgham]

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)

[Chris Dumez]

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.

[Brent Fulgham]

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]

[Brent Fulgham]

Created attachment 365228 [details]
Patch

[Chris Dumez]

Comment on attachment 365228 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 365228 [details]
Patch

Clearing flags on attachment: 365228

Committed r243173: <https://trac.webkit.org/changeset/243173>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Follow-up build fix: https://trac.webkit.org/r243178.

[Brent Fulgham]

*** Bug 195424 has been marked as a duplicate of this bug. ***