Developers have requested two minor changes to the Storage Access API which I believe will also make our implementation closer to Mozillas: 1) Only consume the user gesture when the user explicitly denies access, i.e. when the user is prompted and says "Don't allow." 2) Make document.hasStorageAccess() return true when the feature is off – in the case of Safari, when ITP is off. In addition to this, we have refactoring and cleanup to do. Namely: 1) Make use of WebCore::RegistrableDomain all the way. 2) Remove a such of dead code in WebKit::NetworkProcess since the call now go through NetworkConnectionToWebProcess. 3) Fix some weird naming. 4) Introduce boolean enums for state handling. 5) Break out the Storage Access API functionality into a supplement of Document.
<rdar://problem/50527493>
Created attachment 369233 [details] Patch
Attachment 369233 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:488: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/page/ChromeClient.h:489: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
The style error is because the whole class ChromeClient is marked WEBCORE_EXPORT. There are several inline function in there since before, including the ones I touch here.
Created attachment 369299 [details] Patch
Attachment 369299 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:489: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/page/ChromeClient.h:490: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 369300 [details] Patch
Attachment 369300 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:489: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/page/ChromeClient.h:490: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 369300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369300&action=review r=me with changes. > Source/WebCore/dom/DocumentStorageAccess.cpp:60 > + return "DocumentStorageAccess"_s; I do not think this should have a _s prefix since this returns a const char*. > Source/WebCore/dom/DocumentStorageAccess.cpp:109 > + page->chrome().client().hasStorageAccess(WTFMove(subFrameDomain), WTFMove(topFrameDomain), frameID.value(), pageID.value(), [documentReference = makeWeakPtr(*this), promise = WTFMove(promise)] (bool hasAccess) { documentReference -> weakThis ? > LayoutTests/http/tests/storageAccess/deny-with-prompt-does-not-preserve-gesture-expected.txt:8 > +Some tests failed. This looks bad.
(In reply to Chris Dumez from comment #9) > Comment on attachment 369300 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369300&action=review > > r=me with changes. > > > Source/WebCore/dom/DocumentStorageAccess.cpp:60 > > + return "DocumentStorageAccess"_s; > > I do not think this should have a _s prefix since this returns a const char*. Will fix. > > Source/WebCore/dom/DocumentStorageAccess.cpp:109 > > + page->chrome().client().hasStorageAccess(WTFMove(subFrameDomain), WTFMove(topFrameDomain), frameID.value(), pageID.value(), [documentReference = makeWeakPtr(*this), promise = WTFMove(promise)] (bool hasAccess) { > > documentReference -> weakThis ? Will fix. > > LayoutTests/http/tests/storageAccess/deny-with-prompt-does-not-preserve-gesture-expected.txt:8 > > +Some tests failed. > > This looks bad. Yeah, this is me leaving it behind. I filed https://bugs.webkit.org/show_bug.cgi?id=197670 to track the work to enable this test. Thanks for the review, Chris!
Created attachment 369310 [details] Patch for landing
Comment on attachment 369310 [details] Patch for landing Clearing flags on attachment: 369310 Committed r245025: <https://trac.webkit.org/changeset/245025>
All reviewed patches have been landed. Closing bug.
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1557097 to adopt a behaviour similar to (1) in Gecko. I believe (2) is what we do already.