code review 6945074: gotour: automatically open browser window if possible

gotour: automatically open browser window if possible
Fixes issue 51.

[minux1, 2012-12-18 14:36:15]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go-tour/

[minux1, 2012-12-18 16:58:48]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2012-12-18 22:22:04]

https://codereview.appspot.com/6945074/diff/3004/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6945074/diff/3004/gotour/local.go#newcode199
gotour/local.go:199: for {
If someone runs gotour -http=:8080, this loop will run forever. Please add a
maximum number of iterations.

There should be some logic to detect whether a hostname is provided, and if not
it should substitute 127.0.0.1 (I guess).

https://codereview.appspot.com/6945074/diff/3004/gotour/local.go#newcode205
gotour/local.go:205: time.Sleep(10 * time.Millisecond)
do we really want to try 100 times per second? 100 milliseconds should be ok.

https://codereview.appspot.com/6945074/diff/3004/gotour/local.go#newcode221
gotour/local.go:221: log.Printf("A browser window should open, if not, please
visit %s manually", url)
s/ manually//

https://codereview.appspot.com/6945074/diff/3004/gotour/local.go#newcode221
gotour/local.go:221: log.Printf("A browser window should open, if not, please
visit %s manually", url)
s/, if/. If/

[minux1, 2012-12-18 23:07:08]

https://codereview.appspot.com/6945074/diff/3004/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6945074/diff/3004/gotour/local.go#newcode199
gotour/local.go:199: for {
On 2012/12/18 22:22:04, adg wrote:
> If someone runs gotour -http=:8080, this loop will run forever. Please add a
> maximum number of iterations.
Done
> There should be some logic to detect whether a hostname is provided, and if
not
> it should substitute 127.0.0.1 (I guess).
Done. Actually, this change makes it impossible to bind on wildcard address,
however, i think it is correct due to security implications (if the user wants,
he can
always bind to a specific network address).

https://codereview.appspot.com/6945074/diff/3004/gotour/local.go#newcode205
gotour/local.go:205: time.Sleep(10 * time.Millisecond)
On 2012/12/18 22:22:04, adg wrote:
> do we really want to try 100 times per second? 100 milliseconds should be ok.
Done.

https://codereview.appspot.com/6945074/diff/3004/gotour/local.go#newcode221
gotour/local.go:221: log.Printf("A browser window should open, if not, please
visit %s manually", url)
On 2012/12/18 22:22:04, adg wrote:
> s/, if/. If/
Both done.

[minux1, 2012-12-18 23:07:37]

Hello golang-dev@googlegroups.com, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2012-12-18 23:19:07]

LGTM

https://codereview.appspot.com/6945074/diff/20001/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6945074/diff/20001/gotour/local.go#newcode215
gotour/local.go:215: if tries != 0 && startBrowser(url) == nil {
s/!=/>/

[adg, 2012-12-18 23:19:49]

NOT LGTM

We should add a flag -openbrowser=true so that you can run the tour without
opening a browser.

[minux1, 2012-12-18 23:21:41]

On Wed, Dec 19, 2012 at 7:19 AM, <adg@golang.org> wrote:

> We should add a flag -openbrowser=true so that you can run the tour
> without opening a browser.
>
in fact, my local version had just that option to facilitate the testing of
various -http
parameters.

[minux1, 2012-12-18 23:24:25]

PTAL.

[adg, 2012-12-19 00:28:01]

https://codereview.appspot.com/6945074/diff/30001/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6945074/diff/30001/gotour/local.go#newcode84
gotour/local.go:84: go waitAndStartBrowser("http://" + host + ":" + port)
this is kind of nitpicky, but I would prefer these two new functions weren't
intermingled

url := ...
go func() {
  if *openBrowser && waitForServer(url) && startBrowser(url) {
        log.Printf("A browser window...")
        return
  }
  log.Printf("Please open your...")
}()

https://codereview.appspot.com/6945074/diff/30001/gotour/local.go#newcode204
gotour/local.go:204: func waitAndStartBrowser(url string) {
return a bool

https://codereview.appspot.com/6945074/diff/30001/gotour/local.go#newcode206
gotour/local.go:206: tries := 100
10 seconds seems a long time to wait. reduce to 20 tries? or even 10?

https://codereview.appspot.com/6945074/diff/30001/gotour/local.go#newcode224
gotour/local.go:224: func startBrowser(url string) error {
we don't use the error, so just return a bool

[minux1, 2012-12-19 16:20:10]

PTAL.

[adg, 2012-12-19 20:53:44]

LGTM

Thanks!

[minux1, 2012-12-20 18:52:34]

*** Submitted as https://code.google.com/p/go-tour/source/detail?r=680e321d81fd
***

gotour: automatically open browser window if possible
Fixes issue 51.

R=golang-dev, adg
CC=golang-dev
https://codereview.appspot.com/6945074