Bug 197648 – Storage Access API: Make two changes requested by developers and complete refactoring and cleanup

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

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.

[Radar WebKit Bug Importer]

<rdar://problem/50527493>

[John Wilander]

Created attachment 369233 [details]
Patch

[EWS Watchlist]

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.

[John Wilander]

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.

[John Wilander]

Created attachment 369299 [details]
Patch

[EWS Watchlist]

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.

[John Wilander]

Created attachment 369300 [details]
Patch

[EWS Watchlist]

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.

[Chris Dumez]

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.

[John Wilander]

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

[John Wilander]

Created attachment 369310 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 369310 [details]
Patch for landing

Clearing flags on attachment: 369310

Committed r245025: <https://trac.webkit.org/changeset/245025>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ehsan Akhgari [:ehsan]]

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.