Bug 234439 – Fix Safari-side SafeBrowsing telemetry

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eliot Hsu]

Telemetry for Safari Safe Browsing was previously handled in Safari. Now that WebKit decides whether or not to show Safe Browsing warnings, and queries the Safe Browsing Service itself, we should pass along the (anonymized and privacy-preserving) telemetry to Safari to fix the metrics that were previously there.

[Eliot Hsu]

Created attachment 447466 [details]
Patch

[Eliot Hsu]

Created attachment 447631 [details]
Patch

[Alex Christensen]

Comment on attachment 447631 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1801
>          if (navigatesFrame && forMainFrameNavigation) {
>              // The safe browsing warning will be hidden once the next page is shown.
> +            bool continuingUnsafeLoad = WTF::switchOn(result,
> +                [] (ContinueUnsafeLoad continueUnsafeLoad) { return continueUnsafeLoad == ContinueUnsafeLoad::Yes; },
> +                [] (const URL&) { return false; }
> +            );

If navigatesFrame is true, then we know that result is ContinueUnsafeLoad::Yes.  This check seems unnecessary.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1806
> +            else
> +                dictionary.set("action"_s, String("redirect to url"));

And this seems unreachable.

[Eliot Hsu]

Comment on attachment 447631 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1801
>> +            );
> 
> If navigatesFrame is true, then we know that result is ContinueUnsafeLoad::Yes.  This check seems unnecessary.

In the case where the Safe Browsing warning page shows, and the user clicks on the "Learn more..." or "report an error" links instead of "Visit this unsafe website", then `navigatesFrame` and `forMainFrameNavigation` are both true, but `continueUnsafeLoad` is `ContinueUnsafeLoad::No`.

[Alex Christensen]

Comment on attachment 447631 [details]
Patch

yep, I read it wrong the first time.

[EWS]

Committed r287333 (245479@main): <https://commits.webkit.org/245479@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447631 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86786182>