Code review comment for lp://staging/~gary/juju-gui/socket_url_revisited

Revision history for this message
Gary Poster (gary) wrote :

*** Submitted:

Prefer socket_port and socket_protocol

In order to allow the charm to be backwards compatible, change the
websocket url code to prefer socket_port and/or socket_protocol if they
are also provided with socket_url in the configuration. Added tests.
Moved pertinent code from index.html to app because it was easier to
test there.

R=bcsaller, bac, jeff.pihach
CC=
https://codereview.appspot.com/7703047

https://codereview.appspot.com/7703047/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7703047/diff/1/app/app.js#newcode276
app/app.js:276: socket_protocol = this.get('socket_protocol');
On 2013/03/13 17:41:15, bac wrote:
> Don't we prefer 'one var per declaration'?

Done.

https://codereview.appspot.com/7703047/diff/1/app/app.js#newcode276
app/app.js:276: socket_protocol = this.get('socket_protocol');
On 2013/03/13 17:47:43, jeff.pihach wrote:
> Should these not be camelCased?

Done.

https://codereview.appspot.com/7703047/diff/1/app/app.js#newcode281
app/app.js:281: socket_protocol = socket_protocol || 'ws';
On 2013/03/13 17:47:43, jeff.pihach wrote:
> I wonder if we shouldn't default this to 'wss' instead of 'ws'
thoughts?

I thought and then agreed. :-) Done.

https://codereview.appspot.com/7703047/

« Back to merge proposal