API Review: CoreWebView2Frame.FrameCreated #4982

JosephJin0815 · 2024-12-10T19:11:40Z

This is a review for the new CoreWebView2Frame.FrameCreated API.

Add spec for NestedFrame.md

oldnewthing · 2024-12-12T17:12:39Z

specs/NestedFrame.md

+for the nested iframe.
+
+To prevent unnecessary performance implication, WebView2 does
+not track any nested iframes by default. It only tracks a nested


This seems to violate Windows Runtime guidelines, because it means that you don't get a CoreWebView2.FrameNavigationStarting unless you subscribe to the FrameCreated event (even if the event handler does nothing).

❌ DO NOT alter behavior based on whether an event handler is registered or the number of registered event handlers. In other words, registering an event handler that does nothing should have no effect on behavior.

An implementation is permitted to perform optimizations based on whether an event handler is registered. The most common case of this is bypassing an expensive operation if there is no way for the app to observe any of the operation’s side effects.

In this case, the app can observe the side effects (through the FrameNavigationStarting event).

We could have a new Boolean property on the frame ShouldRaiseNavigationEvents which defaults to true for first-level frames and false for deeper frames. (But see the componentization problem later.)

I need to clarify the documentation here. The CoreWebView2.FrameNavigationStarting event will still be triggered for all child iframes, regardless of whether we subscribe to the CoreWebView2Frame.FrameCreated event or not. In this context, we are referring to the APIs in CoreWebView2Frame. We think that event handler matters because if developers do not subscribe to it, there is no need to expose any CoreWebView2Frame APIs

Please clarify that this is an implementation note. This is only about an internal perf optimization - no observable behavior changes described here.

oldnewthing · 2024-12-12T17:17:45Z

specs/NestedFrame.md

+        }
+    }


Suggested change

}

}

};

};

Please fix.

oldnewthing · 2024-12-12T17:26:04Z

specs/NestedFrame.md

+std::map<int, std::vector<std::wstring>> m_frame_navigation_urls;
+// In this example, a WebView2 application wants to manage the 
+// navigation of third-party content residing in second-level iframes
+// (Main frame -> First-level frame -> Second-level third-party frames).


Maybe also a sample showing full depth frame tracking?

void TrackAllFrameNavigations() { webView.CoreWebView2.FrameCreated += (_, args) => { OnFrameCreated(args.Frame); }; } void OnFrameCreated(CoreWebView2Frame frame) { frame.FrameCreated += (_, e) => OnFrameCreated(e.Frame); frame.NavigationStarting += OnFrameNavigationStarting; } void OnFrameNavigationStarting(object sender, CoreWebView2NavigationStartingEventArgs e) { // as before }

Please consider adding an additional sample.

oldnewthing · 2024-12-12T17:34:15Z

specs/NestedFrame.md

+                    CHECK_FAILURE(args->get_Frame(&webviewFrame));
+                    // Track nested (second-level) webview frame.
+                    auto frame7 = webviewFrame.try_query<ICoreWebView2Frame7>();
+                    frame7->add_FrameCreated(


Need a null check on frame7 in case the try_query fails.

Please fix.

oldnewthing · 2024-12-12T17:44:06Z

specs/NestedFrame.md

+                                        Callback<ICoreWebView2FrameNavigationStartingEventHandler>(
+                                            [this](
+                                                ICoreWebView2Frame* sender,
+                                                ICoreWebView2NavigationStartingEventArgs* args) -> HRESULT {


Suggested change

ICoreWebView2NavigationStartingEventArgs* args) -> HRESULT {

ICoreWebView2NavigationStartingEventArgs* args) noexcept -> HRESULT {

Fail fast if an exception comes out of the [] or push_back.

Please fix.

oldnewthing · 2024-12-12T17:45:15Z

specs/NestedFrame.md

+### C++ Sample
+```cpp
+wil::com_ptr<ICoreWebView2> m_webview;
+std::map<int, std::vector<std::wstring>> m_frame_navigation_urls;


Suggested change

std::map<int, std::vector<std::wstring>> m_frame_navigation_urls;

std::map<UINT32, std::vector<std::wstring>> m_frame_navigation_urls;

Why not just use a UINT32 to match the type of FrameId?

Please fix.

oldnewthing · 2024-12-12T17:47:56Z

specs/NestedFrame.md

+                                                // navigation if it's on block list.
+                                                UINT32 frameId = 0;
+                                                auto frame5 = wil::com_ptr<ICoreWebView2Frame>(sender)
+                                                                .try_query<ICoreWebView2Frame5>();


Missing null check from try_query. Maybe we should use ICoreWebView2Frame5 in the outer lambda, so that we don't even get here if v5 is not supported. Then that would allow us to use query() instead of try_query().

Please consider simple fix of null check for simple sample code.

oldnewthing · 2024-12-12T17:51:08Z

specs/NestedFrame.md

+                                                // navigation if it's on block list.
+                                                UINT32 frameId = 0;
+                                                auto frame5 = wil::com_ptr<ICoreWebView2Frame>(sender)
+                                                                .try_query<ICoreWebView2Frame5>();


Can be simplified to

auto frame5 = wil::try_com_query<ICoreWebView2Frame5>(sender);

avoids an unnecessary temporary com_ptr.

Please fix.

oldnewthing · 2024-12-12T17:58:09Z

specs/NestedFrame.md

+events will be raised from the top-level iframe. With the support
+of tracking nested iframes, we can now handle these requests directly
+within the nested iframe. Specifically, these requests are raised to
+the nearest tracked frame, which is the `CoreWebView2Frame` closest


This could lead to a problem if an app combines multiple components, and one of them adds a FrameCreated handler and accidentally messes up the other component which expected the event from the top-level frame. Is the answer "Yeah, you can't really componentize your app like that because features used by one app might have adverse consequences far away." So it means I can't have a "app usage telemetry" component that hooks all frame activity and logs it, because that would accidentally break my "permission manager" component.

From the Windows Runtime design guidelines:

A Well-Designed API Avoids Shared Behavior

A developer is often unpleasantly surprised when he or she uses one API that causes a side-effect elsewhere.

☑ DO design APIs to allow multiple components running in the same process to use the API without having to explicitly coordinate, for example, two plug-ins loaded into the same instance of Internet Explorer

For PermissionRequested API, there are two factors. One is the presence of the FrameCreated handler, and the other is the need to handle the PermissionRequested API. Simply subscribing to the FrameCreated handler does not affect where the PermissionRequested event is actually handled.
Suppose there is a permission request from frame D. Developers want to handle the PermissionRequested event from the top-level iframe B in component I. Meanwhile, in component II, developers have an "app usage telemetry" that hooks all frame activity. Unless the developers subscribe to the permission request and Handled in either frame D or frame C, otherwise the PermissionRequested event will still bubble up to the top-level iframe and handle in component I.

// Example: // A (main frame/CoreWebView2) // | // B (first-level iframe/CoreWebView2Frame) // | // C (nested iframe) // | // D (nested iframe)

But I think this kind of shared behavior should be avoid by developers. Even if we, align with the browser, track all iframes by default and the PermissionRequested event starts to fire on the iframe where it originates, there could be shared behavior issue. If developers handle the PermissionRequested event in a nested iframe within one component, then another component won't have the opportunity to handle the PermissionRequested event at the top-level iframe.

No issue after discussion: event bubbles out in existing event bubbling pattern.

david-risney

Please update according to comments, thanks!

david-risney · 2024-12-12T21:06:37Z

specs/NestedFrame.md

+for the nested iframe.
+
+To prevent unnecessary performance implication, WebView2 does
+not track any nested iframes by default. It only tracks a nested


Please clarify that this is an implementation note. This is only about an internal perf optimization - no observable behavior changes described here.

david-risney · 2024-12-12T21:07:45Z

specs/NestedFrame.md

+### C++ Sample
+```cpp
+wil::com_ptr<ICoreWebView2> m_webview;
+std::map<int, std::vector<std::wstring>> m_frame_navigation_urls;


Please fix.

david-risney · 2024-12-12T21:09:08Z

specs/NestedFrame.md

+std::map<int, std::vector<std::wstring>> m_frame_navigation_urls;
+// In this example, a WebView2 application wants to manage the 
+// navigation of third-party content residing in second-level iframes
+// (Main frame -> First-level frame -> Second-level third-party frames).


Please consider adding an additional sample.

david-risney · 2024-12-12T21:09:53Z

specs/NestedFrame.md

+                    CHECK_FAILURE(args->get_Frame(&webviewFrame));
+                    // Track nested (second-level) webview frame.
+                    auto frame7 = webviewFrame.try_query<ICoreWebView2Frame7>();
+                    frame7->add_FrameCreated(


Please fix.

david-risney · 2024-12-12T21:10:25Z

specs/NestedFrame.md

+                                        Callback<ICoreWebView2FrameNavigationStartingEventHandler>(
+                                            [this](
+                                                ICoreWebView2Frame* sender,
+                                                ICoreWebView2NavigationStartingEventArgs* args) -> HRESULT {


Please fix.

david-risney · 2024-12-12T21:10:35Z

specs/NestedFrame.md

+                                                // navigation if it's on block list.
+                                                UINT32 frameId = 0;
+                                                auto frame5 = wil::com_ptr<ICoreWebView2Frame>(sender)
+                                                                .try_query<ICoreWebView2Frame5>();


Please consider simple fix of null check for simple sample code.

david-risney · 2024-12-12T21:11:41Z

specs/NestedFrame.md

+                                                // navigation if it's on block list.
+                                                UINT32 frameId = 0;
+                                                auto frame5 = wil::com_ptr<ICoreWebView2Frame>(sender)
+                                                                .try_query<ICoreWebView2Frame5>();


Please fix.

david-risney · 2024-12-12T21:12:01Z

specs/NestedFrame.md

+        }
+    }


Please fix.

david-risney · 2024-12-12T21:15:16Z

specs/NestedFrame.md

+events will be raised from the top-level iframe. With the support
+of tracking nested iframes, we can now handle these requests directly
+within the nested iframe. Specifically, these requests are raised to
+the nearest tracked frame, which is the `CoreWebView2Frame` closest


No issue after discussion: event bubbles out in existing event bubbling pattern.

Wangsong Jin and others added 8 commits November 26, 2024 14:35

Added Spec for CoreWebView2Frame::FrameCreated API

132f3f6

Fixed typo

cc43133

fixed format and nits

44eedbe

format and rephase

8d67f77

refined the sample code and added impact APIs description

6e42f58

fixed nit

1fd53bc

added more description for impacted API

04b1b94

Merge pull request #4950 from MicrosoftEdge/NestedFrame-draft

2e237f6

Add spec for NestedFrame.md

JosephJin0815 added the API Proposal Review WebView2 API Proposal for review. label Dec 10, 2024

JosephJin0815 requested a review from david-risney December 10, 2024 19:11

david-risney requested review from aluhrs13, vickiez and shrinaths December 10, 2024 19:14

oldnewthing reviewed Dec 12, 2024

View reviewed changes

david-risney requested changes Dec 12, 2024

View reviewed changes

JosephJin0815 mentioned this pull request Dec 13, 2024

API Review: CoreWebView2Frame.FrameCreated MicrosoftEdge/WebView2Announcements#105

Open

victorhuangwq mentioned this pull request Dec 13, 2024

FrameCreated is not called for iframe inside another iframe #2440

Open

JosephJin0815 force-pushed the NestedFrame branch from ecb66d5 to 7e5018c Compare December 16, 2024 21:55

addressed comments from API review meeting

4d00028

JosephJin0815 force-pushed the NestedFrame branch from 7e5018c to 4d00028 Compare December 16, 2024 22:05

david-risney approved these changes Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Review: CoreWebView2Frame.FrameCreated #4982

API Review: CoreWebView2Frame.FrameCreated #4982

JosephJin0815 commented Dec 10, 2024

oldnewthing Dec 12, 2024

JosephJin0815 Dec 12, 2024

david-risney Dec 12, 2024

oldnewthing Dec 12, 2024

david-risney Dec 12, 2024

oldnewthing Dec 12, 2024

david-risney Dec 12, 2024

oldnewthing Dec 12, 2024

david-risney Dec 12, 2024

oldnewthing Dec 12, 2024

david-risney Dec 12, 2024

oldnewthing Dec 12, 2024

david-risney Dec 12, 2024

oldnewthing Dec 12, 2024

david-risney Dec 12, 2024

oldnewthing Dec 12, 2024

david-risney Dec 12, 2024

oldnewthing Dec 12, 2024

JosephJin0815 Dec 12, 2024

david-risney Dec 12, 2024

david-risney left a comment

david-risney Dec 12, 2024

david-risney Dec 12, 2024

david-risney Dec 12, 2024

david-risney Dec 12, 2024

david-risney Dec 12, 2024

david-risney Dec 12, 2024

david-risney Dec 12, 2024

david-risney Dec 12, 2024

david-risney Dec 12, 2024

-        }
-    }
+        };
+    };

	ICoreWebView2NavigationStartingEventArgs* args) -> HRESULT {
	ICoreWebView2NavigationStartingEventArgs* args) noexcept -> HRESULT {

	std::map<int, std::vector<std::wstring>> m_frame_navigation_urls;
	std::map<UINT32, std::vector<std::wstring>> m_frame_navigation_urls;

API Review: CoreWebView2Frame.FrameCreated #4982

Are you sure you want to change the base?

API Review: CoreWebView2Frame.FrameCreated #4982

Conversation

JosephJin0815 commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A Well-Designed API Avoids Shared Behavior

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-risney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment