Bug 197648

Summary: Storage Access API: Make two changes requested by developers and complete refactoring and cleanup
Product: WebKit Reporter: John Wilander <wilander>
Component: WebKit Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, ehsan, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=197670
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description John Wilander 2019-05-06 22:06:44 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-05-06 22:07:10 PDT
<rdar://problem/50527493>
Comment 2 John Wilander 2019-05-06 22:32:31 PDT
Created attachment 369233 [details]
Patch
Comment 3 EWS Watchlist 2019-05-06 22:35:26 PDT
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.
Comment 4 John Wilander 2019-05-06 22:35:55 PDT
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.
Comment 5 John Wilander 2019-05-07 10:03:48 PDT
Created attachment 369299 [details]
Patch
Comment 6 EWS Watchlist 2019-05-07 10:06:24 PDT
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.
Comment 7 John Wilander 2019-05-07 10:34:25 PDT
Created attachment 369300 [details]
Patch
Comment 8 EWS Watchlist 2019-05-07 10:38:00 PDT
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 9 Chris Dumez 2019-05-07 11:54:27 PDT
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.
Comment 10 John Wilander 2019-05-07 12:13:10 PDT
(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!
Comment 11 John Wilander 2019-05-07 12:14:25 PDT
Created attachment 369310 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2019-05-07 12:55:37 PDT
Comment on attachment 369310 [details]
Patch for landing

Clearing flags on attachment: 369310

Committed r245025: <https://trac.webkit.org/changeset/245025>
Comment 13 WebKit Commit Bot 2019-05-07 12:55:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Ehsan Akhgari [:ehsan] 2019-06-05 10:46:41 PDT
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.