Storage Access quirks should prompt up to twice if a user does not allow storage access
Created attachment 431617 [details] Patch
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.
(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.
Created attachment 431690 [details] Patch for landing
Created attachment 431702 [details] Patch for landing
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].
<rdar://problem/79466721>