Fix up remoting UI to make it closer to spec

Fix up remoting UI to make it closer to spec


BUG=NONE
TEST=Run Remoting and appreciate it's new shininess.

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

[dmac, 2011-05-28 00:40:31]

PTAL

[Jamie, 2011-05-31 18:11:49]

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
File remoting/webapp/me2mom/choice.html (right):

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:19: <div id="auth_panel">
I think it would be good if this panel had a different colour if
unauthenticated, or otherwise indicated somehow that the app is not yet ready to
run. Colours are perhaps too culture-specific, I'm not sure.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:27: onchange="handleOAuth2CodeChange(this);"
id="oauth2_code" />
Why trap both onkeyup and onchange, and why pass 'this' only to one of them. I
think the function should be declared as taking a 'form' argument as it gives a
more convenient syntax for accessing the fields (form['field-name']).

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:27: onchange="handleOAuth2CodeChange(this);"
id="oauth2_code" />
Having the id after the onchange makes it easy to miss. I've tried to be
consistent about having one attribute per line in this file; failing that, it
would be better on the first line, after the name.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:37: Email:<span id="current_email"></span>
Add a space after "Email:". It looks a bit cramped otherwise.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:69: </div> <!-- xmpp_div -->
I think I'd lose this div entirely now. I know Albert wanted to keep it in case
it was needed for testing, so it would be good to check with him first.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:73: <div id="container">
Can we give this a more descriptive name, if it needs a name at all?

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:75: <img height="64" width="64"
src="chromoting128.png">
Do we have a 64x64 version of this icon? If not, we ought to get one rather than
scaling down the 128x128 one.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:99: style="width:173px">
Why specify the width explicitly?

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:101: </button>
Can we disable this button if the OAuth2 token isn't present? Or maybe show an
error if the user clicks it in this case.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:108: </div> <!-- preparing_to_share -->
This is never shown at present, correct? As long as this is on the TODO list, I
don't have a problem with that, but at the moment, clicking 'Share this
computer' twice crashes the plugin.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:127: </div> <!-- ready_to_share -->
This div was never shown in my tests, although the code looks fine. I'm guessing
there's a bug in the plugin but since it's not related to this CL, let's move
on.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:145: &lsquo;Share this computer&rsquo; then
read to you their security
s/security/access/

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:153: <button onclick="tryConnect();"
style="width: 80px">
Get rid of width?

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:155: </button>
Can we disable this if either the Oauth2 token or email address aren't set? Or
maybe show an error if the user tries to click it in this case.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:186: style="width: 80px">
Get rid of width?

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:200: </a>.
Move the closing tag to the preceding line, otherwise there's a space before the
full stop.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:208: </a>.
Move the closing tag to the preceding line, otherwise there's a space before the
full stop.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/remot...
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/remot...
remoting/webapp/me2mom/remoting.js:366: function tryConnect() {
Why not pass the access code in as before? That way, the place where you need to
know the id is right next to the id itself and the two are easier to keep in
sync.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/remot...
File remoting/webapp/me2mom/remoting_session.html (left):

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/remot...
remoting/webapp/me2mom/remoting_session.html:17: <span id="status_msg"
class="status_msg">Initializing...</span>
It's not clear to me what the purpose of the changes in this file are.

[dmac, 2011-06-01 23:01:57]

Fixed (or responded to) all of your issues. Did some refactoring along the way
to try and clean us up a bit and get rid of some repetition. Also fixed up some
of our HTML to make it HTML5 compliant.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
File remoting/webapp/me2mom/choice.html (right):

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:19: <div id="auth_panel">
On 2011/05/31 18:11:49, Jamie wrote:
> I think it would be good if this panel had a different colour if
> unauthenticated, or otherwise indicated somehow that the app is not yet ready
to
> run. Colours are perhaps too culture-specific, I'm not sure.

Done.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:27: onchange="handleOAuth2CodeChange(this);"
id="oauth2_code" />
On 2011/05/31 18:11:49, Jamie wrote:
> Why trap both onkeyup and onchange, and why pass 'this' only to one of them. I
> think the function should be declared as taking a 'form' argument as it gives
a
> more convenient syntax for accessing the fields (form['field-name']).


I need onkeyup to handle typing, and onchange to handle paste. Passing this was
an error.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:37: Email:<span id="current_email"></span>
On 2011/05/31 18:11:49, Jamie wrote:
> Add a space after "Email:". It looks a bit cramped otherwise.

Done.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:69: </div> <!-- xmpp_div -->
On 2011/05/31 18:11:49, Jamie wrote:
> I think I'd lose this div entirely now. I know Albert wanted to keep it in
case
> it was needed for testing, so it would be good to check with him first.

Done.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:73: <div id="container">
On 2011/05/31 18:11:49, Jamie wrote:
> Can we give this a more descriptive name, if it needs a name at all?

Done.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:75: <img height="64" width="64"
src="chromoting128.png">
On 2011/05/31 18:11:49, Jamie wrote:
> Do we have a 64x64 version of this icon? If not, we ought to get one rather
than
> scaling down the 128x128 one.

Why? the 128x128 looks fine since it's en even div by 2.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:99: style="width:173px">
On 2011/05/31 18:11:49, Jamie wrote:
> Why specify the width explicitly?

Thought I needed to. Turns out I don't, but there is a bug in Chrome. Bug
logged, commented, and hack added.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:101: </button>
On 2011/05/31 18:11:49, Jamie wrote:
> Can we disable this button if the OAuth2 token isn't present? Or maybe show an
> error if the user clicks it in this case.

Done.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:108: </div> <!-- preparing_to_share -->
On 2011/05/31 18:11:49, Jamie wrote:
> This is never shown at present, correct? As long as this is on the TODO list,
I
> don't have a problem with that, but at the moment, clicking 'Share this
> computer' twice crashes the plugin.

It is shown... but we should fix it up so it shows faster. Will do in a later
CL.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:127: </div> <!-- ready_to_share -->
On 2011/05/31 18:11:49, Jamie wrote:
> This div was never shown in my tests, although the code looks fine. I'm
guessing
> there's a bug in the plugin but since it's not related to this CL, let's move
> on.

Yes.. needs to be fixed.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:145: &lsquo;Share this computer&rsquo; then
read to you their security
On 2011/05/31 18:11:49, Jamie wrote:
> s/security/access/

Done.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:153: <button onclick="tryConnect();"
style="width: 80px">
On 2011/05/31 18:11:49, Jamie wrote:
> Get rid of width?

see above

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:155: </button>
On 2011/05/31 18:11:49, Jamie wrote:
> Can we disable this if either the Oauth2 token or email address aren't set? Or
> maybe show an error if the user tries to click it in this case.

Done.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:186: style="width: 80px">
On 2011/05/31 18:11:49, Jamie wrote:
> Get rid of width?

see above

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:200: </a>.
On 2011/05/31 18:11:49, Jamie wrote:
> Move the closing tag to the preceding line, otherwise there's a space before
the
> full stop.

Done.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:208: </a>.
On 2011/05/31 18:11:49, Jamie wrote:
> Move the closing tag to the preceding line, otherwise there's a space before
the
> full stop.

Done.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/remot...
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/remot...
remoting/webapp/me2mom/remoting.js:366: function tryConnect() {
On 2011/05/31 18:11:49, Jamie wrote:
> Why not pass the access code in as before? That way, the place where you need
to
> know the id is right next to the id itself and the two are easier to keep in
> sync.

Because currently it doesn't work with the "form" hack to get buttons centered.

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/remot...
File remoting/webapp/me2mom/remoting_session.html (left):

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/remot...
remoting/webapp/me2mom/remoting_session.html:17: <span id="status_msg"
class="status_msg">Initializing...</span>
On 2011/05/31 18:11:49, Jamie wrote:
> It's not clear to me what the purpose of the changes in this file are.

comments added in CSS files.

[Jamie, 2011-06-02 00:12:29]

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
File remoting/webapp/me2mom/choice.html (right):

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:27: onchange="handleOAuth2CodeChange(this);"
id="oauth2_code" />
On 2011/06/01 23:01:58, dmac wrote:
> On 2011/05/31 18:11:49, Jamie wrote:
> > Why trap both onkeyup and onchange, and why pass 'this' only to one of them.
I
> > think the function should be declared as taking a 'form' argument as it
gives
> a
> > more convenient syntax for accessing the fields (form['field-name']).
> 
> 
> I need onkeyup to handle typing, and onchange to handle paste. Passing this
was
> an error.

I think that passing 'this' is the way to go. It means that you don't need to
know the form's id in the Javascript because it's passed as a parameter.

Also, did you miss my comment about moving the id attribute, or did you
disagree?

http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.html:75: <img height="64" width="64"
src="chromoting128.png">
On 2011/06/01 23:01:58, dmac wrote:
> On 2011/05/31 18:11:49, Jamie wrote:
> > Do we have a 64x64 version of this icon? If not, we ought to get one rather
> than
> > scaling down the 128x128 one.
> 
> Why? the 128x128 looks fine since it's en even div by 2.

I thought it looked a bit more blurry than it might. Not a big deal though.

http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/choi...
remoting/webapp/me2mom/choice.html:60: <input type="hidden"
name="xmpp_captcha_token" />
Didn't we decide not to use Chromoting in any user-facing strings?

http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/choi...
remoting/webapp/me2mom/choice.html:176: <p class="message"
id="invalid_access_code">
I had originally used different anchor names for the to links so that the
"visited" colour didn't show up when it shouldn't. I don't see any CSS for
switch-mode; perhaps we should use it to make the visited colour the same as the
normal one?

http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/remo...
File remoting/webapp/me2mom/remoting_session.js (right):

http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/remo...
remoting/webapp/me2mom/remoting_session.js:32: window.addEventListener("load",
init_, false);
Is this different to setting the onload attribute in the <body> tag?

[Jamie, 2011-06-02 00:55:32]

http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/choi...
File remoting/webapp/me2mom/choice.html (right):

http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/choi...
remoting/webapp/me2mom/choice.html:57: Chromoting ˃ Share
The greater-than symbol here (and below) is small and superscript for some
reason and looks rather odd. The original patch in this CL has it as a regular
greater-than, so I'm guessing it wasn't deliberate.

[dmac, 2011-06-02 20:25:06]

On 2011/06/02 00:12:29, Jamie wrote:
>
http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
> File remoting/webapp/me2mom/choice.html (right):
> 
>
http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
> remoting/webapp/me2mom/choice.html:27:
onchange="handleOAuth2CodeChange(this);"
> id="oauth2_code" />
> On 2011/06/01 23:01:58, dmac wrote:
> > On 2011/05/31 18:11:49, Jamie wrote:
> > > Why trap both onkeyup and onchange, and why pass 'this' only to one of
them.
> I
> > > think the function should be declared as taking a 'form' argument as it
> gives
> > a
> > > more convenient syntax for accessing the fields (form['field-name']).
> > 
> > 
> > I need onkeyup to handle typing, and onchange to handle paste. Passing this
> was
> > an error.
> 
> I think that passing 'this' is the way to go. It means that you don't need to
> know the form's id in the Javascript because it's passed as a parameter.
> 
> Also, did you miss my comment about moving the id attribute, or did you
> disagree?
> 

I think I got all the ids, and I'm not passing this to make all the routines
somewhat consistent.
>
http://codereview.chromium.org/7078022/diff/8001/remoting/webapp/me2mom/choic...
> remoting/webapp/me2mom/choice.html:75: <img height="64" width="64"
> src="chromoting128.png">
> On 2011/06/01 23:01:58, dmac wrote:
> > On 2011/05/31 18:11:49, Jamie wrote:
> > > Do we have a 64x64 version of this icon? If not, we ought to get one
rather
> > than
> > > scaling down the 128x128 one.
> > 
> > Why? the 128x128 looks fine since it's en even div by 2.
> 
> I thought it looked a bit more blurry than it might. Not a big deal though.
> 
>
http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/choi...
> remoting/webapp/me2mom/choice.html:60: <input type="hidden"
> name="xmpp_captcha_token" />
> Didn't we decide not to use Chromoting in any user-facing strings?

Right now I'm using "Chromoting" for anything user-facing so that we can find it
if/when we change it in the future to whatever we are going to be using for the
official product name.

Remoting still sticks for our code though.

> 
>
http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/choi...
> remoting/webapp/me2mom/choice.html:176: <p class="message"
> id="invalid_access_code">
> I had originally used different anchor names for the to links so that the
> "visited" colour didn't show up when it shouldn't. I don't see any CSS for
> switch-mode; perhaps we should use it to make the visited colour the same as
the
> normal one?

The links don't actually change color for me.

> 
>
http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/remo...
> File remoting/webapp/me2mom/remoting_session.js (right):
> 
>
http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/remo...
> remoting/webapp/me2mom/remoting_session.js:32: window.addEventListener("load",
> init_, false);
> Is this different to setting the onload attribute in the <body> tag?

I'm just trying to move as much of the scripting out of the HTML as possible.

[dmac, 2011-06-02 20:26:51]

On 2011/06/02 00:55:32, Jamie wrote:
>
http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/choi...
> File remoting/webapp/me2mom/choice.html (right):
> 
>
http://codereview.chromium.org/7078022/diff/14004/remoting/webapp/me2mom/choi...
> remoting/webapp/me2mom/choice.html:57: Chromoting ˃ Share
> The greater-than symbol here (and below) is small and superscript for some
> reason and looks rather odd. The original patch in this CL has it as a regular
> greater-than, so I'm guessing it wasn't deliberate.

Sigh... unfortunate that Linux has bad fonts. Done.

[Jamie, 2011-06-02 20:42:59]

LGTM