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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Nov 13 13:59:42 PST 2020

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

Attachment 413893: Patch


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

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 =

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 =

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


> 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

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

Ditto with naming.

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


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


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


More information about the webkit-reviews mailing list