UI element selection debt

[bpasero]

Some feedback from #246643

• I find that the complexity of the methods justifies putting them into their own respective service. Native host service is already a bit of a kitchen sink of sorts and was never meant to contain all methods for all use cases. But I feel that getElementData specifically is getting large enough to think about having it in its own service. Specifically because it seems to depend on the simple browser, which is some hardcoded knowledge that does not seem right for the native host.

• the use of cancellation is not very robust, I would generate some kind of UUID or increasing integer and register a channel with that UUID specifically to cancel that one invocation of the use of the method. we use a similar technique to control native context menus here

• As with the code, there seems to be leaks, specifically the use of this._register from getNodeData is odd because you will add disposables to the global map each time the method is called 🤔

vscode/src/vs/platform/native/electron-main/nativeHostMainService.ts

Line 931 in f8b7b73

this._register(debuggers.off('message', onMessage));

vscode/src/vs/platform/native/electron-main/nativeHostMainService.ts

Line 982 in f8b7b73

this._register(debuggers.off('message', onMessage));

vscode/src/vs/platform/native/electron-main/nativeHostMainService.ts

Line 989 in f8b7b73

this._register(debuggers.on('message', onMessage));

• Finally, the listener to window.webContents.on('ipc-message', async (event, channel) => { is never removed:

vscode/src/vs/platform/native/electron-main/nativeHostMainService.ts

Line 980 in f8b7b73

window.webContents.on('ipc-message', async (event, channel) => {

[justschen]

gotcha! thanks for the feedback!

I'll address the listener issues this iteration, and yeah this whole chrome debugger tools thing is super complicated, and agree with the new service.

the native host main service was just the easiest place to put it at the moment. maybe naming something along the lines of simpleBrowserDebuggerService or something of the sort 🏎

[bpasero]

👍 , I added another point about the way how you support cancellation.