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

cmd/pprof: select browser for web output more intelligently #10259

Closed
jonhoo opened this issue Mar 26, 2015 · 7 comments
Closed

cmd/pprof: select browser for web output more intelligently #10259

jonhoo opened this issue Mar 26, 2015 · 7 comments
Milestone

Comments

@jonhoo
Copy link

@jonhoo jonhoo commented Mar 26, 2015

When asking pprof for output using web, it currently selects browsers using an in-order traversal of the list '"chrome", "google-chrome", "firefox"'. On each platform, it also tries a platform-specific file opener (xdg-open on *nix) if all the list of browsers all fail to execute.

This behavior is non-intuitive, as users' browser preferences are not respect if they have one of those browsers installed, but use another browser by default (this is particularly common for web developers). Instead, only the platform-specific opener should be used, and only in the case where it fails should fallbacks be attempted.

@nightlyone
Copy link
Contributor

@nightlyone nightlyone commented Mar 27, 2015

Yes, I agree that the defaults are in the wrong order. The platform-specific file openers should be first and the opinionated fallbacks should come later.

The fix is simple:

diff --git a/src/cmd/pprof/internal/commands/commands.go b/src/cmd/pprof/internal/commands/commands.go
index 51397a3..2c5e2d0 100644
--- a/src/cmd/pprof/internal/commands/commands.go
+++ b/src/cmd/pprof/internal/commands/commands.go
@@ -82,7 +82,8 @@ func PProf(c Completer, interactive **bool, svgpan **string) Commands {
 // browsers returns a list of commands to attempt for web visualization
 // on the current platform
 func browsers() []string {
-       cmds := []string{"chrome", "google-chrome", "firefox"}
+       var cmds []string
+
        switch runtime.GOOS {
        case "darwin":
                cmds = append(cmds, "/usr/bin/open")
@@ -91,7 +92,7 @@ func browsers() []string {
        default:
                cmds = append(cmds, "xdg-open")
        }
-       return cmds
+       return append(cmds, "chrome", "google-chrome", "firefox")
 }

Any other opinions? If ok, I will send a CL.

@jonhoo
Copy link
Author

@jonhoo jonhoo commented Mar 27, 2015

I suspect the fallbacks will never be used in this case (well, unless the OS opener does not exist). We could potentially get rid of them altogether?

@nightlyone
Copy link
Contributor

@nightlyone nightlyone commented Mar 27, 2015

I would keep them, since I don't know how dragonfly or plan9 implements that.

@jonhoo
Copy link
Author

@jonhoo jonhoo commented Mar 27, 2015

Wouldn't a preferred fallback to $BROWSER be better for those OSes?

@nightlyone
Copy link
Contributor

@nightlyone nightlyone commented Mar 28, 2015

I honestly don't know, so I suggested the minimal fix.

@jonhoo
Copy link
Author

@jonhoo jonhoo commented Mar 28, 2015

My guess for best order of operations would be ["os-specific opener", $BROWSER if set, "chrome", "google-chrome", "chromium" "firefox"].

The fallback list also has other problems like the "google-chrome" binary sometimes being called "google-chrome-stable", beta versions of browsers are never used, Opera and Safari aren't mentioned, etc., but those are second-order concerns.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 15, 2015

CL https://golang.org/cl/12201 mentions this issue.

@rsc rsc closed this in 2bd1e5e Jul 15, 2015
@mikioh mikioh modified the milestones: Go1.5, Go1.5Maybe Jul 16, 2015
@golang golang locked and limited conversation to collaborators Jul 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.