Skip to content

Commit

Permalink
Make resolveExternalUri return just a plain uri instead of a disposab…
Browse files Browse the repository at this point in the history
…le result

For #81131

We had trouble coming up with cases where extensions could actually reliably dispose of the resolved uri
  • Loading branch information
mjbvz committed Sep 23, 2019
1 parent 00983bc commit a62b7da
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 39 deletions.
11 changes: 6 additions & 5 deletions src/vs/editor/browser/services/openerService.ts
Expand Up @@ -58,15 +58,18 @@ export class OpenerService extends Disposable implements IOpenerService {
}
}

const { resolved } = await this.resolveExternalUri(resource, options);

// check with contributed openers
for (const opener of this._openers.toArray()) {
const handled = await opener.open(resource, options);
const handled = await opener.open(resolved, options);
if (handled) {
return true;
}
}

// use default openers
return this._doOpen(resource, options);
return this._doOpen(resolved, options);
}

public async resolveExternalUri(resource: URI, options?: { readonly allowTunneling?: boolean }): Promise<{ resolved: URI, dispose(): void }> {
Expand Down Expand Up @@ -135,9 +138,7 @@ export class OpenerService extends Disposable implements IOpenerService {
}

private async _doOpenExternal(resource: URI, options: OpenOptions | undefined): Promise<boolean> {
const { resolved } = await this.resolveExternalUri(resource, options);
dom.windowOpenNoOpener(encodeURI(resolved.toString(true)));

dom.windowOpenNoOpener(encodeURI(resource.toString(true)));
return Promise.resolve(true);
}

Expand Down
8 changes: 3 additions & 5 deletions src/vs/vscode.proposed.d.ts
Expand Up @@ -1147,18 +1147,16 @@ declare module 'vscode' {
* Resolves an *external* uri, such as a `http:` or `https:` link, from where the extension is running to a
* uri to the same resource on the client machine.
*
* This is a no-oop if the extension is running locally. Currently only supports `https:` and `http:`.
* This is a no-op if the extension is running locally. Currently only supports `https:` and `http:`.
*
* If the extension is running remotely, this function automatically establishes port forwarding from
* the local machine to `target` on the remote and returns a local uri that can be used to for this connection.
*
* Note that uris passed through `openExternal` are automatically resolved.
*
* @return A uri that can be used on the client machine. Extensions should dispose of the returned value when
* both the extension and the user are no longer using the value. For port forwarded uris, `dispose` will
* close the connection.
* @return A uri that can be used on the client machine.
*/
export function resolveExternalUri(target: Uri): Thenable<{ resolved: Uri, dispose(): void }>;
export function resolveExternalUri(target: Uri): Thenable<Uri>;
}

//#endregion
Expand Down
20 changes: 2 additions & 18 deletions src/vs/workbench/api/browser/mainThreadWindow.ts
Expand Up @@ -14,8 +14,6 @@ import { ExtHostContext, ExtHostWindowShape, IExtHostContext, IOpenUriOptions, M
@extHostNamedCustomer(MainContext.MainThreadWindow)
export class MainThreadWindow implements MainThreadWindowShape {

private handlePool = 1;

private readonly proxy: ExtHostWindowShape;
private readonly disposables = new DisposableStore();
private readonly resolved = new Map<number, IDisposable>();
Expand Down Expand Up @@ -49,23 +47,9 @@ export class MainThreadWindow implements MainThreadWindowShape {
return this.openerService.open(uri, { openExternal: true, allowTunneling: options.allowTunneling });
}

async $resolveExternalUri(uriComponents: UriComponents, options: IOpenUriOptions): Promise<{ handle: number, result: UriComponents }> {
async $resolveExternalUri(uriComponents: UriComponents, options: IOpenUriOptions): Promise<UriComponents> {
const uri = URI.revive(uriComponents);
const handle = ++this.handlePool;

const result = await this.openerService.resolveExternalUri(uri, options);
this.resolved.set(handle, result);

return { handle, result: result.resolved };
}

async $releaseResolvedExternalUri(handle: number): Promise<boolean> {
const entry = this.resolved.get(handle);
if (entry) {
entry.dispose();
this.resolved.delete(handle);
return true;
}
return false;
return result.resolved;
}
}
3 changes: 1 addition & 2 deletions src/vs/workbench/api/common/extHost.protocol.ts
Expand Up @@ -744,8 +744,7 @@ export interface IOpenUriOptions {
export interface MainThreadWindowShape extends IDisposable {
$getWindowVisibility(): Promise<boolean>;
$openUri(uri: UriComponents, options: IOpenUriOptions): Promise<boolean>;
$resolveExternalUri(uri: UriComponents, options: IOpenUriOptions): Promise<{ readonly handle: number, readonly result: UriComponents }>;
$releaseResolvedExternalUri(handle: number): Promise<boolean>;
$resolveExternalUri(uri: UriComponents, options: IOpenUriOptions): Promise<UriComponents>;
}

// -- extension host
Expand Down
12 changes: 3 additions & 9 deletions src/vs/workbench/api/common/extHostWindow.ts
Expand Up @@ -9,7 +9,6 @@ import { WindowState } from 'vscode';
import { URI } from 'vs/base/common/uri';
import { Schemas } from 'vs/base/common/network';
import { isFalsyOrWhitespace } from 'vs/base/common/strings';
import { once } from 'vs/base/common/functional';

export class ExtHostWindow implements ExtHostWindowShape {

Expand Down Expand Up @@ -55,19 +54,14 @@ export class ExtHostWindow implements ExtHostWindowShape {
return this._proxy.$openUri(stringOrUri, options);
}

async resolveExternalUri(uri: URI, options: IOpenUriOptions): Promise<{ resolved: URI, dispose(): void }> {
async resolveExternalUri(uri: URI, options: IOpenUriOptions): Promise<URI> {
if (isFalsyOrWhitespace(uri.scheme)) {
return Promise.reject('Invalid scheme - cannot be empty');
} else if (!new Set([Schemas.http, Schemas.https]).has(uri.scheme)) {
return Promise.reject(`Invalid scheme '${uri.scheme}'`);
}

const { result, handle } = await this._proxy.$resolveExternalUri(uri, options);
return {
resolved: URI.from(result),
dispose: once(() => {
this._proxy.$releaseResolvedExternalUri(handle);
}),
};
const result = await this._proxy.$resolveExternalUri(uri, options);
return URI.from(result);
}
}

0 comments on commit a62b7da

Please sign in to comment.