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
| [propget] HRESULT Timestamp( | ||
| [out, retval] INT64* value); | ||
|
|
||
| /// Returns category-specific diagnostic data as a JSON |
There was a problem hiding this comment.
We prefer giving data an API versus representing it with JSON or some other format inside a string. Strings are a weak contract that skips our API tooling for compat validation and documentation, isn't discoverable via intellisense, gives the end devs more work to actually get a property value out.
If this was only for telemetry and the details string varied dramatically with no set form - like devtools console.log messages - then it would make sense to have it as a string. But in this case so far there's one kind of networking event that has a well-defined shape for a bunch of different kinds of errors.
Can we add a Details object that varies type based on the kind of error? Eg CoreWebView2DiagnosticReceivedEventArgs.Details property that for networking errors is a CoreWebView2DiagnosticNetworkErrorDetails class. Or if the networking error only corresponds to an existing event - use that existing event as the details object. And remove the SetDiagnosticFilter and instead allow the end dev to implement their own filtering using the details object.
There was a problem hiding this comment.
Updated description to reflect intent of the API. The real intent of this api is for host apps to collect additional information in rare, hard to debug, tricky conditions and relay the information back to us. In that sense it is like a console logging api...
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>
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.