Bug 227099 - Storage Access quirks should prompt up to twice if a user does not allow storage access
Summary: Storage Access quirks should prompt up to twice if a user does not allow stor...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-16 15:52 PDT by Kate Cheney
Modified: 2021-06-17 13:04 PDT (History)
9 users (show)

See Also:


Attachments
Patch (16.13 KB, patch)
2021-06-16 15:54 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (21.27 KB, patch)
2021-06-17 10:51 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (17.94 KB, patch)
2021-06-17 12:23 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2021-06-16 15:52:34 PDT
Storage Access quirks should prompt up to twice if a user does not allow storage access
Comment 1 Kate Cheney 2021-06-16 15:54:44 PDT
Created attachment 431617 [details]
Patch
Comment 2 John Wilander 2021-06-16 17:21:47 PDT
Comment on attachment 431617 [details]
Patch

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

r=me with comments.

> Source/WebCore/ChangeLog:12
> +        patch removes this code and utilizes maxNumberOfTimesExplicitlyDeniedFrameSpecificStorageAccess

We could probably rename it to just maxNumberOfTimesExplicitlyDeniedStorageAccess now that we don't do frame-specific storage access and the member variable inherently belongs to the document.

> Source/WebCore/dom/DocumentStorageAccess.cpp:258
> +    if (!m_document.frame() || !m_document.frame()->page() || !isAllowedToRequestFrameSpecificStorageAccess()) {

This too could probably be renamed isAllowedToRequestStorageAccess().

> Source/WebCore/dom/Element.h:68
> +enum class IsSyntheticClick : bool { No, Yes };

Nice. I like these boolean abstractions.

> Source/WebCore/loader/ResourceLoadObserver.h:-76
> -    virtual bool hasDeniedCrossPageStorageAccess(const SubResourceDomain&, const TopFrameDomain&) const { return false; }

These have no other implementations than in WebResourceLoadObserver, right?

> Source/WebCore/page/Quirks.cpp:1225
> +        // If the click is synthetic, the user has already gone through the storage access flow. We should not request again.

I think you should rephrase to "… the storage access flow and we should not …" to make sure that it's clear that those two result from the condition.
Comment 3 Kate Cheney 2021-06-17 09:57:28 PDT
(In reply to John Wilander from comment #2)
> Comment on attachment 431617 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431617&action=review
> 
> r=me with comments.

Thanks for the review!

> 
> > Source/WebCore/ChangeLog:12
> > +        patch removes this code and utilizes maxNumberOfTimesExplicitlyDeniedFrameSpecificStorageAccess
> 
> We could probably rename it to just
> maxNumberOfTimesExplicitlyDeniedStorageAccess now that we don't do
> frame-specific storage access and the member variable inherently belongs to
> the document.
> 

Good idea, will change.

> > Source/WebCore/dom/DocumentStorageAccess.cpp:258
> > +    if (!m_document.frame() || !m_document.frame()->page() || !isAllowedToRequestFrameSpecificStorageAccess()) {
> 
> This too could probably be renamed isAllowedToRequestStorageAccess().

Ditto, will fix.

> 
> > Source/WebCore/dom/Element.h:68
> > +enum class IsSyntheticClick : bool { No, Yes };
> 
> Nice. I like these boolean abstractions.

 :) 

> 
> > Source/WebCore/loader/ResourceLoadObserver.h:-76
> > -    virtual bool hasDeniedCrossPageStorageAccess(const SubResourceDomain&, const TopFrameDomain&) const { return false; }
> 
> These have no other implementations than in WebResourceLoadObserver, right?
> 

Right, this patch deletes the entire original implementation of storing denied storage access in the web process now that we use the Document.

> > Source/WebCore/page/Quirks.cpp:1225
> > +        // If the click is synthetic, the user has already gone through the storage access flow. We should not request again.
> 
> I think you should rephrase to "… the storage access flow and we should not
> …" to make sure that it's clear that those two result from the condition.

Will do.
Comment 4 Kate Cheney 2021-06-17 10:51:34 PDT
Created attachment 431690 [details]
Patch for landing
Comment 5 Kate Cheney 2021-06-17 12:23:40 PDT
Created attachment 431702 [details]
Patch for landing
Comment 6 EWS 2021-06-17 13:03:14 PDT
Committed r279004 (238929@main): <https://commits.webkit.org/238929@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431702 [details].
Comment 7 Radar WebKit Bug Importer 2021-06-17 13:04:36 PDT
<rdar://problem/79466721>