Remove 'Enable' button on unsupported platforms. Added 'No hosts' message.

Remove 'Enable' button on unsupported platforms. Added 'No hosts' message.

BUG=131403
TEST=Manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=145045

[Jamie, 2012-06-28 21:38:39]

ptal.

The "no hosts registered" message includes instructions on how to set up a host,
including a reference to another string in messages.json. Rather than assuming
that that string will be translated identically in both places, I added some
code to allow indirect substitutions to be marked up in HTML. This CL could be
simplified a lot if we wanted rephrase things to avoid the explicit
instructions.

[Sergey Ulanov, 2012-06-28 21:57:56]

couple drive-by nits.

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/_local...
File remoting/webapp/_locales/en/messages.json (right):

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/_local...
remoting/webapp/_locales/en/messages.json:222: "message": "You have no computers
registered. Before you can use Chromoting to connect to a computer, you must
first register it by installing this web-app and clicking
$boldBegin$$buttonName$$boldEnd$.",
Users would read that message when the webapp is already installed, so the
message can be confusing.

You have no computers registered. You can add a computer here by clicking
$boldBegin$$buttonName$$boldEnd$ on the computer you need to access.

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/_local...
remoting/webapp/_locales/en/messages.json:226: "content": "$1",
might be better to use $1 for the message itself while $2 and $3 for <b>. Why
can't <b> be part of the resource itself?

[Jamie, 2012-06-28 22:07:12]

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/_local...
File remoting/webapp/_locales/en/messages.json (right):

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/_local...
remoting/webapp/_locales/en/messages.json:222: "message": "You have no computers
registered. Before you can use Chromoting to connect to a computer, you must
first register it by installing this web-app and clicking
$boldBegin$$buttonName$$boldEnd$.",
On 2012/06/28 21:57:56, sergeyu wrote:
> Users would read that message when the webapp is already installed, so the
> message can be confusing.
> 
> You have no computers registered. You can add a computer here by clicking
> $boldBegin$$buttonName$$boldEnd$ on the computer you need to access.

I think that's an improvement, but it doesn't tell them where to find that
button (on Chrome OS there is no such button on the local app, so it's
non-obvious). How about:

You have no computers registered. You can add a computer here by installing
Chromoting on it and clicking $boldBegin$$buttonName$$boldEnd$.

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/_local...
remoting/webapp/_locales/en/messages.json:226: "content": "$1",
On 2012/06/28 21:57:56, sergeyu wrote:
> might be better to use $1 for the message itself while $2 and $3 for <b>. Why
> can't <b> be part of the resource itself?

If the HTML tags are part of the resource then it's possible for translators to
change them. I've used this approach elsewhere to preserve markup in l10n. I
think I'd prefer to keep the placeholders in the correct order.

[Wez, 2012-06-28 23:00:20]

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/_local...
File remoting/webapp/_locales/en/messages.json (right):

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/_local...
remoting/webapp/_locales/en/messages.json:222: "message": "You have no computers
registered. Before you can use Chromoting to connect to a computer, you must
first register it by installing this web-app and clicking
$boldBegin$$buttonName$$boldEnd$.",
On 2012/06/28 22:07:12, Jamie wrote:
> On 2012/06/28 21:57:56, sergeyu wrote:
> > Users would read that message when the webapp is already installed, so the
> > message can be confusing.
> > 
> > You have no computers registered. You can add a computer here by clicking
> > $boldBegin$$buttonName$$boldEnd$ on the computer you need to access.
> 
> I think that's an improvement, but it doesn't tell them where to find that
> button (on Chrome OS there is no such button on the local app, so it's
> non-obvious). How about:
> 
> You have no computers registered. You can add a computer here by installing
> Chromoting on it and clicking $boldBegin$$buttonName$$boldEnd$.

SGTM, although you need to brandify Chromoting. :)

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/host_l...
File remoting/webapp/host_list.js (right):

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/host_l...
remoting/webapp/host_list.js:27: remoting.HostList = function(table, noHosts,
errorMsg, errorButton) {
nit: table, noHost, errorMsg -> hostListDiv, noHostDiv, errorDiv?

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/host_l...
remoting/webapp/host_list.js:192: var noHosts = (this.hosts_.length == 0);
nit: noHosts -> noHostsRegistered, otherwise it's a little confusing that it's a
bool while noHosts_ is a div, although renaming noHosts_ to noHostsDiv_ would
help too.

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/l10n.js
File remoting/webapp/l10n.js (right):

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/l10n.j...
remoting/webapp/l10n.js:68: substitutions = [];
Why do you do this? Can't you initialize substitutions to [] initially and treat
that as nothing-to-do?

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/verify...
File remoting/webapp/verify-webapp.py (right):

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/verify...
remoting/webapp/verify-webapp.py:16: or ...i18n-value-name-1="BUTTON_NAME"...
This comment doesn't mention the pre-existing i18n-value-N form?

[Jamie, 2012-06-28 23:28:57]

Branding is done when the strings are sent for l10n; "Chromoting" is the correct
form for this file.

Since I've changed the logic that determines whether to set innerText vs.
innerHTML, ptal again to make sure I haven't messed it up.

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/host_l...
File remoting/webapp/host_list.js (right):

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/host_l...
remoting/webapp/host_list.js:27: remoting.HostList = function(table, noHosts,
errorMsg, errorButton) {
On 2012/06/28 23:00:20, Wez wrote:
> nit: table, noHost, errorMsg -> hostListDiv, noHostDiv, errorDiv?

It's a fair point, but if I'm we're going try to get this merged, I'd prefer to
touch as little code as possible, so I'll follow up with something that cleans
up the variable names.

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/host_l...
remoting/webapp/host_list.js:192: var noHosts = (this.hosts_.length == 0);
On 2012/06/28 23:00:20, Wez wrote:
> nit: noHosts -> noHostsRegistered, otherwise it's a little confusing that it's
a
> bool while noHosts_ is a div, although renaming noHosts_ to noHostsDiv_ would
> help too.

Done.

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/l10n.js
File remoting/webapp/l10n.js (right):

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/l10n.j...
remoting/webapp/l10n.js:68: substitutions = [];
On 2012/06/28 23:00:20, Wez wrote:
> Why do you do this? Can't you initialize substitutions to [] initially and
treat
> that as nothing-to-do?

Done.

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/verify...
File remoting/webapp/verify-webapp.py (right):

https://chromiumcodereview.appspot.com/10703034/diff/1/remoting/webapp/verify...
remoting/webapp/verify-webapp.py:16: or ...i18n-value-name-1="BUTTON_NAME"...
On 2012/06/28 23:00:20, Wez wrote:
> This comment doesn't mention the pre-existing i18n-value-N form?

i18n-value-N is used verbatim so there's nothing for this script to check. I
needed to add a check for the named version because it's another place that
string definitions and references could get out-of-sync.

[Wez, 2012-06-29 00:11:49]

lgtm

https://chromiumcodereview.appspot.com/10703034/diff/6002/remoting/webapp/l10...
File remoting/webapp/l10n.js (right):

https://chromiumcodereview.appspot.com/10703034/diff/6002/remoting/webapp/l10...
remoting/webapp/l10n.js:78: break;
nit: You log an error if there's a missing translation but not if there's a
missing substitution?

[Jamie, 2012-06-29 00:21:40]

https://chromiumcodereview.appspot.com/10703034/diff/6002/remoting/webapp/l10...
File remoting/webapp/l10n.js (right):

https://chromiumcodereview.appspot.com/10703034/diff/6002/remoting/webapp/l10...
remoting/webapp/l10n.js:78: break;
On 2012/06/29 00:11:49, Wez wrote:
> nit: You log an error if there's a missing translation but not if there's a
> missing substitution?

To detect a missing substitution you'd have to do a dummy translation,
substituting something like $1..$9 (or, better, something more distinctive to
avoid false positives) for the placeholders, then check for each $n that there's
an i18n-value-[name-]n attribute. We could do that, but I'm not sure it's worth
it.

[Wez, 2012-06-29 00:26:42]

On 2012/06/29 00:21:40, Jamie wrote:
>
https://chromiumcodereview.appspot.com/10703034/diff/6002/remoting/webapp/l10...
> File remoting/webapp/l10n.js (right):
> 
>
https://chromiumcodereview.appspot.com/10703034/diff/6002/remoting/webapp/l10...
> remoting/webapp/l10n.js:78: break;
> On 2012/06/29 00:11:49, Wez wrote:
> > nit: You log an error if there's a missing translation but not if there's a
> > missing substitution?
> 
> To detect a missing substitution you'd have to do a dummy translation,
> substituting something like $1..$9 (or, better, something more distinctive to
> avoid false positives) for the placeholders, then check for each $n that
there's
> an i18n-value-[name-]n attribute. We could do that, but I'm not sure it's
worth
> it.

Oh, I see what you mean; makes sense.

[Jamie, 2012-06-29 20:08:55]

Updated to match our recent discussions.

[Wez, 2012-06-29 21:40:51]

lgtm

https://chromiumcodereview.appspot.com/10703034/diff/2005/remoting/webapp/hos...
File remoting/webapp/host_controller.js (right):

https://chromiumcodereview.appspot.com/10703034/diff/2005/remoting/webapp/hos...
remoting/webapp/host_controller.js:267: if (configStr != '{}') {
Under what circumstances do we see empty host configs?

[Jamie, 2012-06-29 21:56:27]

https://chromiumcodereview.appspot.com/10703034/diff/2005/remoting/webapp/hos...
File remoting/webapp/host_controller.js (right):

https://chromiumcodereview.appspot.com/10703034/diff/2005/remoting/webapp/hos...
remoting/webapp/host_controller.js:267: if (configStr != '{}') {
On 2012/06/29 21:40:51, Wez wrote:
> Under what circumstances do we see empty host configs?

If there's no host registered you get this.

[Jamie, 2012-06-29 22:52:09]

One more time please, now that we've had a proper UX person look at it.

[Wez, 2012-06-30 00:51:34]

lgtm