Bug 192906

Summary: Add diagnostic logging for safe browsing provider and action
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: NEW ---    
Severity: Normal CC: cdumez, jonlee, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch achristensen: review-

Description Alex Christensen 2018-12-19 17:47:46 PST
Add diagnostic logging for safe browsing provider and action
Comment 1 Alex Christensen 2018-12-19 17:51:41 PST
Created attachment 357767 [details]
Patch
Comment 2 Chris Dumez 2018-12-19 19:19:11 PST
Comment on attachment 357767 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1635
> +            weakThis->m_page->logDiagnosticMessageWithResult(WebCore::DiagnosticLoggingKeys::safeBrowsingAction(), emptyString(), *action, WebCore::ShouldSample::No);

I am not convinced this works, the 3rd parameter to logDiagnosticMessageWithResult() is a DiagnosticLoggingResultType, you're passing it a uint32_t. We should try to match whatever logging you're replacing (make sure the underlying logging method called is the same, and ideally even the key / value).
Comment 3 Chris Dumez 2018-12-19 19:33:24 PST
Comment on attachment 357767 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1627
> +        m_page->logDiagnosticMessageWithResult(WebCore::DiagnosticLoggingKeys::safeBrowsingProvider(), emptyString(), *identifier, WebCore::ShouldSample::No);

E.g., to try and match previous code, I think we should pass "SafeBrowsing" as key (first parameter).
Then the second parameter should be "Showed Warning".
Then the third parameter would be the provider name as a String.

We would call WebPageProxy::logDiagnosticMessageWithValue() instead of logDiagnosticMessageWithResult(). We'd need to add an WebPageProxy::logDiagnosticMessageWithValue() overload which takes a 3 String parameters though but that no big deal since those are the types used internally.

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1635
>> +            weakThis->m_page->logDiagnosticMessageWithResult(WebCore::DiagnosticLoggingKeys::safeBrowsingAction(), emptyString(), *action, WebCore::ShouldSample::No);
> 
> I am not convinced this works, the 3rd parameter to logDiagnosticMessageWithResult() is a DiagnosticLoggingResultType, you're passing it a uint32_t. We should try to match whatever logging you're replacing (make sure the underlying logging method called is the same, and ideally even the key / value).

E.g., to try and match previous code, I think we should pass "SafeBrowsing" as key (first parameter).
Then the second parameter should be "User Ignored Warning" / "User Went Back" / "User Closed Page".
And we'd call logDiagnosticMessage() instead of logDiagnosticMessageWithResult().
Comment 4 Chris Dumez 2018-12-19 19:42:59 PST
(In reply to Chris Dumez from comment #3)
> Comment on attachment 357767 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357767&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1627
> > +        m_page->logDiagnosticMessageWithResult(WebCore::DiagnosticLoggingKeys::safeBrowsingProvider(), emptyString(), *identifier, WebCore::ShouldSample::No);
> 
> E.g., to try and match previous code, I think we should pass "SafeBrowsing"
> as key (first parameter).
> Then the second parameter should be "Showed Warning".
> Then the third parameter would be the provider name as a String.
> 
> We would call WebPageProxy::logDiagnosticMessageWithValue() instead of
> logDiagnosticMessageWithResult(). We'd need to add an
> WebPageProxy::logDiagnosticMessageWithValue() overload which takes a 3
> String parameters though but that no big deal since those are the types used
> internally.
> 
> >> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1635
> >> +            weakThis->m_page->logDiagnosticMessageWithResult(WebCore::DiagnosticLoggingKeys::safeBrowsingAction(), emptyString(), *action, WebCore::ShouldSample::No);
> > 
> > I am not convinced this works, the 3rd parameter to logDiagnosticMessageWithResult() is a DiagnosticLoggingResultType, you're passing it a uint32_t. We should try to match whatever logging you're replacing (make sure the underlying logging method called is the same, and ideally even the key / value).
> 
> E.g., to try and match previous code, I think we should pass "SafeBrowsing"
> as key (first parameter).
> Then the second parameter should be "User Ignored Warning" / "User Went
> Back" / "User Closed Page".
> And we'd call logDiagnosticMessage() instead of
> logDiagnosticMessageWithResult().

Posted some comments on the radar as well.
Comment 5 Jon Lee 2019-04-01 16:38:37 PDT
rdar://problem/46287888