feat: WebView2 debugging support #264

johnemau · 2020-01-27T17:36:28Z

Minimum changed needed to support msedge debugger type, it is just an alias for chrome.
This change is equivalent to the launch.json {type: 'chrome', runTimeExecutable: 'path/to/msedge.exe'}.
Minimum change needed to support backwards compatibility with WebView2 launch
debugging configurations (useWebView) from vscode-edge-debug2.

Follow-ups to consider:

Separate chrome and msedge debugger type configurations with Edge strings.
Validate useWebView configuration is configured with runTimeExecutable.
Tests for useWebView configuration.
Add browser version support (stable, beta, dev, canary) for Microsoft Edge (Chromium)
to match backwards compatibility with vscode-edge-debug2.
Deprecate {useWebView: 'advanced'} in favor of {useWebView: true, urlFilter: 'bing.com'}
Deprecate useWebView in favor of webview debugger type.

Related: #235

connor4312 · 2020-01-27T17:47:37Z

Separate chrome and edge debugger type configurations with Edge strings.

👍 -- we already have an Edge-only configuration option, there'll surely be more eventually. We'll probably want to split BrowserLauncher into a BrowserLauncherBase and then a ChromeLauncher and EdgeLauncher, like we do for the Node launcher already. Firefox is becoming CDP-compatible as well so there may eventually be one of those as well. Would be happy to have that in this PR, but it could be a followup item for anyone to implement.

Validate useWebView configuration is configured with runTimeExecutable.

Would be nice to allow simply entering edge as a launch runtime executable. I assume Edge has well-known location(s). findBrowser.ts has a bunch of platform logic for this; you could add a new Executable in win32() that discovers an edge type, and then it'll work.

Add browser version support (stable, beta, dev, canary) for Microsoft Edge (Chromium)
to match backwards compatibility with vscode-edge-debug2.

Currently for Chrome we overload runtimeExecutable and allow users to enter stable, canary, etc. With multiple browsers this gets awkward. Maybe we introduce a separate browserVersion property for this.

* Minimum changed needed to support `edge` debugger type, it is just an alias for `chrome`. * This change is equivalent to the launch.json `{type: 'chrome', runTimeExecutable: 'path/to/msedge.exe'}`. * Minimum change needed to support backwards compatibility with WebView2 launch debugging configurations (`useWebView`) from [vscode-edge-debug2][1]. * Adds `urlFilter` support for `chrome` `launch` requests. Follow-ups to consider: * Separate `chrome` and `edge` debugger type configurations with Edge strings. * Validate `useWebView` configuration is configured with `runTimeExecutable`. * Tests for `useWebView` configuration. * Add browser `version` support (`stable`, `beta`, `dev`, `canary`) for Microsoft Edge (Chromium) to match backwards compatibility with [vscode-edge-debug2][1]. * Deprecate `{useWebView: 'advanced'}` in favor of `{useWebView: true, urlFilter: 'bing.com'}` * Deprecate `useWebView` in favor of `webview` debugger type. [1]: https://github.com/microsoft/vscode-edge-debug

…caught in tests where launching with a URL other than about:blank was broken because setting the launch URL happens after the target is attached, but it would never attach if the filter failed.

johnemau · 2020-01-27T21:46:56Z

I ran the Chrome integration tests locally (npm run intTests) and here are my results:

  13 passing (5m)
  46 pending
  32 failing

The failures do not match what was reported in issues #44, however I am not* worried because the report is likely out of date and I assume to need undocumented local changes to prune out false-negatives.

johnemau · 2020-01-27T21:58:37Z

Would be happy to have that in this PR, but it could be a followup item for anyone to implement.

Lets do this in a follow-up PR for anyone to implement.

I assume Edge has well-known location(s). findBrowser.ts has a bunch of platform logic for this; you could add a new Executable in win32() that discovers an edge type, and then it'll work.

I agree and propose we update findBrowser.ts in a follow-up PR.

Currently for Chrome we overload runtimeExecutable and allow users to enter stable, canary, etc. With multiple browsers this gets awkward. Maybe we introduce a separate browserVersion property for this.

Sounds good to me, another good follow-up PR.

@connor4312, what is your preferred way to track the follow-up work?
Suggestions: I can file separate github issues or we can start a checklist on issue #235

connor4312 · 2020-01-27T22:03:30Z

I'd prefer separate Github issues that reference the main one; easier to track ongoing milestones, state, and assignments at a more granular level that way.

Chrome integration tests

These aren't super useful right now. It's safe to ignore that result, we have been discussing an effort to either remove or merge the relevant cases into the main set of integration tests.

connor4312 · 2020-01-27T22:05:33Z

Let me know when you're happy merging this PR, and I'll get it in then. Note that this is endgame week for VS Code so we won't be as responsive until we normally are until Monday.

johnemau · 2020-01-27T22:13:28Z

Thanks for the review @connor4312! This PR ready to land so please merge! 🚢 🚀

Per your suggestion I will file separate github issues for the follow-up work and reference the main Edge issue #235.

src/build/strings.ts

package.json

src/targets/browser/browserLauncher.ts

…emoved legacy fallback port of 2015, and added '(Chromium)' suffix to Edge identifier

roblourens · 2020-01-29T03:52:54Z

Is there any way to use "edge" for the new Edge and some other id for the legacy version? I think people would go for edge over msedge by default. And how many people want to use legacy edge when the new one is available?

dhanvikapila · 2020-01-29T04:44:59Z

From a VS point of view there are going to be significant numbers of users for both versions of Edge. For external customer facing strong sure we can say edge in v3 but we need to say msedge in telemetry to be able to differentiate between old edge and new one. - Dhanvi

…

________________________________ From: Rob Lourens <notifications@github.com> Sent: Tuesday, January 28, 2020 7:52:55 PM To: microsoft/vscode-js-debug <vscode-js-debug@noreply.github.com> Cc: Dhanvi Kapila <Dhanvi.Kapila@microsoft.com>; Comment <comment@noreply.github.com> Subject: Re: [microsoft/vscode-js-debug] feat: WebView2 debugging support (#264) Is there any way to use "edge" for the new Edge and some other id for the legacy version? I think people would go for edge over msedge by default. And how many people want to use legacy edge when the new one is available? — You are receiving this because you commented. Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fvscode-js-debug%2Fpull%2F264%3Femail_source%3Dnotifications%26email_token%3DAH6I4KLZHAYCWUF2LJO66SDRAD4RPA5CNFSM4KMFHWT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKF275Q%23issuecomment-579579894&data=02%7C01%7CDhanvi.Kapila%40microsoft.com%7C3a58a5000a73469c2fed08d7a46eb935%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637158667770168102&sdata=rlbXKPDer0xXRHqtC4YJd6Y2E5d9ZzMKS9eUcYNX57s%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAH6I4KMSQ7PTXPE5DXAQ3JTRAD4RPANCNFSM4KMFHWTQ&data=02%7C01%7CDhanvi.Kapila%40microsoft.com%7C3a58a5000a73469c2fed08d7a46eb935%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637158667770178059&sdata=ocW4Sx7MkhaUodGpfeWK8rdtBfYU%2B3hdqkmCxq2ujks%3D&reserved=0>.

roblourens · 2020-01-29T05:05:06Z

I think we should use "edge" for user-facing things in VS Code and we can map it to msedge for telemetry, that's fine.

connor4312 requested review from roblourens and connor4312 January 27, 2020 17:47

John Emau added 2 commits January 27, 2020 13:22

Only set filter for advanced web view debugging. This fixes an issue …

de0ebbb

…caught in tests where launching with a URL other than about:blank was broken because setting the launch URL happens after the target is attached, but it would never attach if the filter failed.

connor4312 approved these changes Jan 27, 2020

View reviewed changes

connor4312 requested a review from Shenniey January 27, 2020 23:47

dhanvikapila reviewed Jan 28, 2020

View reviewed changes

src/build/strings.ts Outdated Show resolved Hide resolved

dhanvikapila reviewed Jan 28, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/targets/browser/browserLauncher.ts Outdated Show resolved Hide resolved

Addressing review feedback: Changed target from edge to msedge, r…

56bebee

…emoved legacy fallback port of 2015, and added '(Chromium)' suffix to Edge identifier

connor4312 merged commit 811b2e7 into microsoft:master Jan 28, 2020

This was referenced Jan 30, 2020

feat: support multi-browser process web view debugging #285

Merged

Tests: Add integration test for WebView launch scenario #303

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: WebView2 debugging support #264

feat: WebView2 debugging support #264

johnemau commented Jan 27, 2020 •

edited

Loading

connor4312 commented Jan 27, 2020

johnemau commented Jan 27, 2020 •

edited

Loading

johnemau commented Jan 27, 2020

connor4312 commented Jan 27, 2020 •

edited

Loading

connor4312 commented Jan 27, 2020

johnemau commented Jan 27, 2020 •

edited

Loading

roblourens commented Jan 29, 2020

dhanvikapila commented Jan 29, 2020 via email

roblourens commented Jan 29, 2020

feat: WebView2 debugging support #264

feat: WebView2 debugging support #264

Conversation

johnemau commented Jan 27, 2020 • edited Loading

connor4312 commented Jan 27, 2020

johnemau commented Jan 27, 2020 • edited Loading

johnemau commented Jan 27, 2020

connor4312 commented Jan 27, 2020 • edited Loading

connor4312 commented Jan 27, 2020

johnemau commented Jan 27, 2020 • edited Loading

roblourens commented Jan 29, 2020

dhanvikapila commented Jan 29, 2020 via email

roblourens commented Jan 29, 2020

johnemau commented Jan 27, 2020 •

edited

Loading

johnemau commented Jan 27, 2020 •

edited

Loading

connor4312 commented Jan 27, 2020 •

edited

Loading

johnemau commented Jan 27, 2020 •

edited

Loading