[webkit-reviews] review granted: [Bug 188871] Implement safe browsing in WebKit : [Attachment 353870] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 5 10:25:41 PST 2018


Tim Horton <thorton at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 188871: Implement safe browsing in WebKit
https://bugs.webkit.org/show_bug.cgi?id=188871

Attachment 353870: Patch

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




--- Comment #54 from Tim Horton <thorton at apple.com> ---
Comment on attachment 353870
  --> https://bugs.webkit.org/attachment.cgi?id=353870
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6657
> +- (NSView *)_safeBrowsingWarningForTesting

This obviously isn't going to fly on iOS

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:200
> ++ (instancetype)viewWithAttributedString:(NSMutableAttributedString
*)attributedString linkTarget:(WKSafeBrowsingWarning *)target;

Why mutable! (Ideally there would be a mutableCopy hiding inside here, and the
caller's string wouldn't get mutated)

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:295
> +    _completionHandler(makeUnexpected(link));

Alex says "I could use a variant instead of Expected"

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:302
> +    NSStackView *bottom =box.views.lastObject;

Missing a space after the =

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1609
> +	       weakThis->m_ignoresNonWheelEvents = oldIgnoresNonWheelEvents;

?? do you want to allow scrolling (but not other events) ??


More information about the webkit-reviews mailing list