API Review: Diagnostic Monitor API#5565
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();
}
}…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
daaab37 to
82a6439
Compare
- Rename COREWEBVIEW2_DIAGNOSTIC_SCOPE_WEB_VIEW to ..._WEBVIEW to match
existing one-word convention (e.g. COREWEBVIEW2_*).
- Rename JSON field "protocol" to "scheme" and update prose to "URI scheme".
- Strengthen errorCode documentation: mark net_error_list.h as the
authoritative source, note stability and additive evolution, and
instruct consumers to treat unknown codes as generic failures.
- Convert GetDetailsAsJson() to a DetailsAsJson property (COM [propget]
and WinRT { get; }); update C++ and .NET samples accordingly.
- Drop the "returns {} for unrecognized categories" sentence; only one
populated category exists.
- Add HRESULT Close() to ICoreWebView2DiagnosticMonitor, mirroring the
ICoreWebView2SharedBuffer pattern for deterministic teardown.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… Timestamp, drop profileName - Rename COREWEBVIEW2_DIAGNOSTIC_CATEGORY_NETWORK_ERROR to ..._NETWORK_REQUEST (WinRT: NetworkError -> NetworkRequest). The category emits a per-request lifecycle signal, not failures only; errorCode == 0 means success. - Rewrite the category description to spell out exactly which requests fire it (navigation, sub-resource, fetch/XHR, dedicated/shared workers, attached service workers) and what is excluded (other host apps sharing the user data folder, CSP violation reports). - Switch Timestamp to wall-clock: COM double seconds since UNIX epoch, WinRT Windows.Foundation.DateTime. Enables correlation with other timestamped telemetry sources. - Drop profileName from the filter schema, both code samples, and intro prose to keep the filter consistent with the emitted Details. - Update intro and example prose to refer to "network requests" rather than "network errors". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The filter JSON schema and the DetailsAsJson schema vary per category, so they belong on each COREWEBVIEW2_DIAGNOSTIC_CATEGORY value rather than on the category-agnostic methods. As we add more categories the method-level docs would otherwise grow into a per-category dumping ground. - Move NETWORK_REQUEST filter schema (errorCode/statusCode/uriPattern/ httpMethod, wildcard rules, AND/OR semantics) and Details schema (errorCode/statusCode/httpMethod/elapsedTime/scheme/uri, with field descriptions) onto the COREWEBVIEW2_DIAGNOSTIC_CATEGORY_NETWORK_REQUEST enum value. Update the top-of-enum doc comment to point readers there. - Strip SetDiagnosticFilter down to category-agnostic behavior (empty filter = match all, replace-on-recall, E_INVALIDARG on bad JSON or schema mismatch) and link to the enum for the schema. - Strip DetailsAsJson down to category-agnostic behavior and link to the enum for the schema. - Mirror the same restructure on the WinRT enum and DetailsAsJson property. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rewrite the Background and Description sections to position the Diagnostic Monitor API as an on-demand field diagnostics surface for non-functioning deployments, not as a general error-handling or telemetry mechanism. Clarifies that the API is complementary and tangential to existing error-handling APIs and is intended to be turned on externally to capture detailed diagnostic data from a specific misbehaving instance. Also justifies the JSON-in / JSON-out shape as the right contract for shipping new categories and fields to deployments already in the field, and notes that monitors stay inert until SetDiagnosticFilter is called. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rewrite the Description in third-person declarative voice matching
the convention used in other WebView2Feedback specs (CookieManagement,
ClearBrowsingData, CustomDownload, Autofill). Replace "you/we" and
phrases like "Its purpose is", "matters for", "shape is intentional"
with named subjects ("The monitor", "The host app", "A monitor")
and direct statements. Preserves all content and approximate length.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| error paths. When that happens, the only recourse is detailed | ||
| logging information from the affected WebView2 instance, often | ||
| behind a security boundary that prevents the host app itself | ||
| from collecting it inline. |
There was a problem hiding this comment.
"security boundary prevents host app itself from collecting it inline" needs to be rephrased.
At the end of the day, its the host app that collects and uploads logs and telemetry.
I suppose what you are trying to say is, at present there is no way the host app can collect additional logging information from webview2 that highlights events that lead to a failure
|
|
||
| None of today's WebView2 APIs cover this scenario. Existing | ||
| error-handling APIs such as `ServerCertificateErrorDetected` are | ||
| interactive, per-instance, and shaped around in-flow decisions — |
| None of today's WebView2 APIs cover this scenario. Existing | ||
| error-handling APIs such as `ServerCertificateErrorDetected` are | ||
| interactive, per-instance, and shaped around in-flow decisions — | ||
| not around capturing rich, free-form diagnostic data from a |
There was a problem hiding this comment.
"free-form diagnostic data" => capturing rich, varying diagnostic information that can be logged.
| misbehaving deployment after the fact. | ||
|
|
||
| The Diagnostic Monitor API addresses this gap. It is an | ||
| **observation-only** surface that lets a host app opt in, |
- Rephrase 'security boundary ... collecting it inline' to clarify that today there is no built-in way for the host app to collect this additional WebView2 logging data that is not available otherwise. - Reword 'in-flow decisions' to 'real-time decisions'. - Replace 'free-form diagnostic data' with 'varying diagnostic information that can be logged'. - Replace 'observation-only' with 'logging' (both occurrences) and update 'A monitor observes' to 'A monitor captures' for consistency with the new logging framing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- COM IDL: change Timestamp from double seconds to INT64 milliseconds since the UNIX epoch. Aligns with the INT64 timestamp variable already used in the Win32 C++ sample. - WinRT: change Timestamp from Windows.Foundation.DateTime to Int64 so the projection mirrors the COM type and matches the long timestamp variable already used in the .NET sample. - Update the doc comment to say 'milliseconds since the UNIX epoch' instead of 'seconds since the UNIX epoch'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| ## .NET and WinRT | ||
|
|
||
| ```c# | ||
| namespace Microsoft.Web.WebView2.Core |
There was a problem hiding this comment.
Possible candidate for .Diagnostics sub-namespace, if we think these are "Advanced" / rare APIs for WebView2.Core consumers? Or "no", because the type is returned by a method on CoreWebView2Environment?
There was a problem hiding this comment.
Or "no", because the type is returned by a method on CoreWebView2Environment?
It won't be a different namespace because its on the CoreWebView2Environment.
| /// pairs beyond those listed above. Consumers | ||
| /// should ignore unknown keys. | ||
| NetworkRequest = 0, | ||
| }; |
There was a problem hiding this comment.
enums with only a single value are rare. What is a plausible second value for this, and how soon do we expect one to ship?
| WebView = 0, | ||
|
|
||
| /// Signal from a profile. | ||
| Profile = 1, |
There was a problem hiding this comment.
Is the expectation that when Scope is WebView or Profile, the json contains information to tell which one from the Environment is relevant? I don't see this in the NetworkRequest schema documentation.
| L"{}")); | ||
|
|
||
| // Subscribe to the diagnostic event. | ||
| CHECK_FAILURE(m_monitor->add_DiagnosticReceived( |
There was a problem hiding this comment.
Could this sample miss an event, since we set the filter before subscribing?
| ## Filter with field-level JSON criteria | ||
|
|
||
| You can pass a JSON object to `SetDiagnosticFilter` to | ||
| restrict which events are delivered. An empty JSON object `"{}"` receives |
There was a problem hiding this comment.
The cpp definition comment below says it accepts "" as well as "{}" to represent an empty json object. Is that also true in .Net/WinRT ? Or does empty string throw there?
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.