Bug 185335 – Add experimental feature to prompt for Storage Access API use

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

[Brent Fulgham]

Create an experimental feature that allows a client to check for user agreement to allow Storage Access API use.

[Brent Fulgham]

Note: This should default to an off state. We don't want this active for most uses.

[Radar WebKit Bug Importer]

<rdar://problem/39994649>

[Brent Fulgham]

Created attachment 339616 [details]
Patch

[EWS Watchlist]

Attachment 339616 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:349:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/APIUIClient.h:131:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/C/WKPage.cpp:1535:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 339651 [details]
Patch

[Brent Fulgham]

Created attachment 339652 [details]
Patch

[EWS Watchlist]

Attachment 339652 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:349:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/APIUIClient.h:131:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/C/WKPage.cpp:1535:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 339655 [details]
Patch

[EWS Watchlist]

Attachment 339655 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:349:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/APIUIClient.h:131:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/C/WKPage.cpp:1535:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 339655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339655&action=review

> Source/WebCore/testing/InternalSettings.cpp:786
> +#if ENABLE(MEDIA_STREAM)
> +    RuntimeEnabledFeatures::sharedFeatures().setStorageAccessPromptsEnabled(enabled);
> +#else

Why is this restricted to media streams? The runtime feature isn't.

> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:1995
> +void WKPreferencesSetPromptForStorageAccessAPIEnabled(WKPreferencesRef preferencesRef, bool enabled)
> +{
> +    toImpl(preferencesRef)->setStorageAccessPromptsEnabled(enabled);
> +}
> +
> +bool WKPreferencesGetPromptForStorageAccessAPIEnabled(WKPreferencesRef preferencesRef)
> +{
> +    return toImpl(preferencesRef)->storageAccessPromptsEnabled();
> +}

Do you need these versions for the legacy C API? I don't think so.

> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:324
> +// Defaults to false.
> +WK_EXPORT void WKPreferencesSetPromptForStorageAccessAPIEnabled(WKPreferencesRef preferences, bool enabled);
> +WK_EXPORT bool WKPreferencesGetPromptForStorageAccessAPIEnabled(WKPreferencesRef preferences);

Do you need these versions for the legacy C API? I don't think so.

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:2
> - * Copyright (C) 2014-2017 Apple Inc. All rights reserved.
> + * Copyright (C) 2014-2018 Apple Inc. All rights reserved.

You didn't change anything else in this file.

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:130
> +/*! @abstract Displays a non-partitioned storage access panel.
> + @param webView The web view invoking the delegate method.
> + @param topPrivatelyControlledDomain The eTLD+1 of the domain requesting
> + access to its non-partitioned website data.
> + @param underFirstPartyTopPrivatelyControlledDomain The eTLD+1 of the
> + first-party domain under which the request for website data access is
> + being made.
> + @param completionHandler The completion handler to call after the confirm
> + panel has been dismissed. Pass YES if the user chose OK, NO if the user
> + chose Don't Allow.

Seeing as this will be public, do we expect people to know what an eTLD is? And an eTLD+1? I also note that your parameter is called topPrivatelyControlledDomainUnderWhichStorageAccessWillBeProvided

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:334
> +    WTFLogAlways("ResourceLoadLog | UIClient::requestStorageAccessConfirm() 1.");

Why LogAlways instead of a dedicated log channel? Also, this seems like printf debugging during development. Same for the other two in this function.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3288
> +    WebCore::RuntimeEnabledFeatures::sharedFeatures().setStorageAccessPromptsEnabled(m_preferences->storageAccessPromptsEnabled());

I *think* this is done automatically in generated code.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4286
> +    // Since runJavaScriptConfirm() can spin a nested run loop we need to turn off the responsiveness timer.

Where is runJavaScriptConfirm called? And when do you turn the timer back on?

> Source/WebKit/UIProcess/WebPageProxy.cpp:4294
> +    WTFLogAlways("ResourceLoadLog | WebPageProxy::requestStorageAccessConfirm().");

Another log always that doesn't seem useful as a global message.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1300
> +    WTFLogAlways("ResourceLoadLog | The user granted storage access to %s on page from %s.", subFrameHost.utf8().data(), topFrameHost.utf8().data());

And again.

[Brent Fulgham]

Comment on attachment 339655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339655&action=review

>> Source/WebCore/testing/InternalSettings.cpp:786
>> +#else
> 
> Why is this restricted to media streams? The runtime feature isn't.

Because I am an idiot! I was duplicating code for another feature to do this, and did a poor job of it!

>> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:1995
>> +}
> 
> Do you need these versions for the legacy C API? I don't think so.

You are right. I'll remove them.

>> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:324
>> +WK_EXPORT bool WKPreferencesGetPromptForStorageAccessAPIEnabled(WKPreferencesRef preferences);
> 
> Do you need these versions for the legacy C API? I don't think so.

You are correct.

>> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.h:2
>> + * Copyright (C) 2014-2018 Apple Inc. All rights reserved.
> 
> You didn't change anything else in this file.

Why don't you like 2018, Dean?

>> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:130
>> + chose Don't Allow.
> 
> Seeing as this will be public, do we expect people to know what an eTLD is? And an eTLD+1? I also note that your parameter is called topPrivatelyControlledDomainUnderWhichStorageAccessWillBeProvided

I'll adjust the verbosity. This should also be under WKUIDelegatePrivate (as Alex suggested separately), so I'll fix that.

>> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:334
>> +    WTFLogAlways("ResourceLoadLog | UIClient::requestStorageAccessConfirm() 1.");
> 
> Why LogAlways instead of a dedicated log channel? Also, this seems like printf debugging during development. Same for the other two in this function.

Yeah, they are. I'll remove them.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3288
>> +    WebCore::RuntimeEnabledFeatures::sharedFeatures().setStorageAccessPromptsEnabled(m_preferences->storageAccessPromptsEnabled());
> 
> I *think* this is done automatically in generated code.

Testing indicates that it doesn't seem to happen on the UIProcess side of things (maybe until after you restart). WebProcess picks changes up immediately.

>> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1300
>> +    WTFLogAlways("ResourceLoadLog | The user granted storage access to %s on page from %s.", subFrameHost.utf8().data(), topFrameHost.utf8().data());
> 
> And again.

OK.

[Brent Fulgham]

Created attachment 339659 [details]
Patch

[EWS Watchlist]

Attachment 339659 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:346:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/APIUIClient.h:131:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/C/WKPage.cpp:1535:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 339659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339659&action=review

> Source/WebKit/UIProcess/API/APIUIClient.h:131
> +    virtual void requestStorageAccessConfirm(WebKit::WebPageProxy*, WebKit::WebFrameProxy*, const WTF::String& requestingDomain, const WTF::String& currentDomain, Function<void (bool)>&& completionHandler) { completionHandler(true); }

I think we should make the parameters a WebPageProxy& and a WebFrameProxy&

> Source/WebKit/UIProcess/API/C/WKPage.cpp:1535
> +    Function<void (bool)> m_completionHandler;

This and all uses of Function in this patch could be a CompletionHandler.
I also think we don't have a space between void and (bool) in our style.

> Source/WebKit/UIProcess/API/C/WKPage.cpp:2028
> +            RefPtr<RequestStorageAccessConfirmResultListener> listener = RequestStorageAccessConfirmResultListener::create(WTFMove(completionHandler));

auto listener

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:119
> +- (void)webView:(WKWebView *)webView requestStorageAccessPanelForDomain:(NSString *)requestingDomain underCurrentDomain:(NSString *)currentDomain completionHandler:(void (^)(BOOL result))completionHandler;

webView should be _webView because this is SPI

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:345
> +    RefPtr<CompletionHandlerCallChecker> checker = CompletionHandlerCallChecker::create(delegate.get(), @selector(webView:requestStorageAccessPanelForDomain:underCurrentDomain:completionHandler:));

auto checker

> Source/WebKit/UIProcess/WebPageProxy.cpp:4292
> +    WTFLogAlways("ResourceLoadLog | WebPageProxy::requestStorageAccessConfirm().");

Please remove

> Source/WebKit/UIProcess/WebPreferences.cpp:137
> +    if (key == WebPreferencesKey::storageAccessPromptsEnabledKey())

This feels out of place because it's the only such thing here.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1292
>  void WebChromeClient::requestStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, WTF::CompletionHandler<void (bool)>&& callback)

We can remove WTF:: here

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1295
> +    if (!WebProcess::singleton().parentProcessConnection()->sendSync(Messages::WebPageProxy::RequestStorageAccessConfirm(frameID, subFrameHost, topFrameHost), Messages::WebPageProxy::RequestStorageAccessConfirm::Reply(result), pageID, Seconds::infinity(), IPC::SendSyncOption::InformPlatformProcessWillSuspend) || !result) {

I don't think we should add synchronous IPC for this.  I think we should have a different message type for the reply, and store the CompletionHandler in a HashMap with an identifier so we don't hang the WebProcess during this.

[Alex Christensen]

Comment on attachment 339659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339659&action=review

>> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:119
>> +- (void)webView:(WKWebView *)webView requestStorageAccessPanelForDomain:(NSString *)requestingDomain underCurrentDomain:(NSString *)currentDomain completionHandler:(void (^)(BOOL result))completionHandler;
> 
> webView should be _webView because this is SPI

And this needs an availability macro.

[Brent Fulgham]

Comment on attachment 339659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339659&action=review

>> Source/WebKit/UIProcess/API/APIUIClient.h:131
>> +    virtual void requestStorageAccessConfirm(WebKit::WebPageProxy*, WebKit::WebFrameProxy*, const WTF::String& requestingDomain, const WTF::String& currentDomain, Function<void (bool)>&& completionHandler) { completionHandler(true); }
> 
> I think we should make the parameters a WebPageProxy& and a WebFrameProxy&

Done.

>> Source/WebKit/UIProcess/API/C/WKPage.cpp:1535
>> +    Function<void (bool)> m_completionHandler;
> 
> This and all uses of Function in this patch could be a CompletionHandler.
> I also think we don't have a space between void and (bool) in our style.

Yeah, I've let Chris Dumez rub off on me too much here. I'll remove the space.

>> Source/WebKit/UIProcess/API/C/WKPage.cpp:2028
>> +            RefPtr<RequestStorageAccessConfirmResultListener> listener = RequestStorageAccessConfirmResultListener::create(WTFMove(completionHandler));
> 
> auto listener

I tried this initially, but clang get confused about type and toAPI fails.

>>> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:119
>>> +- (void)webView:(WKWebView *)webView requestStorageAccessPanelForDomain:(NSString *)requestingDomain underCurrentDomain:(NSString *)currentDomain completionHandler:(void (^)(BOOL result))completionHandler;
>> 
>> webView should be _webView because this is SPI
> 
> And this needs an availability macro.

Done and Done!

>> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:345
>> +    RefPtr<CompletionHandlerCallChecker> checker = CompletionHandlerCallChecker::create(delegate.get(), @selector(webView:requestStorageAccessPanelForDomain:underCurrentDomain:completionHandler:));
> 
> auto checker

Done!

>> Source/WebKit/UIProcess/WebPageProxy.cpp:4292
>> +    WTFLogAlways("ResourceLoadLog | WebPageProxy::requestStorageAccessConfirm().");
> 
> Please remove

Done!

>> Source/WebKit/UIProcess/WebPreferences.cpp:137
>> +    if (key == WebPreferencesKey::storageAccessPromptsEnabledKey())
> 
> This feels out of place because it's the only such thing here.

I can't find a more appropriate place to set this value in the call stack. I did it the same way as WebPreferencesKey::privateBrowsingEnabledKey(), the only difference being that this is an experimental feature, while private browsing is a full feature.

Since this is an experimental feature, its state is controlled by the client application, so I need to set it in this call stack, since some clients (e.g., Safari) manipulate multiple WKPreferences for different uses and not all of them bother to read the correct state from NSPreferences, so it will improperly reset this value to its default.

I can get into the details with you in person if you want to know, but I'd ask that you allow this quirk for the time being.

>> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1292
>>  void WebChromeClient::requestStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, WTF::CompletionHandler<void (bool)>&& callback)
> 
> We can remove WTF:: here

Done.

>> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1295
>> +    if (!WebProcess::singleton().parentProcessConnection()->sendSync(Messages::WebPageProxy::RequestStorageAccessConfirm(frameID, subFrameHost, topFrameHost), Messages::WebPageProxy::RequestStorageAccessConfirm::Reply(result), pageID, Seconds::infinity(), IPC::SendSyncOption::InformPlatformProcessWillSuspend) || !result) {
> 
> I don't think we should add synchronous IPC for this.  I think we should have a different message type for the reply, and store the CompletionHandler in a HashMap with an identifier so we don't hang the WebProcess during this.

Hmm. Okay. I'll do that.

[Brent Fulgham]

Created attachment 339693 [details]
Patch

[EWS Watchlist]

Attachment 339693 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:346:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/API/APIUIClient.h:131:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/WebProcess/WebProcess.cpp:84:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1292:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 339697 [details]
Patch

[EWS Watchlist]

Comment on attachment 339697 [details]
Patch

Attachment 339697 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7590023

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 339700 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Brent Fulgham]

Those windows-specific issues are unrelated to this modern WebKit patch. Apple WebKit on Win does not use the modern WebKit infrastructure.

[Alex Christensen]

Comment on attachment 339697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339697&action=review

Better, but still some substantial changes needed.

> Source/WebKit/UIProcess/API/APIUIClient.h:131
> +    virtual void requestStorageAccessConfirm(WebKit::WebPageProxy&, WebKit::WebFrameProxy&, const WTF::String& requestingDomain, const WTF::String& currentDomain, Function<void(bool)>&& completionHandler) { completionHandler(true); }

This Function could be a CompletionHandler

> Source/WebKit/UIProcess/API/C/WKPage.cpp:2028
> +            RefPtr<RequestStorageAccessConfirmResultListener> listener = RequestStorageAccessConfirmResultListener::create(WTFMove(completionHandler));

auto listener.  Really.  This would make the code cleaner, and make listener a ref, and you would need to call listener.ptr() below.

> Source/WebKit/UIProcess/Cocoa/UIDelegate.h:91
> +        void requestStorageAccessConfirm(WebPageProxy&, WebFrameProxy&, const WTF::String& requestingDomain, const WTF::String& currentDomain, Function<void(bool)>&& completionHandler) final;

CompletionHandler

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1294
> +    static std::atomic<uint64_t> currentHandlerIdentifier;

Can this be accessed by more than one thread?  If not (and I think the answer is no) then this doesn't need to be atomic and we don't need m_requestStorageAccessCallbackLock.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:367
> +    HashMap<uint64_t, std::tuple<String, String, CompletionHandler<void(bool)>>> m_requestStorageAccessCallbacks;

You can just make the value of this HashMap a CompletionHandler and capture subFrameHost and topFrameHost and the given CompletionHandler in a lambda.

[youenn fablet]

Comment on attachment 339697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339697&action=review

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1313
> +    auto callbackTuple = m_requestStorageAccessCallbacks.take(callbackIdentifier);

We often exit early if there is no callbakIdentifier in the map.
Might use find/remove(iterator)

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:366
> +    Lock m_requestStorageAccessCallbackLock;

Do we need this lock, maybe all calls are main thread only?

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:367
> +    HashMap<uint64_t, std::tuple<String, String, CompletionHandler<void(bool)>>> m_requestStorageAccessCallbacks;

Since this is a map of CompletionHandler, you might need to call all callbacks in WebChromeClient destructor.
Probably my preference only, but defining a struct instead of a tuple gives usually more readable code to me.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1056
> +    void requestStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, WTF::CompletionHandler<void(bool)>&& callback);

Might be able to remove WTF::

> Source/WebKit/WebProcess/WebProcess.cpp:1693
> +void WebProcess::didRequestStorageAccessConfirm(uint64_t frameID, uint64_t pageID, uint64_t callbackIdentifier, bool confirmed)

s/confirmed/didConfirm?

> Source/WebKit/WebProcess/WebProcess.h:322
> +    void didRequestStorageAccessConfirm(uint64_t frameID, uint64_t pageID, uint64_t callbackIdentifier, bool hasActivity);

s/hasActivity/didConfirm?

[Brent Fulgham]

Comment on attachment 339697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339697&action=review

>> Source/WebKit/UIProcess/API/APIUIClient.h:131
>> +    virtual void requestStorageAccessConfirm(WebKit::WebPageProxy&, WebKit::WebFrameProxy&, const WTF::String& requestingDomain, const WTF::String& currentDomain, Function<void(bool)>&& completionHandler) { completionHandler(true); }
> 
> This Function could be a CompletionHandler

Done!

>> Source/WebKit/UIProcess/API/C/WKPage.cpp:2028
>> +            RefPtr<RequestStorageAccessConfirmResultListener> listener = RequestStorageAccessConfirmResultListener::create(WTFMove(completionHandler));
> 
> auto listener.  Really.  This would make the code cleaner, and make listener a ref, and you would need to call listener.ptr() below.

Ah! Got it. Thanks.

>> Source/WebKit/UIProcess/Cocoa/UIDelegate.h:91
>> +        void requestStorageAccessConfirm(WebPageProxy&, WebFrameProxy&, const WTF::String& requestingDomain, const WTF::String& currentDomain, Function<void(bool)>&& completionHandler) final;
> 
> CompletionHandler

Done.

>> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1294
>> +    static std::atomic<uint64_t> currentHandlerIdentifier;
> 
> Can this be accessed by more than one thread?  If not (and I think the answer is no) then this doesn't need to be atomic and we don't need m_requestStorageAccessCallbackLock.

I added an assertion that this is only hit on the main thread and removed the Lock and the atomic.

>> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1313
>> +    auto callbackTuple = m_requestStorageAccessCallbacks.take(callbackIdentifier);
> 
> We often exit early if there is no callbakIdentifier in the map.
> Might use find/remove(iterator)

Done.

>> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:366
>> +    Lock m_requestStorageAccessCallbackLock;
> 
> Do we need this lock, maybe all calls are main thread only?

Yes -- removed.

>>> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:367
>>> +    HashMap<uint64_t, std::tuple<String, String, CompletionHandler<void(bool)>>> m_requestStorageAccessCallbacks;
>> 
>> You can just make the value of this HashMap a CompletionHandler and capture subFrameHost and topFrameHost and the given CompletionHandler in a lambda.
> 
> Since this is a map of CompletionHandler, you might need to call all callbacks in WebChromeClient destructor.
> Probably my preference only, but defining a struct instead of a tuple gives usually more readable code to me.

I captured in a lambda as Alex suggested.

>> Source/WebKit/WebProcess/WebPage/WebPage.h:1056
>> +    void requestStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, WTF::CompletionHandler<void(bool)>&& callback);
> 
> Might be able to remove WTF::

Done.

>> Source/WebKit/WebProcess/WebProcess.cpp:1693
>> +void WebProcess::didRequestStorageAccessConfirm(uint64_t frameID, uint64_t pageID, uint64_t callbackIdentifier, bool confirmed)
> 
> s/confirmed/didConfirm?

Done.

>> Source/WebKit/WebProcess/WebProcess.h:322
>> +    void didRequestStorageAccessConfirm(uint64_t frameID, uint64_t pageID, uint64_t callbackIdentifier, bool hasActivity);
> 
> s/hasActivity/didConfirm?

Doh! Yes.

[Brent Fulgham]

Created attachment 339735 [details]
Patch

[Alex Christensen]

Comment on attachment 339735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339735&action=review

I'll fix these up and land it.

> Source/WebCore/page/ChromeClient.h:472
> +    virtual void didRequestStorageAccessConfirm(uint64_t /*frameID*/, uint64_t /*pageID*/, uint64_t /*callbackIdentifier*/, bool /*hasActivity*/) { }

You'll probably want to replace callbackIdentifier with a CompletionHandler of some sort, but we can do that when this is adopted.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1292
> +void WebChromeClient::requestStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, CompletionHandler<void(bool)>&& completionHandler)

It looks like this logic should be moved to the page.

> Source/WebKit/WebProcess/WebProcess.messages.in:139
> +    DidRequestStorageAccessConfirm(uint64_t frameID, uint64_t pageID, uint64_t callbackIdentifier, bool didConfirm)

I think this should be in WebPage.messages.in.

[youenn fablet]

Comment on attachment 339735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339735&action=review

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1297
> +    uint64_t identifier = ++currentHandlerIdentifier;

We now usually try to use typed identifiers using ObjectIdentifier<>.
This allows making sure we are not taking an identifier for another.
Would be good to be done in a follow-up especially as we are handling 3 uint64_t here.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1299
> +    auto result = m_requestStorageAccessCallbacks.ensure(identifier, [this, subFrameHost, topFrameHost, frameID, pageID, completionHandler = WTFMove(completionHandler)]() mutable {

No need for ensure since identifier is unique.
m_requestStorageAccessCallbacks.add should be good enough.
You can do ASSERT(!m_requestStorageAccessCallbacks.contains(identifier)) maybe.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1311
> +    WebProcess::singleton().parentProcessConnection()->send(Messages::WebPageProxy::RequestStorageAccessConfirm(frameID, pageID, subFrameHost, topFrameHost, identifier), m_page.pageID());

Can we have a case where pageID != m_page.pageID?
If not, maybe we should not pass pageID as a method parameter.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1318
> +    auto currentCallbackIter = m_requestStorageAccessCallbacks.find(callbackIdentifier);

s/Iter/Iterator maybe

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:357
> +    void didRequestStorageAccessConfirm(uint64_t frameID, uint64_t pageID, uint64_t callbackIdentifier, bool hasActivity) final;

s/hasActivity/confirmedAccess.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:366
> +    HashMap<uint64_t, CompletionHandler<void(bool)>> m_requestStorageAccessCallbacks;

We probably need to call all completion handlers of m_requestStorageAccessCallbacks at destruction of WebChromeClient.
Otherwise, we might hit debug assertions that the completion handlers were not called.

[Brent Fulgham]

Alex has a slightly revised version he plans to land shortly.

[Alex Christensen]

Comment on attachment 339735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339735&action=review

> Source/WebKit/UIProcess/API/APIUIClient.h:131
> +    virtual void requestStorageAccessConfirm(WebKit::WebPageProxy&, WebKit::WebFrameProxy&, const WTF::String& requestingDomain, const WTF::String& currentDomain, CompletionHandler<void(bool)>&& completionHandler) { completionHandler(true); }

You also have inconsistent defaults.  Here it's true.

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:335
> +        completionHandler(false);

Here it's false.  I'm going to make it false everywhere.

[Alex Christensen]

(In reply to Alex Christensen from comment #30)
> I'm going to make it false everywhere.
Actually true to retain current behavior.

[Brent Fulgham]

Comment on attachment 339735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339735&action=review

>> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:335
>> +        completionHandler(false);
> 
> Here it's false.  I'm going to make it false everywhere.

Let's make it true for now so that we don't have a behavior change until we've completed this work. Right now, storage access is requested and always allowed to proceed to checking with ResourceLoadStatistics to see if it's a tracker. This prompt would prevent that check from happening.

I think tests will fail if we default to false right now.

[Alex Christensen]

Created attachment 339742 [details]
Patch

[Brent Fulgham]

(In reply to Alex Christensen from comment #31)
> (In reply to Alex Christensen from comment #30)
> > I'm going to make it false everywhere.
> Actually true to retain current behavior.

Yes, right.

[Alex Christensen]

Comment on attachment 339742 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339742&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:139
> +    _preferences->setStorageAccessPromptsEnabled(enabled);

And this has no corresponding C SPI.  I'll add one.

[Brent Fulgham]

Comment on attachment 339742 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339742&action=review

>> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:139
>> +    _preferences->setStorageAccessPromptsEnabled(enabled);
> 
> And this has no corresponding C SPI.  I'll add one.

We did have that in the patch originally, but removed it after review comments suggested it wasn't desirable to keep.

[Alex Christensen]

Created attachment 339745 [details]
Patch

[Alex Christensen]

(In reply to Dean Jackson from comment #10)
> Do you need these versions for the legacy C API? I don't think so.
Unfortunately they are still needed.

[Alex Christensen]

Created attachment 339755 [details]
Patch

[John Wilander]

Youenn and I discussed https://bugs.webkit.org/show_bug.cgi?id=185368 and concluded that the prompt code will be called in the UI process to avoid an extra round trip to the web process. Prompting will be done straight from WebKit::WebResourceLoadStatisticsStore through WebKit::WebsiteDataStore. Thus the WebProcess infrastructure in this patch is not needed. It's OK to land it, I can remove it.

[Alex Christensen]

http://trac.webkit.org/r231457

[Brent Fulgham]

Reopening to add follow-up patch.

[Brent Fulgham]

Created attachment 339766 [details]
Patch

[Alex Christensen]

Comment on attachment 339766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339766&action=review

> Source/WebKit/UIProcess/WebPageProxy.cpp:7490
> +        RunLoop::main().dispatch([this, protectedThis = makeRef(*this), requestingDomain = WTFMove(requestingDomain), currentDomain = WTFMove(currentDomain), promptEnabled, wasGranted, frameID, completionHandler = WTFMove(completionHandler)] () mutable {

This should be in WebResourceLoadStatisticsStore::requestStorageAccess

[Brent Fulgham]

Main thread bug is being handled by Bug 185403

[Brent Fulgham]

Re-closing.