[webkit-reviews] review granted: [Bug 227099] Storage Access quirks should prompt up to twice if a user does not allow storage access : [Attachment 431617] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted Kate Cheney
<katherine_cheney at apple.com>'s request for review:
Bug 227099: Storage Access quirks should prompt up to twice if a user does not
allow storage access
https://bugs.webkit.org/show_bug.cgi?id=227099

Attachment 431617: Patch

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




--- Comment #2 from John Wilander <wilander at apple.com> ---
Comment on attachment 431617
  --> https://bugs.webkit.org/attachment.cgi?id=431617
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.