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.

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

Webview2 initialization #14671

Merged
merged 17 commits into from
Dec 6, 2023
Merged

Webview2 initialization #14671

merged 17 commits into from
Dec 6, 2023

Conversation

pinzart90
Copy link
Contributor

@pinzart90 pinzart90 commented Dec 1, 2023

Part of the many issues during testing (and not only):

  1. WebView2 controls will block EnsureCoreWebView2Async until the control is visible.
    Pretty good description here.
    I tried to move most of the webview2 initialization calls in the Loaded callback (basically after the webview2 control is added to the visual layout). DocumentationBrowser's webview2 has not been refactored in such a way because it is used in slightly more complicated workflows.
  2. Ensured Dispose is properly called on the webview2 controls.
  3. Ensured that DocumentationBrowser's webview2 is initialized only from the UI thread.

An example of buggy behavior:

  • Open Dynamo.
  • Dynamo will create the ViewExtensions.
  • Some view extensions will call asyncInitialize for their webview2 controls, even though the webview2 controls are still hidden (not part of the visual tree)
  • Calls to EnsureCoreWebView2Async will be blocked, waiting for webview2 to become visible.
  • Close Dynamo, which will call webview2.Dispose().

This might cause crashes, memory leaks etc.

this.documentationBrowser.DpiChanged -= DocumentationBrowser_DpiChanged;

if (this.documentationBrowser.CoreWebView2 != null)
if (this.documentationBrowser != null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

code here was just moved under a null check

@@ -422,6 +414,8 @@ private void Browser_Loaded(object sender, RoutedEventArgs e)
{
string msg = "Browser Loaded";
LogToDynamoConsole(msg);

InitializeAsync();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We initialize webview2 only after it is loaded as part of the visual tree (Browser_Loaded)

@@ -48,7 +48,7 @@ public void UpdateNotificationWindowSize(int height)
}
}

public class NotificationCenterController
public class NotificationCenterController : IDisposable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

proper disposable

@pinzart90 pinzart90 marked this pull request as ready for review December 4, 2023 14:07
@pinzart90 pinzart90 mentioned this pull request Dec 4, 2023
@pinzart90 pinzart90 requested review from sm6srw and dnenov December 4, 2023 18:23
* update

* update

* Update DocumentationBrowserView.xaml.cs

* update

* Update DocumentationBrowserView.xaml.cs

* update

* update

---------

Co-authored-by: pinzart <[email protected]>
@pinzart90
Copy link
Contributor Author

OK, waiting for the final checks then I will merge
Any final reviews @mjkkirschner @aparajit-pratap @sm6srw ?

@@ -162,6 +165,9 @@ public void ViewExtensionIgnoresExternalEvents()
// Act
var tabsBeforeExternalEventTrigger = this.ViewModel.SideBarTabItems.Count;
viewExtension.HandleRequestOpenDocumentationLink(externalEvent);
DispatcherUtil.DoEventsLoop();

Assert.AreEqual(AsyncMethodState.NotStarted, viewExtension.BrowserView.initState);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it not started here? Won't webview2 initialization be complete by this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using an externalLink will not startup webview2.
That is what the test is verifying

@pinzart90 pinzart90 merged commit c599f9f into master Dec 6, 2023
29 checks passed
@pinzart90 pinzart90 deleted the webview2_initialization branch December 6, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants