[Chromoting] Show any startup errors in the LoadingWindow.

[Chromoting] Show any startup errors in the LoadingWindow.

Show any connection errors during startup in the LoadingWindow rather
than creating a new error window for the error.

This cl also adds a new updateErrorMessage method to the MessageWindow
to update the message and reset the message window as appropriate for
an error message (disabling the spinner, updating the button label).

BUG=

Committed: https://crrev.com/d44e8a774ce99a89c350d10d5b3f9f644eb0c3d1
Cr-Commit-Position: refs/heads/master@{#330022}

[garykac, 2015-05-09 00:55:02]

garykac@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2015-05-09 01:17:00]

https://codereview.chromium.org/1139543002/diff/1/remoting/webapp/base/js/mes...
File remoting/webapp/base/js/message_window.js (right):

https://codereview.chromium.org/1139543002/diff/1/remoting/webapp/base/js/mes...
remoting/webapp/base/js/message_window.js:152: case 'update_message':
How about renaming this message 'update' and having it take the same parameters
as show, although with all of them optional to indicate "no change"? It would
mean that a lot of the code could be shared with the 'show' message, and any
parameter could be changed on the fly, rather than these select few.

[garykac, 2015-05-11 21:34:16]

https://codereview.chromium.org/1139543002/diff/1/remoting/webapp/base/js/mes...
File remoting/webapp/base/js/message_window.js (right):

https://codereview.chromium.org/1139543002/diff/1/remoting/webapp/base/js/mes...
remoting/webapp/base/js/message_window.js:152: case 'update_message':
On 2015/05/09 01:17:00, Jamie wrote:
> How about renaming this message 'update' and having it take the same
parameters
> as show, although with all of them optional to indicate "no change"? It would
> mean that a lot of the code could be shared with the 'show' message, and any
> parameter could be changed on the fly, rather than these select few.

Done.

[Jamie, 2015-05-11 22:54:53]

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/message_window.js (right):

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:86: // all optional for the 'update'
command.
I think we should do the type check regardless. For 'show', you should
additionally check that they're not undefined. It would also make the
conditionals later on clearer, ie:

  if (title !== undefined) {...}

instead of

  if (typeof(title) === 'string') {...}

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:133: // Set up clickhandlers for the
buttons.
s/clickhandlers/click-handlers/

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:137: 'click',
this.sendReply_.bind(this, event.source, messageId, 0), false);
Optional: Consider moving the event handlers to the ctor, which is more
consistent with what we do with other classes.

[garykac, 2015-05-13 22:05:27]

ptal

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/message_window.js (right):

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:86: // all optional for the 'update'
command.
On 2015/05/11 22:54:53, Jamie wrote:
> I think we should do the type check regardless. For 'show', you should
> additionally check that they're not undefined. It would also make the
> conditionals later on clearer, ie:
> 
>   if (title !== undefined) {...}
> 
> instead of
> 
>   if (typeof(title) === 'string') {...}

The same needs to be done for cancelButtonLabel (which is an optional argument
for 'show').

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:133: // Set up clickhandlers for the
buttons.
On 2015/05/11 22:54:53, Jamie wrote:
> s/clickhandlers/click-handlers/

Done.

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:137: 'click',
this.sendReply_.bind(this, event.source, messageId, 0), false);
On 2015/05/11 22:54:53, Jamie wrote:
> Optional: Consider moving the event handlers to the ctor, which is more
> consistent with what we do with other classes.

That doesn't feel like it belongs in this cl.

It might change the behavior of when message (like close window) are sent.
Currently, we'll only send close messages for windows that have been shown.

[Jamie, 2015-05-13 23:43:18]

LGTM with nits.

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/message_window.js (right):

https://codereview.chromium.org/1139543002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:137: 'click',
this.sendReply_.bind(this, event.source, messageId, 0), false);
On 2015/05/13 22:05:27, garykac wrote:
> On 2015/05/11 22:54:53, Jamie wrote:
> > Optional: Consider moving the event handlers to the ctor, which is more
> > consistent with what we do with other classes.
> 
> That doesn't feel like it belongs in this cl.
> 
> It might change the behavior of when message (like close window) are sent.
> Currently, we'll only send close messages for windows that have been shown.

Acknowledged.

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
File remoting/webapp/base/js/message_window.js (right):

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:106: if (isString(title)) {
I think (title !== undefined) would be a more natural way of expressing this
(and save calling isString a second time).

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
File remoting/webapp/base/js/typecheck.js (right):

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/typecheck.js:22: * @param {boolean=} allowUndefined True
to allow undefined as a valid boolean.
Optional parameters should be prefixed with opt_

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/typecheck.js:26: if (allowUndefined && typeof value ===
'undefined') {
No need for typeof here; "value === undefined" is equivalent and verifiable by
the compiler.

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/typecheck.js:34: * @param {boolean=} allowUndefined True
to allow undefined as a valid number.
You haven't added allowUndefined to the parameter list, so I'm surprised this
compiles.

[garykac, 2015-05-15 01:29:36]

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
File remoting/webapp/base/js/message_window.js (right):

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:106: if (isString(title)) {
On 2015/05/13 23:43:17, Jamie wrote:
> I think (title !== undefined) would be a more natural way of expressing this
> (and save calling isString a second time).

Done.

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
File remoting/webapp/base/js/typecheck.js (right):

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/typecheck.js:22: * @param {boolean=} allowUndefined True
to allow undefined as a valid boolean.
On 2015/05/13 23:43:18, Jamie wrote:
> Optional parameters should be prefixed with opt_

Done.

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/typecheck.js:26: if (allowUndefined && typeof value ===
'undefined') {
On 2015/05/13 23:43:18, Jamie wrote:
> No need for typeof here; "value === undefined" is equivalent and verifiable by
> the compiler.

Done.

https://codereview.chromium.org/1139543002/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/typecheck.js:34: * @param {boolean=} allowUndefined True
to allow undefined as a valid number.
On 2015/05/13 23:43:18, Jamie wrote:
> You haven't added allowUndefined to the parameter list, so I'm surprised this
> compiles.

Fixed.

[garykac, 2015-05-15 01:29:42]

The CQ bit was checked by garykac@chromium.org

[garykac, 2015-05-15 01:29:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1139543002/#ps80001
(title: "Address review comments")

[commit-bot: I haz the power, 2015-05-15 01:30:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139543002/80001

[commit-bot: I haz the power, 2015-05-15 02:14:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-15 02:15:09]

Patchset 5 (id:??) landed as
https://crrev.com/d44e8a774ce99a89c350d10d5b3f9f644eb0c3d1
Cr-Commit-Position: refs/heads/master@{#330022}

[Dan Beam, 2015-05-15 06:15:01]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-05-15 06:15:02]

https://codereview.chromium.org/1139543002/diff/80001/remoting/webapp/base/js...
File remoting/webapp/base/js/message_window.js (right):

https://codereview.chromium.org/1139543002/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/message_window.js:96: isBoolean(showSpinner,
optionalFieldUpdate) {
you're missing a closing ) here
http://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Lin...

[Dan Beam, 2015-05-15 06:20:20]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1144593002/ by dbeam@chromium.org.

The reason for reverting is: Syntax error in the JS, breaking the closure bot
and almost surely doesn't work well/breaking something in remoting.
http://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Lin....

[garykac, 2015-05-15 21:29:23]

On 2015/05/15 06:20:20, Dan Beam wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/1144593002/ by mailto:dbeam@chromium.org.
> 
> The reason for reverting is: Syntax error in the JS, breaking the closure bot
> and almost surely doesn't work well/breaking something in remoting.
>
http://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Lin....

I was running JS compile over AppRemoting instead of DesktopRemoting, so I
wasn't actually running it over the correct code :-(