Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P0: VS Code: Delegate launching browser to DevTools to always get Chrome #1505

Closed
DanTup opened this issue Mar 4, 2019 · 12 comments · Fixed by #1645
Closed

P0: VS Code: Delegate launching browser to DevTools to always get Chrome #1505

DanTup opened this issue Mar 4, 2019 · 12 comments · Fixed by #1645
Assignees
Labels
important Something important! in commands Relates to commands (usually invoked from the command Palette) is enhancement
Milestone

Comments

@DanTup
Copy link
Member

DanTup commented Mar 4, 2019

Requires flutter/devtools#463

@DanTup DanTup added is enhancement in commands Relates to commands (usually invoked from the command Palette) labels Mar 4, 2019
@DanTup DanTup added this to the v2.25.0 milestone Mar 4, 2019
@DanTup
Copy link
Member Author

DanTup commented Mar 4, 2019

This will be handled in the server instead.

@DanTup DanTup closed this as completed Mar 4, 2019
@DanTup DanTup removed in commands Relates to commands (usually invoked from the command Palette) is enhancement labels Mar 4, 2019
@DanTup DanTup removed this from the v2.25.0 milestone Mar 4, 2019
@skybur
Copy link

skybur commented Apr 5, 2019

Sorry for commenting in this closed issue. So how do I start devtools in chrome if it's not my default browser? Thanks

@DanTup
Copy link
Member Author

DanTup commented Apr 5, 2019

Currently you'd have to open Chrome manually and copy the URL from the default browser that is launched. This will be fixed, but requires flutter/devtools#463.

I'm re-opening this, since there will be work today when that lands which I forgot about.

@DanTup DanTup reopened this Apr 5, 2019
@DanTup DanTup added this to the v2.26.0 milestone Apr 5, 2019
@DanTup DanTup added blocked on dart / flutter Requires a change in Dart or Flutter to progress in commands Relates to commands (usually invoked from the command Palette) is enhancement labels Apr 5, 2019
@DanTup DanTup changed the title Try to always run Chrome for DevTools instead of default browser VS Code: Try to always run Chrome for DevTools instead of default browser Apr 10, 2019
@DanTup DanTup changed the title VS Code: Try to always run Chrome for DevTools instead of default browser P?: VS Code: Try to always run Chrome for DevTools instead of default browser Apr 10, 2019
@DanTup DanTup changed the title P?: VS Code: Try to always run Chrome for DevTools instead of default browser P?: VS Code: Delegate launching browser to DevTools to always get Chrome Apr 10, 2019
@DanTup DanTup added the important Something important! label Apr 10, 2019
@DanTup DanTup self-assigned this Apr 10, 2019
@devoncarew
Copy link
Contributor

From the above comments, it sounds like this is in progress?

@DanTup
Copy link
Member Author

DanTup commented Apr 22, 2019

@devoncarew Yeah, DevTools now allows you to send it VM service URIs while it's running (over stdin, like { method: vm.register, params: { uri: <vm-service-uri> } } and it'll connect to them and register a launchDevTools service. If you call it, it'll spawn the browser for you.

It's not ready to go yet, but here's how it works in Dart Code (on a branch):

Register VMs with DevTools when a debug session starts (and for any existing sessions at start time):

83f2eeb#diff-5611b1bb550fbff23450e8dd4f2dfb2aR98

Call the service when we need to open the browser:

83f2eeb#diff-5611b1bb550fbff23450e8dd4f2dfb2aR62

It needs some additional changes landing in DevTools (flutter/devtools#563) and also some tweaks to support arguments for theme/hide - they're not done yet though.

@devoncarew
Copy link
Contributor

Interesting - we don't ask the devtools server to open a browser tab? We ask it to register with the vm service protocol, then ask over the vm service protocol to open a browser?

I'm curious why that is - it seems more straightforward to ask the devtools server to open a browser tab directly.

@DanTup
Copy link
Member Author

DanTup commented Apr 22, 2019

@jacob314 can probably answer this best, but I believe it's because some of the things we'll ask from the DevTools server (eg. to provide data for widget rebuilds, etc.) will need it to have connected to the VM service anyway (so this keeps those things working the same).

I think we could potentially expose it over stdin too if you think it makes sense (for ex. if IntelliJ won't make use of the other services, that would probably simplify things a little for it).

@DanTup DanTup removed the blocked on dart / flutter Requires a change in Dart or Flutter to progress label Apr 22, 2019
@csells csells changed the title P?: VS Code: Delegate launching browser to DevTools to always get Chrome P0: VS Code: Delegate launching browser to DevTools to always get Chrome Apr 22, 2019
@jacob314
Copy link

Yes this design seems very odd without context of what we will do with the server next.
If this was the only feature we needed from the server for then passing in the VM service port would be overkill.
The next uses for the server are:

  1. Rebuild indicator stats tracked on the server and consumed by VSCode & DevTools.
  2. Triggering opening DevTools from the device itself by clicking on the debug banner or a debug gesture.
  3. Database of memory profiling data.
  4. Record log messages sent before devtools opens. Currently all such log messages are lost resulting a confusing experience.

All these use cases are easier or only possible if the server knows about all running apps.

@DanTup DanTup removed this from the v2.26.0 milestone Apr 25, 2019
@DanTup DanTup added this to the v2.26.1 milestone Apr 25, 2019
@devoncarew
Copy link
Contributor

Gotcha. That all makes sense - adding more functionality into the server, so that its logic can serve multiple clients.

For some clients, it may be difficult to use devtools in this manner to launch chrome however (this would look like: tell devtools about the new service protocol url; devtools registers a 'launchDevTools' service with the service protocol; IntelliJ waits for a 'launchDevTools' service to be available on the service protocol and uses it to open a browser). That would be a lot of async programming for a platform where that's a bit cumbersome.

We may want the devtools server to support a stdio command along the order of devtools.openBrowser: serviceProtocolUrl. IntelliJ could call that and pass in the url for a service protocol connection. Devtools could open a browser configured to connect to the service protocol connection (and possibly, optionally register the service protocol url with the known list of running vms).

@jacob314
Copy link

Yes adding a 'devtools.openBrowser' api on stdio makes sense. It should be very little work to support both.

@DanTup
Copy link
Member Author

DanTup commented Apr 30, 2019

I've opened flutter/devtools#611 in the DevTools repo for supporting this from stdin (since this issue will soon be closed). If it's urgent, let me know.

@DanTup
Copy link
Member Author

DanTup commented May 1, 2019

I'm disabling this for v2.26.1 because I'd like to release the other fixes and there are complications with this. If it fails - most likely because Chrome isn't installed - it seems to unregister the service unexpectedly. I'll put it back in another patch once I'm sure we can handle the failure in a reasonable way.

@DanTup DanTup reopened this May 1, 2019
@DanTup DanTup modified the milestones: v2.26.1, v2.26.2 May 1, 2019
DanTup added a commit that referenced this issue May 1, 2019
DanTup added a commit that referenced this issue May 1, 2019
@DanTup DanTup closed this as completed in 674e912 May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
important Something important! in commands Relates to commands (usually invoked from the command Palette) is enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants