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 webview's use of setIgnoreMenuShortcuts to main process #98131

Merged
merged 4 commits into from
May 27, 2020

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented May 19, 2020

Part of #95955

Switches from calling setIgnoreMenuShortcuts in our renderers processes to instead call it in the main process

@mjbvz mjbvz added this to the May 2020 milestone May 19, 2020
@mjbvz mjbvz requested review from bpasero and deepak1556 May 19, 2020 05:37
@mjbvz mjbvz self-assigned this May 19, 2020
ipc.on('vscode:webview.setIgnoreMenuShortcuts', (_event: IpcMainEvent, webContentsId: number, enabled: boolean) => {
const contents = webContents.fromId(webContentsId);
if (!contents) {
throw new Error(`Invalid webContentsId: ${webContentsId}`);
Copy link
Member

Choose a reason for hiding this comment

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

Is this helpful? Should it not use the log service instead?

@bpasero bpasero self-requested a review May 19, 2020 07:30
@@ -296,6 +296,16 @@ export class CodeApplication extends Disposable {

ipc.on('vscode:reloadWindow', (event: IpcMainEvent) => event.sender.reload());

ipc.on('vscode:webview.setIgnoreMenuShortcuts', (_event: IpcMainEvent, webContentsId: number, enabled: boolean) => {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using raw IPC I would prefer to go through IElectronService and friends if possible. We need to move off the direct IPC usage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Just pushed an update that moves to use channels instead. I've never worked with these directly before so let me know I've I'm using them incorrectly

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.

@mjbvz thanks, any reason you decided to introduce a new service vs. just using the existing IElectronService?

I typically prefer to call interfaces XY-Main-XY only when they are only used in electron-main. In your case I would rename it to IWebviewService. But again, I think everything can reuse the existing electron service for this purpose.

@mjbvz
Copy link
Collaborator Author

mjbvz commented May 20, 2020

@bpasero Webviews require a number of ipc calls:

  • setIgnoreMenuShortcuts (this PR)
  • Local resource protocol loading (currently also using ipc instead of a service)
  • Port forwarding (not implemented yet)

I felt it would be better to have these all in a webview specific service rather than adding them to IElectronService. I can put them in IElectronService if you feel this would be better

@bpasero
Copy link
Member

bpasero commented May 22, 2020

@mjbvz ok I wasn't aware there are more methods needed. I think it is fine to have your own service then as long as you set it up the way the electron service is setup, e.g. using the createChannelReceiver/createChannelSender helpers.

@mjbvz
Copy link
Collaborator Author

mjbvz commented May 27, 2020

@bpasero Thanks. I've aligned the service more with ElectronMainService. I'm going to merge this PR and then move the existing webview protocol handling code from using ipc to use this service instead

@mjbvz mjbvz force-pushed the move-setIgnoreMenuShortcuts-to-app branch from 465a9ef to 60a3a85 Compare May 27, 2020 19:31
@mjbvz mjbvz merged commit 5684336 into microsoft:master May 27, 2020
@bpasero
Copy link
Member

bpasero commented May 28, 2020

I suggest you look at createChannelReceiver/createChannelSender, with that you do not need to explicitly implement any methods, the helper will do everything for you. Check how IElectronService works, there is no duplication to spell out the methods or events, it is all handled automatically via Proxy.

@mjbvz
Copy link
Collaborator Author

mjbvz commented May 28, 2020

Ah ok, I'll update it to use createChannelReceiver too to cut down on the duplicated code

mjbvz added a commit that referenced this pull request May 28, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants