Code review comment for lp:~axwalk/juju-core/apiclient-open-parallel

A couple of comments, but nothing that would block landing this.

LGTM

https://codereview.appspot.com/82450043/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/82450043/diff/1/state/api/apiclient.go#newcode142
state/api/apiclient.go:142: // TODO what does "origin" really mean, and
is localhost always ok?
origin would have sense if you were doing actual web requests, I think
it is the location that redirected you here.
I believe the websocket requires something, but it accepts localhost, so
we just do it.

It isn't actually used in our implementation, so it is ok.

See something like:
http://learnitcorrect.com/blog/websocket-is-great-but-not-the-origin-policy.html

It is intended to avoid cross-site scripting, etc. But we just let
people connect to our websockets, and do the auth inside the websocket
(as part of Login) rather than assuming that connections to us are
already valid.

https://codereview.appspot.com/82450043/diff/1/state/api/apiclient.go#newcode167
state/api/apiclient.go:167: return conn, nil
Would we want to check <-stop at this point, in case we got a stop while
the dial was processing?

I guess it has to be handled up a layer anyway. We just know that
DialConfig is blocking, so we sort of expect to be in the middle of
dialing while we get a stop request. (because someone else succeeded.)

https://codereview.appspot.com/82450043/diff/1/state/api/apiclient.go#newcode169
state/api/apiclient.go:169: log.Debugf("state/api: %v", err)
maybe a little more context about "error trying to Dial API server, will
retry: %v" ?

It depends what errors we generally get, but it seems helpful to give a
bit more context than just an error.

https://codereview.appspot.com/82450043/