API Review: Diagnostic Monitor API#5565
API Review: Diagnostic Monitor API#5565chetanpandey1266 wants to merge 3 commits intouser/chetanpandey/DiagnosticMonitorAPIfrom
Conversation
shrinaths
left a comment
There was a problem hiding this comment.
Prose, Grammar, and Voice Review
Line-by-line pass focused on grammar, tone, and narrative voice consistency. Suggested corrections are inline below. The overall prose quality is good — these are polish items to bring the spec to Microsoft documentation standard.
shrinaths
left a comment
There was a problem hiding this comment.
Code Sample Issue: The destructor calls m_monitor.Reset() without first calling remove_DiagnosticReceived. This teaches an anti-pattern — callbacks may fire during or after destruction. Best-practice samples should demonstrate proper cleanup order.
Suggested correction:
DiagnosticComponent::~DiagnosticComponent()
{
if (m_monitor)
{
// Unsubscribe before releasing the monitor to
// ensure no callbacks arrive during teardown.
m_monitor->remove_DiagnosticReceived(
m_diagnosticToken);
m_monitor.Reset();
}
}| /// `protocol` is the protocol scheme (e.g. "https"). | ||
| /// `uri` is the request URI. | ||
| /// | ||
| /// For categories the runtime does not yet populate, |
There was a problem hiding this comment.
Blocking API Issue: Scope tells the kind of source (WebView/Profile/Environment) but not which instance produced the event. The filter supports profileName but the returned details JSON does not include it -- so you can filter by profile but cannot attribute received events to a profile. Add ProfileName and WebViewId properties to the event args: [propget] HRESULT ProfileName([out, retval] LPWSTR* value); [propget] HRESULT WebViewId([out, retval] LPWSTR* value); ProfileName should return empty string when Scope is Environment. WebViewId should return empty string when Scope is not WebView.
There was a problem hiding this comment.
I agree that we can include profile name as an entry in JSON but for CoreWebView2, not sure which unique id might help here. We can have frame id or something but considering we are only having network error codes for now, would not include that here. Any other enum which might require such details can simply extend the json and add these details.
…onvention alignment - Fix grammar: 'An empty JSON' -> 'An empty JSON object' (2x) - Clarify ambiguity: 'mixed-content blocks' -> 'mixed-content blocked requests' - Use prescriptive language: 'should match' -> 'must match' in API contract - Add missing 'that' in relative clause + imperative voice for CoTaskMemFree - Remove markdown bold (**any**) from IDL comments (plain text only) - Add missing comma after introductory phrase 'On failure' - Split run-on sentence in event handler comment - Add missing comma before 'but' in compound sentence - Remove 'NetworkContext' reference from profile scope comment - C++ destructor: unsubscribe before Reset() for proper cleanup order - C# Dispose: unsubscribe before Dispose() for proper cleanup order - Add _environment field declaration in filtered C# sample - Name opaque error codes: ERR_NAME_NOT_RESOLVED (-105), ERR_TIMED_OUT (-7) - Align section headers: 'COM' -> 'Win32 C++', '.NET C#' -> '.NET, WinRT' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c4ce409 to
0894cc0
Compare
Add Diagnostic Monitor API spec — Introduces ICoreWebView2DiagnosticMonitor, a new observation-only API that
delivers diagnostic signals (e.g., network failures) from all WebViews, profiles, and the environment through a
single DiagnosticReceived event. Host apps create a monitor via CreateDiagnosticMonitor and opt in per category
using AddDiagnosticReceivedFilter.