Improve UI of Chromoting client UI to select available hosts.

Improve UI of Chromoting client UI to select available hosts.

BUG=53988
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58862

[Sergey Ulanov, 2010-09-07 22:03:30]

http://codereview.chromium.org/3338014/diff/4001/5009
File remoting/tools/gethosts.sh (right):

http://codereview.chromium.org/3338014/diff/4001/5009#newcode1
remoting/tools/gethosts.sh:1: #!/bin/bash
Maybe add some comments about what this script is doing, and why we need it.

http://codereview.chromium.org/3338014/diff/4001/5009#newcode5
remoting/tools/gethosts.sh:5: echo ""
You don't need quotes here. Just "echo" will do the same thing.

http://codereview.chromium.org/3338014/diff/7002/9002
File remoting/client/extension/client.js (right):

http://codereview.chromium.org/3338014/diff/7002/9002#newcode12
remoting/client/extension/client.js:12: window.setTimeout(listHosts, 500);
Why 500? Isn't it too much?

http://codereview.chromium.org/3338014/diff/7002/9002#newcode81
remoting/client/extension/client.js:81: var count = 2;
Maybe just use two flags: chromoting_auth_done and chromiumsync_auth_done?

http://codereview.chromium.org/3338014/diff/7002/9002#newcode90
remoting/client/extension/client.js:90: function(cauth_token) {
camelCase

http://codereview.chromium.org/3338014/diff/7002/9002#newcode92
remoting/client/extension/client.js:92: var status =
document.getElementById('login_status');
Do we still need this line?

http://codereview.chromium.org/3338014/diff/7002/9002#newcode96
remoting/client/extension/client.js:96: function(xauth_token) {
camelCase

http://codereview.chromium.org/3338014/diff/7002/9004
File remoting/client/extension/login.html (right):

http://codereview.chromium.org/3338014/diff/7002/9004#newcode20
remoting/client/extension/login.html:20: <td align="left"><input type="text"
name="username" id="username" value="" /></td>
80 chars

[garykac, 2010-09-08 18:41:39]

http://codereview.chromium.org/3338014/diff/4001/5009
File remoting/tools/gethosts.sh (right):

http://codereview.chromium.org/3338014/diff/4001/5009#newcode1
remoting/tools/gethosts.sh:1: #!/bin/bash
On 2010/09/07 22:03:30, sergeyu wrote:
> Maybe add some comments about what this script is doing, and why we need it.

Done.

http://codereview.chromium.org/3338014/diff/4001/5009#newcode5
remoting/tools/gethosts.sh:5: echo ""
On 2010/09/07 22:03:30, sergeyu wrote:
> You don't need quotes here. Just "echo" will do the same thing.

Done.

http://codereview.chromium.org/3338014/diff/7002/9002
File remoting/client/extension/client.js (right):

http://codereview.chromium.org/3338014/diff/7002/9002#newcode12
remoting/client/extension/client.js:12: window.setTimeout(listHosts, 500);
On 2010/09/07 22:03:30, sergeyu wrote:
> Why 500? Isn't it too much?

It doesn't seem to matter that much since the extension popup takes about 1/2
sec to pop up. But I changed it to 100ms.

http://codereview.chromium.org/3338014/diff/7002/9002#newcode81
remoting/client/extension/client.js:81: var count = 2;
On 2010/09/07 22:03:30, sergeyu wrote:
> Maybe just use two flags: chromoting_auth_done and chromiumsync_auth_done?

This is the way Albert wrote it originally. I didn't see a problem with the way
it was so I left it.

http://codereview.chromium.org/3338014/diff/7002/9002#newcode90
remoting/client/extension/client.js:90: function(cauth_token) {
On 2010/09/07 22:03:30, sergeyu wrote:
> camelCase

Done.

http://codereview.chromium.org/3338014/diff/7002/9002#newcode92
remoting/client/extension/client.js:92: var status =
document.getElementById('login_status');
On 2010/09/07 22:03:30, sergeyu wrote:
> Do we still need this line?

Nope. Leftovers from when I had debugging info. Thanks!

http://codereview.chromium.org/3338014/diff/7002/9002#newcode96
remoting/client/extension/client.js:96: function(xauth_token) {
On 2010/09/07 22:03:30, sergeyu wrote:
> camelCase 

Done.

http://codereview.chromium.org/3338014/diff/7002/9004
File remoting/client/extension/login.html (right):

http://codereview.chromium.org/3338014/diff/7002/9004#newcode20
remoting/client/extension/login.html:20: <td align="left"><input type="text"
name="username" id="username" value="" /></td>
On 2010/09/07 22:03:30, sergeyu wrote:
> 80 chars

Done.

[Sergey Ulanov, 2010-09-08 18:57:31]

LGTM, but can you please rename all variables that still don't use camelCase?
Thanks!

http://codereview.chromium.org/3338014/diff/12/16002
File remoting/client/extension/client.js (right):

http://codereview.chromium.org/3338014/diff/12/16002#newcode20
remoting/client/extension/client.js:20: var login_div =
document.getElementById('login_div');
camelCase

http://codereview.chromium.org/3338014/diff/12/16002#newcode189
remoting/client/extension/client.js:189: var error_response =
JSON.parse(xhr.responseText);
camelCase

http://codereview.chromium.org/3338014/diff/12/16002#newcode215
remoting/client/extension/client.js:215: var hostlist_div =
document.getElementById('hostlist_div');
camelCase

http://codereview.chromium.org/3338014/diff/12/16002#newcode238
remoting/client/extension/client.js:238: var host_entry =
document.createElement('div');
camelCase here and below.

[garykac, 2010-09-08 19:43:36]

http://codereview.chromium.org/3338014/diff/12/16002
File remoting/client/extension/client.js (right):

http://codereview.chromium.org/3338014/diff/12/16002#newcode20
remoting/client/extension/client.js:20: var login_div =
document.getElementById('login_div');
On 2010/09/08 18:57:31, sergeyu wrote:
> camelCase

Done.

http://codereview.chromium.org/3338014/diff/12/16002#newcode189
remoting/client/extension/client.js:189: var error_response =
JSON.parse(xhr.responseText);
On 2010/09/08 18:57:31, sergeyu wrote:
> camelCase

Done.

http://codereview.chromium.org/3338014/diff/12/16002#newcode215
remoting/client/extension/client.js:215: var hostlist_div =
document.getElementById('hostlist_div');
On 2010/09/08 18:57:31, sergeyu wrote:
> camelCase

Done.

http://codereview.chromium.org/3338014/diff/12/16002#newcode238
remoting/client/extension/client.js:238: var host_entry =
document.createElement('div');
On 2010/09/08 18:57:31, sergeyu wrote:
> camelCase here and below.

Done.

[Sergey Ulanov, 2010-09-08 19:53:32]

LGTM