Bug 191441 – [iOS] Implement safe browsing in WebKit

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

[iOS] Implement safe browsing in WebKit

[Alex Christensen]

Created attachment 354284 [details]
Patch

[Alex Christensen]

Comment on attachment 354284 [details]
Patch

The UI isn't quite right on this first patch, but it works!

[Alex Christensen]

Created attachment 354348 [details]
Patch

[Alex Christensen]

Comment on attachment 354348 [details]
Patch

iOS UI still isn't quite right, but much closer!

[Alex Christensen]

Created attachment 354402 [details]
Patch

[Alex Christensen]

So close!  I need to use UIUserInterfaceSizeClass instead of hard-coding a narrow width, listen for orientation transitions, and make it scroll on iOS, but other than that it's good!  Preliminary reviews welcome.

[Alex Christensen]

Created attachment 354436 [details]
Patch

[Wenson Hsieh]

Comment on attachment 354436 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:279
> +        NSUnderlineStyleAttributeName: @(NSUnderlineStyleSingle),
> +        NSUnderlineColorAttributeName: [UIColor whiteColor],
> +        NSForegroundColorAttributeName: colorForItem(item, warning),

Nit - extra spaces after the :

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:316
> +@interface WKSafeBrowsingTextViewDelegate : NSObject<NSTextViewDelegate>

It's not common practice in ObjC to create a new instance of a class whose sole purpose is to be a delegate. I would've thought that the safe browsing view should be the text view delegate. Then, you wouldn't need this extra bit of plumbing when handling a click:

[((WKSafeBrowsingTextView *)textView)->_warning clickedOnLink:link];

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:456
> +    for (ViewType *subview in self.subviews) {

It seems strange to go digging around in the view hierarchy in search of a view that we created and added ourselves. Can we instead store a pointer to the WKSafeBrowsingTextView above and just invalidate its intrinsic content size here?

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:560
> +    constexpr auto noHeightConstraint = 1000000;

Perhaps CGFLOAT_MAX instead of an arbitrary large number?

[Alex Christensen]

Created attachment 354442 [details]
Patch

[Alex Christensen]

Created attachment 354446 [details]
Patch

[Alex Christensen]

Created attachment 354449 [details]
Patch

[Alex Christensen]

Created attachment 354450 [details]
Patch

[Alex Christensen]

Created attachment 354452 [details]
Patch

[Tim Horton]

Comment on attachment 354452 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/mac/WKContextPrivateMac.mm:176
> -    return DEFAULT_SAFE_BROWSING_ENABLED;
> +    return true;

Still don't understand why this doesn't follow the preference (or why you have a preference if you're not going to follow it), but OK

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:3604
> +    if (_safeBrowsingWarning)

Why is this not in the Impl? This way it's going to make the eventual plop harder, and also means WKView doesn't get it.

And these aren't used on iOS so they can't be relevant to this patch.

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:169
> +    [exclamationPoint appendBezierPathWithArcWithCenter: { centerX, lineBottomCenterY } radius:lineRadius startAngle:0 endAngle:180 clockwise:YES];

Choose one space-after-colon style and stick to it (and it should be "no").

[Alex Christensen]

Created attachment 354590 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r238115

[Radar WebKit Bug Importer]

<rdar://problem/46010524>

[Alex Christensen]

http://trac.webkit.org/r238133