Skip to content

API Review: Diagnostic Monitor API#5565

Open
chetanpandey1266 wants to merge 3 commits intouser/chetanpandey/DiagnosticMonitorAPIfrom
user/chetanpandey/DiagnosticMonitorAPI-draft
Open

API Review: Diagnostic Monitor API#5565
chetanpandey1266 wants to merge 3 commits intouser/chetanpandey/DiagnosticMonitorAPIfrom
user/chetanpandey/DiagnosticMonitorAPI-draft

Conversation

@chetanpandey1266
Copy link
Copy Markdown
Contributor

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.

@chetanpandey1266 chetanpandey1266 added the API Proposal Review WebView2 API Proposal for review. label Apr 15, 2026
Copy link
Copy Markdown

@shrinaths shrinaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md Outdated
Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md Outdated
Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md Outdated
Comment thread specs/DiagnosticMonitor.md Outdated
Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md
Copy link
Copy Markdown

@shrinaths shrinaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread specs/DiagnosticMonitor.md
Copy link
Copy Markdown

@shrinaths shrinaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline

Comment thread specs/DiagnosticMonitor.md Outdated
Copy link
Copy Markdown

@shrinaths shrinaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline

Comment thread specs/DiagnosticMonitor.md Outdated
Copy link
Copy Markdown

@shrinaths shrinaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline

Comment thread specs/DiagnosticMonitor.md
Copy link
Copy Markdown

@shrinaths shrinaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline

Comment thread specs/DiagnosticMonitor.md Outdated
Copy link
Copy Markdown

@shrinaths shrinaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline

Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md Outdated
Comment thread specs/DiagnosticMonitor.md Outdated
/// `protocol` is the protocol scheme (e.g. "https").
/// `uri` is the request URI.
///
/// For categories the runtime does not yet populate,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@chetanpandey1266 chetanpandey1266 Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md
Comment thread specs/DiagnosticMonitor.md
Chetan Pandey and others added 2 commits April 27, 2026 14:16
…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>
@chetanpandey1266 chetanpandey1266 force-pushed the user/chetanpandey/DiagnosticMonitorAPI-draft branch from c4ce409 to 0894cc0 Compare April 27, 2026 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API Proposal Review WebView2 API Proposal for review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants