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

Move off in Webview #206363

Merged
merged 18 commits into from Mar 12, 2024
Merged

Move off in Webview #206363

merged 18 commits into from Mar 12, 2024

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Feb 27, 2024

I've been working on refactoring the webview related code by dropping all $window references. This task turned out to be quite complex, as it involves a deep understanding of the system to decide how to correctly associate the code windows.

Here are the steps I've taken so far:

  • I've made it a requirement to specify the codeWindow when initializing webviews. This is because the webview needs to be aware of the codeWindow from the start, as it listens to messages and events from both window and document ref
  • Consequently, I've updated the WebviewElement to use CodeWindow. ref

Next steps are revising the use of webviews across the workbench. For areas where the codeWindow is clearly identified, like in editors (where we have group.windowId), it's straightforward to proceed:

  • Notebook. This is straight foward as when creating a notebook, we already know the containing CodeWindow
  • Notebook Diff. Same as it's an Editor so I can reading from group.windowId
  • Extension Editor. Same as above.
  • CodeInset. CodeInset is always insides a monaco editor, so we use the same CodeWindow containing the editor.
  • CustomEditor

For cases where the code window isn't explicitly known, especially for requests initiated from the extension host, I used getActiveWindow, but I'm uncertain about its reliability:

@bpasero @mjbvz

@rebornix rebornix self-assigned this Feb 27, 2024
Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! Since the WebviewElement is mounted into the Dom, I wonder if we can use this to get the code window in some cases instead of passing it in

That still leaves the call to parentOriginHash(this.codeWindow.origin, ...) (and maybe others I'm overlooking)

Does the origin change for detached editors? If not, maybe we can just the origin of the main window in this case?

src/vs/workbench/contrib/webview/browser/webviewElement.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/webview/browser/webviewElement.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/webview/browser/webview.ts Outdated Show resolved Hide resolved
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Cool stuff, one thing that worries me is the serialise/deserialise for webviews, I have asked some questions in the code.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Pushed some changes, I will make sure that this.group is defined early on, so that we can continue to use it for getting the window.

the-coder-o

This comment was marked as spam.

@bpasero bpasero added this to the March 2024 milestone Feb 29, 2024
@bpasero bpasero requested a review from mjbvz March 11, 2024 16:12
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

🚀

@rebornix rebornix marked this pull request as ready for review March 12, 2024 15:18
@rebornix rebornix merged commit 19de9d2 into main Mar 12, 2024
6 checks passed
@rebornix rebornix deleted the rebornix/ltd-platypus branch March 12, 2024 15:18
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.

None yet

4 participants