Repository navigation
Call host to launch Chrome unelevated if host sends supportsLaunchUnelevatedProcessRequest flag. - #676
Conversation
Chance An (changsi-an)
changed the title
|
|
||
| private async spawnChromeUnelevatedWithPineZorro(chromePath: string, chromeArgs: string[]): Promise<number> { | ||
| return new Promise<number>((resolve, reject) => { | ||
| this._session.sendRequest("launchUnelevated", { |
There was a problem hiding this comment.
We are wrapping sendRequest in a promise in a similar way here: https://github.com/Microsoft/vscode-chrome-debug-core/blob/47782a37b6df5efd3a50bf1813019c0068a984fe/src/transformers/fallbackToClientPathTransformer.ts#L36
Would it make sense to extract the async sendRequest logic someplace else?
There was a problem hiding this comment.
digeff The two usages are not quite the same, they are very different in the parameters they each have respectively. Having a common utility function will carry the different parameters as well, it almost like repeating this code. So extracting a common logic does not improve much, but reduces readability.
roblourens if the operation does not succeed it will reject, and the call will get an exception thrown then telemetry handler will capture that. Otherwise, there is C# telemetry implemented in the host for each PZ extension request.
There was a problem hiding this comment.
Chance An (@changsi-an) would we know from the telemetry that the error happened because of Chrome launch failure when running as Administrator?
There was a problem hiding this comment.
Dhanvi Kapila (@dhanvikapila) Yes, the admin flag is right on the same DA event (clientRequest/launch). The C# error is on a separate event.
| logger.log(`Parsed output file and got Chrome PID ${pidStr}`); | ||
| this._chromePID = parseInt(pidStr, 10); | ||
| if (this._doesHostSupportLaunchUnelevatedProcessRequest) { | ||
| chromePid = await this.spawnChromeUnelevatedWithPineZorro(chromePath, chromeArgs); |
There was a problem hiding this comment.
Would this be equivalent to use this._chromePID here instead?
There was a problem hiding this comment.
Fixed.
| 'meteor://💻app/*': '${webRoot}/*' | ||
| }; | ||
|
|
||
| interface IExtendedInitializeRequestArgumetns extends DebugProtocol.InitializeRequestArguments{ |
There was a problem hiding this comment.
Fixed.
| return null | ||
| } | ||
|
|
||
| private async spawnChromeUnelevatedWithPineZorro(chromePath: string, chromeArgs: string[]): Promise<number> { |
There was a problem hiding this comment.
Maybe don't name this specifically for pinezorro, just "WithClient"
There was a problem hiding this comment.
Fixed.
|
|
||
| private async spawnChromeUnelevatedWithPineZorro(chromePath: string, chromeArgs: string[]): Promise<number> { | ||
| return new Promise<number>((resolve, reject) => { | ||
| this._session.sendRequest("launchUnelevated", { |
host sends supportsLaunchUnelevatedProcessRequest flag.
f8e89bb to
fec31d5
Compare
|
PTAL |
|
Can we add tests for this change? |
|
Chance An (@changsi-an) LMK when this is ready to merge. I'm ok with or without tests. |
|
roblourens Please help merge it now. I will submit a PR for unit test later. |
5 participants
host sends supportsLaunchUnelevatedProcessRequest flag.
Now PZ host is capable of supporting a
launchUnelevatedrequest from DA. If supportsLaunchUnelevatedProcessRequest flag is set throughinitializerequest, DA will choose to use host's ability to launch Chrome unelevated (when needed).spawnChromeUnelevatedWithWindowsScriptHost()is extracted old code.spawnChromeUnelevatedWithPineZorro()is the new logic to ask host to launch Chrome unelevated.