[webkit-reviews] review granted: [Bug 218778] Can't login to Microsoft Teams : [Attachment 413893] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted katherine_cheney at apple.com's
request for review:
Bug 218778: Can't login to Microsoft Teams
https://bugs.webkit.org/show_bug.cgi?id=218778

Attachment 413893: Patch

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




--- Comment #15 from John Wilander <wilander at apple.com> ---
Comment on attachment 413893
  --> https://bugs.webkit.org/attachment.cgi?id=413893
Patch

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

r=me with comments.

> Source/WebCore/dom/DocumentStorageAccess.cpp:269
> +    auto topDomain =
NetworkStorageSession::mapToTopDomain(RegistrableDomain::uncheckedCreateFromHos
t(m_document.topDocument().securityOrigin().host()));

Why not use the RegistrableDomain(const URL& url) constructor? We should use
the unchecked* calls as little as possible.

> Source/WebCore/page/Quirks.cpp:960
> +    static NeverDestroyed<RegistrableDomain> microsoftLiveDomain =
RegistrableDomain::uncheckedCreateFromRegistrableDomainString("live.com"_s);

I would call these microsoftDotCom and liveDotCom since both are Microsoft
domains and your function is called isMicrosoftDomain().

> Source/WebCore/page/Quirks.h:30
> +#include <wtf/Optional.h>

There doesn't seem to be any new usage of these two in this header file. Could
these includes be moved to the implementation file?

> Source/WebKit/Shared/WebProcessDataStoreParameters.h:80
> +    encoder << domainsWithStorageAccessQuirk;

It's not just that these have a quirk, right? They also have cross-page access.
See below for comments on naming. I think it's kind of leaking in abstraction
here. These lower layers don't really have to know naming-wise that this is a
quirk. Here you can probably name things for what they are. The code higher up
that actually reasons about quirks, accessing the hardcoded domains, can call
things quirky. This is mostly infrastructure to support the quirk in my view.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:203
> +    void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain,
WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&);

See below on naming.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:51
> +    SetDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain,
WebCore::RegistrableDomain> domains) -> () Async

Ditto.

> Source/WebKit/UIProcess/WebProcessPool.h:464
> +    void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain,
WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&);

It looks strange adding this. One wonders why this wasn't needed before. Isn't
it the case here that you need this because these domains have *persistent* or
at least *cross-page* storage access? That should go into the function name
such as setDomainsWithCrossPageStorageAccess(). I would also like some
syntactic sugar with "using" here so that you can make it clear in the header
which of the domains is the first party and which is the third party.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.h:66
> +    void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain,
WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&) final;

Same thing here with function naming. I believe this is needed for the extended
scope of the access because the same WebProcess will be used for all matching
webpages.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.h:87
> +    HashMap<WebCore::RegistrableDomain, WebCore::RegistrableDomain>
m_domainsWithStorageAccess;

Ditto with naming.

> Source/WebKit/WebProcess/WebProcess.cpp:2011
> +void WebProcess::setDomainsWithStorageAccess(HashMap<RegistrableDomain,
RegistrableDomain>&& domains, CompletionHandler<void()>&& completionHandler)

Ditto.

> Source/WebKit/WebProcess/WebProcess.h:499
> +    void setDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain,
WebCore::RegistrableDomain>&&, CompletionHandler<void()>&&);

Ditto.

> Source/WebKit/WebProcess/WebProcess.messages.in:166
> +    SetDomainsWithStorageAccess(HashMap<WebCore::RegistrableDomain,
WebCore::RegistrableDomain> domains) -> () Async

Ditto.