Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Review: Web Notification #3186

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

API Review: Web Notification #3186

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1c1d3d0
Add Web Notification API Spec draft
peiche-jessica Dec 9, 2022
952a757
Apply suggestions from code review
peiche-jessica Dec 13, 2022
e03a214
Address initial feedback part 1
peiche-jessica Dec 13, 2022
cbd9d61
Add enum CoreWebView2TextDirectionKinds to c# API details
peiche-jessica Dec 13, 2022
5e22e5f
Report* and CloseRequested
peiche-jessica Jan 4, 2023
0b9a618
Persistent -> ProfileNotificationReceivedEventHandler
peiche-jessica Jan 6, 2023
61d7ae7
Address initial feedback part 2
peiche-jessica Jan 6, 2023
c61f653
Update ref doc
peiche-jessica Jan 6, 2023
b8395cb
Add DataAsJson and TryGetDataAsString
peiche-jessica Jan 9, 2023
88418d0
Add links to Notification.actions
peiche-jessica Jan 9, 2023
0b6a3aa
Apply API name suggestion
peiche-jessica Jan 10, 2023
25207b4
Clarify Handled and ReportXXX methods
peiche-jessica Jan 10, 2023
8ef88d3
Clarify deferral info
peiche-jessica Jan 10, 2023
4908aaf
Fix sample code
peiche-jessica Jan 10, 2023
993fe9f
Make ReportClicked and ReportClosed async
peiche-jessica Jan 10, 2023
320baf0
Address feedback
peiche-jessica Feb 1, 2023
16ba2bc
Update sample
peiche-jessica Feb 3, 2023
4493c00
Merge pull request #3049 from MicrosoftEdge/api-notification-draft
peiche-jessica Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Clarify deferral info
  • Loading branch information
@peiche-jessica
peiche-jessica committed Jan 10, 2023
commit 8ef88d31c5b92e7b787c6f0dd7250ded2d25b4ef
8 changes: 8 additions & 0 deletions specs/WebNotification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,10 @@ typedef enum COREWEBVIEW2_TEXT_DIRECTION_KINDS {
interface ICoreWebView2Profile3 : IUnknown {
/// Add an event handler for the `NotificationReceived` event for persistent
/// notifications.
///
/// If a deferral is not taken on the event args, the subsequent scripts are
/// blocked until the event handler returns. If a deferral is taken, the
/// scripts are blocked until the deferral is completed.
HRESULT add_NotificationReceived(
Copy link
Contributor

Choose a reason for hiding this comment

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

No sample for this or discussion about why no sample was needed (e.g. because it's identical to the CoreWebView2 version aside from where you register the handler).

Would an app want to treat persistent and non-persistent notifications differently?

Or is the reason that you subscribe to CoreWebView2.NotificationReceived to handle notifications raised by that specific WebView2, and you use CoreWebView2Profile.NotificationReceived for notifications not associated with any WebView2?

Do persistent notifications outlive the WebView2? (Why are they called "persistent"?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please add a third sample for this as we described elsewhere.

Please ensure we document the scope of persistent and non-persistent with respect to WebView2 (environment, profile, browser process, webview2)

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between a profile notification and a webview notification?

Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment to explain a bit more and link to DOM documentation.

[in] ICoreWebView2ProfileNotificationReceivedEventHandler* eventHandler,
[out] EventRegistrationToken* token);
Expand All @@ -297,6 +301,10 @@ interface ICoreWebView2Profile3 : IUnknown {
interface ICoreWebView2_17 : ICoreWebView2_16 {
/// Add an event handler for the `NotificationReceived` event for
/// non-persistent notifications.
///
/// If a deferral is not taken on the event args, the subsequent scripts are
/// blocked until the event handler returns. If a deferral is taken, the
/// scripts are blocked until the deferral is completed.
HRESULT add_NotificationReceived(
[in] ICoreWebView2NotificationReceivedEventHandler* eventHandler,
[out] EventRegistrationToken* token);
Expand Down