Merge lp://staging/~gary/juju-gui/socket_url_revisited into lp://staging/juju-gui/experimental

Reviewers: mp+153202_code.launchpad.net,

Message:
Please take a look.

Description:
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.

https://code.launchpad.net/~gary/juju-gui/socket_url_revisited/+merge/153202

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7703047/

Affected files:
   A [revision details]
   M app/app.js
   M app/config-debug.js
   M app/config-prod.js
   M app/index.html
   M test/test_app.js

LGTM, thanks.

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

LGTM -- one tiny change.

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');
Don't we prefer 'one var per declaration'?

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

https://codereview.appspot.com/7703047/diff/1/test/test_app.js#newcode472
test/test_app.js:472: });
These tests are clean and thorough. Thanks.

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

LGTM Thanks for doing this :-) one trivial and one question.

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');
Should these not be camelCased?

https://codereview.appspot.com/7703047/diff/1/app/app.js#newcode281
app/app.js:281: socket_protocol = socket_protocol || 'ws';
I wonder if we shouldn't default this to 'wss' instead of 'ws' thoughts?

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

*** 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/