Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2325)

Issue 7703047: Prefer socket_port and socket_protocol

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years ago by gary.poster
Modified:
11 years ago
Reviewers:
bac, bcsaller, jeff.pihach, mp+153202
Visibility:
Public.

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)

Patch Set 1 #

Total comments: 7

Patch Set 2 : Prefer socket_port and socket_protocol #

Unified diffs Side-by-side diffs Delta from patch set Stats (+128 lines, -19 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M app/app.js View 1 1 chunk +17 lines, -1 line 0 comments Download
M app/config-debug.js View 1 1 chunk +7 lines, -0 lines 0 comments Download
M app/config-prod.js View 1 1 chunk +7 lines, -0 lines 0 comments Download
M app/index.html View 1 chunk +0 lines, -12 lines 0 comments Download
M test/test_app.js View 1 3 chunks +95 lines, -6 lines 0 comments Download

Messages

Total messages: 5
gary.poster
Please take a look.
11 years ago (2013-03-13 17:26:08 UTC) #1
bcsaller
LGTM, thanks.
11 years ago (2013-03-13 17:30:18 UTC) #2
bac
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 ...
11 years ago (2013-03-13 17:41:14 UTC) #3
jeff.pihach
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): ...
11 years ago (2013-03-13 17:47:43 UTC) #4
gary.poster
11 years ago (2013-03-13 18:03:51 UTC) #5
*** 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b