Created attachment 357767 [details] Patch

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 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().

(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.