Cannot login to microsoftonline.com without allowing storage access

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

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.

[Radar WebKit Bug Importer]

<rdar://problem/75619058>

[Kate Cheney]

Created attachment 423729 [details]
Patch

[Chris Dumez]

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?

[Kate Cheney]

(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.

[Kate Cheney]

Created attachment 423744 [details]
Patch

[Kate Cheney]

Created attachment 423761 [details]
Patch

[John Wilander]

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.

[Kate Cheney]

(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.

[Kate Cheney]

Created attachment 423786 [details]
Patch for landing

[EWS]

Committed r274746: <https://commits.webkit.org/r274746>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423786 [details].