API Review: Diagnostic Monitor API

[chetanpandey1266]

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.

[shrinaths]

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]

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();
    }
}

[shrinaths]

see inline

[shrinaths]

see inline

[shrinaths]

see inline

[shrinaths]

see inline

[shrinaths]

see inline

[shrinaths]

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.

[chetanpandey1266]

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.

[david-risney]

Is chromium net error code already defined somewhere? If so link to that documentation. If not, you need to fully document the values this can be and what they mean.

[david-risney]

'scheme' may be a more accurate name if this is the URI's scheme name and not necessarily a network protocol

[david-risney]

We need more information about exactly what is being reported and when. If this corresponds with existing webview2 events you can describe it in terms of the existing events. Including when it happens in relation to the existing events (before, after, either)

If this doesn't correspond with an existing webview2 event, you need to spell out exactly what network requests this happens for. Like: is it only navigation network requests, or including sub-resource network requests, and fetch, or additionally things like service worker that's not associated with one webview2, or csp error reports that aren't under direct control/observation of a document?

And you say 'Network request failure including ...' which makes it sound like this doesn't fully describe the set of possible failures. If the full set of possible failures is defined by one of the properties that is fully defined you can say that. Otherwise you need to define that.

[david-risney]

Why is this a Get method rather than a property?

[david-risney]

More elaboration about IClosable required here. What resources are held open and require the use of IClosable? And how does it work for COM - I don't see anything in the COM IDL for this (and I don't recall how that works - do we need something for COM?)

[david-risney]

Elsewhere we treat WebView as one word for this sort of thing eg _SCOPE_WEBVIEW (like at the front with COREWEBVIEW2_...)

[david-risney]

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.

[david-risney]

Can we make this an actual wall-clock time? Presumably the end dev will want to collect these diagnostic events together with diagnostic events from other sources and want to compare the timestamps.

[david-risney]

We only have one category and it is populated. We shouldn't have any categories documented or used that aren't populated to avoid confusion and compat issues.