Bug 192906 - Add diagnostic logging for safe browsing provider and action
Summary: Add diagnostic logging for safe browsing provider and action
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-19 17:47 PST by Alex Christensen
Modified: 2018-12-21 15:44 PST (History)
1 user (show)

See Also:


Attachments
Patch (16.69 KB, patch)
2018-12-19 17:51 PST, Alex Christensen
achristensen: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.