[webkit-reviews] review denied: [Bug 220106] Can't login to Skype from Microsoft Outlook account in Safari : [Attachment 416695] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 22 16:35:59 PST 2020

Alex Christensen <achristensen at apple.com> has denied
katherine_cheney at apple.com's request for review:
Bug 220106: Can't login to Skype from Microsoft Outlook account in Safari

Attachment 416695: Patch


--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 416695
  --> https://bugs.webkit.org/attachment.cgi?id=416695

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

> Source/WebCore/page/Quirks.cpp:976
> +	   return element.hasClass() &&
(element.classNames().contains("glyph_signIn_circle") ||
element.classNames().contains("mectrl_headertext") ||
element.classNames().contains("mectrl_header") ||
element.classNames().contains("ext-button primary") ||

This would probably be easier to read if everything after "return
element.hasClass() && (" had new lines.

> Source/WebCore/page/Quirks.cpp:981
> +	   return element.hasClass() &&
(element.classNames().contains("_3ioEp2RGR5vb0gqRDsaFPa") ||

Are these static?  They look like they might change.

> Source/WebCore/page/Quirks.cpp:1038
> +   
WTFMove(domainInNeedOfStorageAccess), [firstPartyDomain,
domainInNeedOfStorageAccess, completionHandler =
WTFMove(completionHandler)](StorageAccessWasGranted storageAccessGranted)
mutable {

m_document is a WeakPtr.  We should probably check if it's null.

> Source/WebCore/page/Quirks.cpp:1040
> +	       return;

You need to call completionHandler here.

> Source/WebCore/page/Quirks.cpp:1125
> +	       return requestStorageAccessAndHandleClick([&element,
platformEvent, eventType, detail, relatedTarget] {

Will this lambda be called immediately always?	If not, it's unsafe to capture
element by reference.  If yes, you may as well just use [&] instead of listing
all captured variables.

> Source/WebCore/page/Quirks.cpp:1136
> +		   auto domWindow = document->domWindow();

document is a WeakPtr.	We should probably check if it's null.

> Source/WebCore/page/Quirks.cpp:1143
> +		       if (is<Frame>(*abstractFrame)) {

I think we shouldn't dereference abstractFrame here without checking it.  Just
use is<Frame>(abstractFrame)

> Source/WebCore/platform/network/NetworkStorageSession.cpp:434
> +    static NeverDestroyed<HashMap<String, RegistrableDomain>> map = [] {

Since this map is never added to after initializing, it seems like overkill to
even have a map here.  Why don't we just have if (urlToMap.host() ==
"login.live.com") return "microsoft.com"; return urlToMap;  or something like
that.  I also think this should live in Quirks rather than
NetworkStorageSession because it has nothing to do with storage.

More information about the webkit-reviews mailing list