If a user denies storage access on sites with the storage access quirk, we cancel the click and they can't login. We should consider changing this behavior, because some sites which require storage access share login flows with sites that do not require storage access.
<rdar://problem/75619058>
Created attachment 423729 [details] Patch
Comment on attachment 423729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423729&action=review > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3829 > +bool WebPage::pageIsParentProcessAFullWebBrowser() I don't think we should have a "page" prefix for WebPage functions. This is not a common pattern. > Source/WebKit/WebProcess/WebPage/WebPage.h:1416 > + bool pageIsParentProcessAFullWebBrowser(); Can this be const?
(In reply to Chris Dumez from comment #3) > Comment on attachment 423729 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423729&action=review > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3829 > > +bool WebPage::pageIsParentProcessAFullWebBrowser() > > I don't think we should have a "page" prefix for WebPage functions. This is > not a common pattern. I needed some way to distinguish it from the isParentProcessAFullWebBrowser function in DefaultWebBrowserChecks.h. I can use something slightly different like isParentProcessAWebBrowser(). > > > Source/WebKit/WebProcess/WebPage/WebPage.h:1416 > > + bool pageIsParentProcessAFullWebBrowser(); > > Can this be const? Yes.
Created attachment 423744 [details] Patch
Created attachment 423761 [details] Patch
Comment on attachment 423761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423761&action=review r=me with comments. > Source/WebCore/ChangeLog:15 > + 2. Allow login even if the user denies storage access. Previously we I wouldn't say "allow login" since the browser doesn't decide that. I'd say "don't cancel the click" instead. > Source/WebKit/ChangeLog:10 > + allow login on sites even if the user denies storage access. See Ditto on allowing login. > Source/WebCore/dom/Element.cpp:400 > + isParentProcessAFullWebBrowser = true; This can be just isParentProcessAFullWebBrowser = MacApplication::isSafari(). > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1962 > + auto* page = m_frame->page(); I looked and other functions use m_frame directly too so should be fine.
(In reply to John Wilander from comment #7) > Comment on attachment 423761 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423761&action=review > > r=me with comments. Thanks! I am also going to replace "tab" with "web content process" in the ChangeLog because a web content process can host multiple tabs sometimes. > > > Source/WebCore/ChangeLog:15 > > + 2. Allow login even if the user denies storage access. Previously we > > I wouldn't say "allow login" since the browser doesn't decide that. I'd say > "don't cancel the click" instead. Good call. > > > Source/WebKit/ChangeLog:10 > > + allow login on sites even if the user denies storage access. See > > Ditto on allowing login. Will fix. > > > Source/WebCore/dom/Element.cpp:400 > > + isParentProcessAFullWebBrowser = true; > > This can be just isParentProcessAFullWebBrowser = MacApplication::isSafari(). > Will fix. > > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1962 > > + auto* page = m_frame->page(); > > I looked and other functions use m_frame directly too so should be fine. Yes, it's a Ref so it can't be null.
Created attachment 423786 [details] Patch for landing
Committed r274746: <https://commits.webkit.org/r274746> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423786 [details].