[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
https://bugs.webkit.org/show_bug.cgi?id=220106

Attachment 416695: Patch

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




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

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") ||
element.classNames().contains("ext-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") ||
element.classNames().contains("_2Am2jvTaBz17UJ8XnfxFOy"));

Are these static?  They look like they might change.

> Source/WebCore/page/Quirks.cpp:1038
> +   
DocumentStorageAccess::requestStorageAccessForNonDocumentQuirk(*m_document,
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