Converted remoting.Error from an enum to a class

Converted remoting.Error to be a class rather than just a string.

Committed: https://crrev.com/3cc9f0ac660fe93634f56986681aee6ae6b38988
Cr-Commit-Position: refs/heads/master@{#319824}

[John Williams, 2015-02-26 04:47:10]

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

[John Williams, 2015-02-26 04:47:58]

On 2015/02/26 04:47:18, John Williams wrote:

Context for this change:

This is needed to support code that uses promises, because otherwise it's
there's not good way to distinguish instances of remoting.Error from other
strings that could be thrown.

This change also opens up the possibility of easily extending errors to include
more detailed information.

It's also possible to extend this class to report useful stack traces when uses
for a rejected promise, but I'm holding back on that change to see if I can get
V8 fixed so we can get stack traces without doing anything special.

[Jamie, 2015-02-26 18:57:43]

Please update the title of this CL, since it's not accurate right now.
Otherwise, this mostly looks good.

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/app_remo...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/app_remo...
remoting/webapp/app_remoting/js/app_remoting.js:194: /** @param
{!remoting.Error} error */
We haven't tended to be very rigourous about using ! in our types elsewhere. Do
you think there's much value in it?

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/app_remo...
remoting/webapp/app_remoting/js/app_remoting.js:201:
remoting.identity.callWithToken(getAppHost, onError);
Hasn't this API been Promisified now?

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:298: console.error('Connection
failed: %o', error);
IMO, the %o syntax is less clear than using "+". Is there any reason to prefer
it?

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/e...
File remoting/webapp/crd/js/error.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/e...
remoting/webapp/crd/js/error.js:133: * @param {number} httpStatus An HTTP status
tag.
Most of the changes in this block seem to global search/replace errors.

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_list.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_list.js:195: onDone(this.lastError_.tag ==
remoting.Error.Tag.NONE);
Not strictly related to your changes, but since neither sortHosts_ nor save_ can
set lastError_, you might as well just pass true here.

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:217: remoting.Error.NOT_AUTHENTICATED);
No need for a new line here.

[kelvinp, 2015-02-27 00:58:06]

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

[kelvinp, 2015-02-27 00:58:07]

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/e...
File remoting/webapp/crd/js/error.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/e...
remoting/webapp/crd/js/error.js:5: 'use strict';
consider wrapping this file in a IEFE

/** @suppress {duplicate} */
var remoting = remoting || {};	   8 var remoting = remoting || {};

(function() {

'use strict'

// Content

})();

[John Williams, 2015-02-27 21:42:11]

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/app_remo...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/app_remo...
remoting/webapp/app_remoting/js/app_remoting.js:194: /** @param
{!remoting.Error} error */
On 2015/02/26 18:57:42, Jamie wrote:
> We haven't tended to be very rigourous about using ! in our types elsewhere.
Do
> you think there's much value in it?

I'm a fan.  It catches some errors and I've never felt it caused a lot of extra
work (like "const poisoning" does).  And on the off chance you haven't
encountered it already, there's the esteemed C.A.R. Hoare:

I call it my billion-dollar mistake. It was the invention of the null reference
in 1965. At that time, I was designing the first comprehensive type system for
references in an object oriented language (ALGOL W). My goal was to ensure that
all use of references should be absolutely safe, with checking performed
automatically by the compiler. But I couldn't resist the temptation to put in a
null reference, simply because it was so easy to implement. This has led to
innumerable errors, vulnerabilities, and system crashes, which have probably
caused a billion dollars of pain and damage in the last forty years. In recent
years, a number of program analysers like PREfix and PREfast in Microsoft have
been used to check references, and give warnings if there is a risk they may be
non-null. More recent programming languages like Spec# have introduced
declarations for non-null references. This is the solution, which I rejected in
1965.

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/app_remo...
remoting/webapp/app_remoting/js/app_remoting.js:201:
remoting.identity.callWithToken(getAppHost, onError);
On 2015/02/26 18:57:42, Jamie wrote:
> Hasn't this API been Promisified now?

I started this branch before that change landed.  Fixed now.

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:298: console.error('Connection
failed: %o', error);
On 2015/02/26 18:57:42, Jamie wrote:
> IMO, the %o syntax is less clear than using "+". Is there any reason to prefer
> it?

If `error` is an object, using %o lets you browse its properties in the log
view; using + just prints something useless like "[object Object]".

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/e...
File remoting/webapp/crd/js/error.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/e...
remoting/webapp/crd/js/error.js:5: 'use strict';
On 2015/02/27 00:58:07, kelvinp wrote:
> consider wrapping this file in a IEFE
> 
> /** @suppress {duplicate} */
> var remoting = remoting || {};	   8 var remoting = remoting || {};
> 
> (function() {
> 
> 'use strict'
> 
> // Content
> 
> })();

Adding this wrapper caused the compiler to act as if remoting.Error.Tag was
never defined.

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/e...
remoting/webapp/crd/js/error.js:133: * @param {number} httpStatus An HTTP status
tag.
On 2015/02/26 18:57:42, Jamie wrote:
> Most of the changes in this block seem to global search/replace errors.

Fixed.

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_list.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_list.js:195: onDone(this.lastError_.tag ==
remoting.Error.Tag.NONE);
On 2015/02/26 18:57:42, Jamie wrote:
> Not strictly related to your changes, but since neither sortHosts_ nor save_
can
> set lastError_, you might as well just pass true here.

Done.

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/identity.js:217: remoting.Error.NOT_AUTHENTICATED);
On 2015/02/26 18:57:42, Jamie wrote:
> No need for a new line here.

Oops, relic of a previous version of the CL.

[Jamie, 2015-03-02 18:28:10]

LGTM once my comments are addressed.

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:298: console.error('Connection
failed: %o', error);
On 2015/02/27 21:42:11, John Williams wrote:
> On 2015/02/26 18:57:42, Jamie wrote:
> > IMO, the %o syntax is less clear than using "+". Is there any reason to
prefer
> > it?
> 
> If `error` is an object, using %o lets you browse its properties in the log
> view; using + just prints something useless like "[object Object]".

In that case, you can achieve the same effect with multiple parameters:

  console.log('Connection failed:', error)

Note no explicit space to separate them. This is the syntax we use elsewhere,
and I think it's clearer than %o.

However, in this case, I think it would be better to log the error tag
explicitly. We often ask users to copy/paste console logs, in which case we'll
lose details of the error messages with this change.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_screen.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_screen.js:99: remoting.Error.MISSING_PLUGIN);
No need for a new line here.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/e...
File remoting/webapp/crd/js/error.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/e...
remoting/webapp/crd/js/error.js:151: 
No need for a blank line here.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/e...
remoting/webapp/crd/js/error.js:154: // TODO(jamiewalch): Tag = UNEXPECTED here
and let calling tag treat that
Search/replace error?

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:50: remoting.Error.CANCELLED);
No new line.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_screen.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_screen.js:197: console.error('Sharing error: %o',
error);
Don't use %o here, either.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/it2me_helpee_channel.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:266: }).catch(function(/** !Error
*/ reason) {
Missing @type?

Also, I thought we had problems with 'catch' as a function name? Is that fixed
in the new jscompile?

[John Williams, 2015-03-02 22:01:38]

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/955283002/diff/40001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:298: console.error('Connection
failed: %o', error);
On 2015/03/02 18:28:09, Jamie wrote:
> On 2015/02/27 21:42:11, John Williams wrote:
> > On 2015/02/26 18:57:42, Jamie wrote:
> > > IMO, the %o syntax is less clear than using "+". Is there any reason to
> prefer
> > > it?
> > 
> > If `error` is an object, using %o lets you browse its properties in the log
> > view; using + just prints something useless like "[object Object]".
> 
> In that case, you can achieve the same effect with multiple parameters:
> 
>   console.log('Connection failed:', error)
> 
> Note no explicit space to separate them. This is the syntax we use elsewhere,
> and I think it's clearer than %o.
> 
> However, in this case, I think it would be better to log the error tag
> explicitly. We often ask users to copy/paste console logs, in which case we'll
> lose details of the error messages with this change.

Done.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_screen.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_screen.js:99: remoting.Error.MISSING_PLUGIN);
On 2015/03/02 18:28:09, Jamie wrote:
> No need for a new line here.

Done.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/e...
File remoting/webapp/crd/js/error.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/e...
remoting/webapp/crd/js/error.js:151: 
On 2015/03/02 18:28:09, Jamie wrote:
> No need for a blank line here.

Done.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/e...
remoting/webapp/crd/js/error.js:154: // TODO(jamiewalch): Tag = UNEXPECTED here
and let calling tag treat that
On 2015/03/02 18:28:09, Jamie wrote:
> Search/replace error?

Done.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/hangout_consent_dialog.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/hangout_consent_dialog.js:50: remoting.Error.CANCELLED);
On 2015/03/02 18:28:09, Jamie wrote:
> No new line.

Done.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_screen.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_screen.js:197: console.error('Sharing error: %o',
error);
On 2015/03/02 18:28:10, Jamie wrote:
> Don't use %o here, either.

Done.

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/i...
File remoting/webapp/crd/js/it2me_helpee_channel.js (right):

https://codereview.chromium.org/955283002/diff/80001/remoting/webapp/crd/js/i...
remoting/webapp/crd/js/it2me_helpee_channel.js:266: }).catch(function(/** !Error
*/ reason) {
On 2015/03/02 18:28:10, Jamie wrote:
> Missing @type?
> 
> Also, I thought we had problems with 'catch' as a function name? Is that fixed
> in the new jscompile?

Yeah, it's fixed now, and the abbreviates type syntax works here because it's a
function.  It doesn't work in real "catch" clauses, though.

[John Williams, 2015-03-02 22:01:51]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-02 22:01:51]

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/955283002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-03-02 22:03:11]

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

[commit-bot: I haz the power, 2015-03-02 22:07:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-02 22:07:50]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[John Williams, 2015-03-03 00:27:14]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-03 00:27:15]

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/955283002/#ps120001
(title: "rebase-update")

[commit-bot: I haz the power, 2015-03-03 00:28:07]

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

[commit-bot: I haz the power, 2015-03-03 00:42:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-03 00:42:10]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[John Williams, 2015-03-03 00:44:30]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-03-03 00:45:19]

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

[commit-bot: I haz the power, 2015-03-03 01:05:14]

The CQ bit was unchecked by commit-bot@chromium.org

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

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[John Williams, 2015-03-06 19:47:21]

The CQ bit was checked by jrw@chromium.org

[commit-bot: I haz the power, 2015-03-06 19:48:58]

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

[commit-bot: I haz the power, 2015-03-06 19:54:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-06 19:54:10]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Jamie, 2015-03-09 23:53:45]

https://codereview.chromium.org/955283002/diff/140001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/error.js (right):

https://codereview.chromium.org/955283002/diff/140001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:70: // remoting.Error constructor directly
Apologies for adding a comment so late, but it occurs to me that this is
dangerous because we test for error equality in some places. The right way to do
that is now to check the tag, but some places may still say, for example:

  if (error == remoting.Error.CANCELLED) { }

With the place-holders, that will compile, but may give false negatives if the
source of error used the ctor rather than the place-holder. It would be better
just to bite the bullet and update all call-sites to use the ctor. It can be a
follow-up CL if you prefer.

[John Williams, 2015-03-09 23:57:29]

On 2015/03/09 23:53:45, Jamie wrote:
>
https://codereview.chromium.org/955283002/diff/140001/remoting/webapp/crd/js/...
> File remoting/webapp/crd/js/error.js (right):
> 
>
https://codereview.chromium.org/955283002/diff/140001/remoting/webapp/crd/js/...
> remoting/webapp/crd/js/error.js:70: // remoting.Error constructor directly
> Apologies for adding a comment so late, but it occurs to me that this is
> dangerous because we test for error equality in some places. The right way to
do
> that is now to check the tag, but some places may still say, for example:
> 
>   if (error == remoting.Error.CANCELLED) { }
> 
> With the place-holders, that will compile, but may give false negatives if the
> source of error used the ctor rather than the place-holder. It would be better
> just to bite the bullet and update all call-sites to use the ctor. It can be a
> follow-up CL if you prefer.

I'm fine with doing that, but definitely in another CL.

[John Williams, 2015-03-09 23:57:36]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-09 23:57:36]

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/955283002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-03-09 23:58:59]

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

[commit-bot: I haz the power, 2015-03-10 00:04:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-10 00:05:00]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[John Williams, 2015-03-10 00:14:27]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-10 00:14:28]

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/955283002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-03-10 00:15:35]

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

[commit-bot: I haz the power, 2015-03-10 02:31:21]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-03-10 02:32:25]

Patchset 10 (id:??) landed as
https://crrev.com/3cc9f0ac660fe93634f56986681aee6ae6b38988
Cr-Commit-Position: refs/heads/master@{#319824}