Use vscode-file:// instead of file://

[alexdima]

Fixes #98682

[alexdima]

@deepak1556 👍 This now looks OK from my side. I was able to eliminate the change in loader.js and hide it behind a setting preferScriptTags. I could also eliminate the need to enumerate all our nodeModules, and now there is a new loader option called amdModulesPattern that is a regex that helps distinguish which modules are AMD and which modules are nodejs. I've also adopted this last option on master, so there are no conflicts, etc. I suggest you further involve @bpasero to review, there is still some ugly string math in this PR and @jrieken for guidance on how to validate that the times are not regressing.

LMK if there's anything more needed from my side.

[deepak1556]

Thanks @alexdima

Todo:

• Fix tests
• Enable v8CacheOption
• Run perf bot against this branch

[jrieken]

Assuming this is still starting the renderer from src/vs/code/electron-browser/workbench/workbench.js then you can check these two perf marks: willLoadWorkbenchMain and didLoadWorkbenchMain. They are printed in the "F1 > Startup Performance" editor like so:

On my dev box times are ~480ms without cached data and ~280ms with cached data. Cached data files are stored in <data_dir>/CachedData/<version_hash> and for the workbench main file there should be around 9MB of cached data.

[bpasero]

Left some comments inline. On top of that, there are window.loadURL calls missing from this change:

• issue reporter
• process explorer
• auth dialog?

[bpasero]

I left some more feedback. Maybe we could do an iterative approach where this is being enabled first for:

• auth dialog
• issue reporter
• process explorer

It looks like the adoption is missing for these still.

[alexdima]

Just to clarify, I created the PR to be able to leave comments...

@deepak1556 I expect that you will address the comments :).

[deepak1556]

Sorry for the silence here, I was mostly focusing on Electron 9 last week. Gonna prioritize this PR from today.

[bpasero]

More work:

• bcfc1a9 and 81ec8ec move the logic of URI conversion fully into LocalFileAccessImpl with some new behaviour: if the file path contains a authority we take it as is instead of adding our own (I renamed app to vscode-app). This helps preserving the authority for Windows UNC paths
• 3fe9d3f adds tests

I am running another build to check if this fixes UNC support 🤞

[bpasero]

I cannot get UNC loading to work and start to think this could be an Electron issue, reported as electron/electron#25618

[bpasero]

With f5513f0, webview should work. Since preload script only supports file protocol, I am explicitly restoring the URL for that piece.

//cc @mjbvz I am not sure if there are other dependencies to file URI in webview land. The thing that will change with this PR is that require.toUrl, as well as the related method getUriFromAmdModule in amd.ts will no longer return a file:// URI but a vscode-file:// URI if called from within a renderer.

[mjbvz]

@bpasero Thanks. I think this is all that is needed for webviews. All other resources are loaded in the webviews themselves. Webviews do not allow using file: urls and instead load local resources using the vcsode-webview-resource scheme

Once we move to iframe based webviews, we no longer need to use a preload script either

[bpasero]

@mjbvz thank you for confirming 👍

[bpasero]

Given the size of this PR and stale comments, I decided to move the FileAccess work to master with a no-op implementation and the changes that we still need into #107437. The discussion should continue there.