Eliminated named constants for instances of remoting.Error.

Eliminated named constants for instances of remoting.Error.

Committed: https://crrev.com/745e53b571adbe742caf3b92cc8c692eb1f51b6e
Cr-Commit-Position: refs/heads/master@{#320795}

[John Williams, 2015-03-11 21:33:21]

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

[Jamie, 2015-03-11 22:16:38]

Thanks for the cleanup!

LGTM with my comments addressed, but please make sure you've run it through the
compiler; the problems I saw seems like they should be caught by the compiler,
so it's worrying if they weren't.

https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
File remoting/webapp/crd/js/error.js (right):

https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
remoting/webapp/crd/js/error.js:98: return new remoting.Error.none();
Don't need new here or for unexpected() (in fact, I'm surprised this compiles).

https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
File remoting/webapp/crd/js/server_log_entry.js (right):

https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
remoting/webapp/crd/js/server_log_entry.js:234: if
(connectionError.tag.isError()) {
isError() is a method on Error, not Tag?

[John Williams, 2015-03-12 02:46:20]

PTAL; I added a hasTag method for the sake of type safety, and some unit tests
since there's enough code now to warrant them.

https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
File remoting/webapp/crd/js/error.js (right):

https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
remoting/webapp/crd/js/error.js:98: return new remoting.Error.none();
On 2015/03/11 22:16:38, Jamie wrote:
> Don't need new here or for unexpected() (in fact, I'm surprised this
compiles).

Done.

https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
File remoting/webapp/crd/js/server_log_entry.js (right):

https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
remoting/webapp/crd/js/server_log_entry.js:234: if
(connectionError.tag.isError()) {
On 2015/03/11 22:16:38, Jamie wrote:
> isError() is a method on Error, not Tag?

Done.

[Jamie, 2015-03-12 19:33:28]

On 2015/03/12 02:46:20, John Williams wrote:
> PTAL; I added a hasTag method for the sake of type safety, and some unit tests
> since there's enough code now to warrant them.
> 
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
> File remoting/webapp/crd/js/error.js (right):
> 
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
> remoting/webapp/crd/js/error.js:98: return new remoting.Error.none();
> On 2015/03/11 22:16:38, Jamie wrote:
> > Don't need new here or for unexpected() (in fact, I'm surprised this
> compiles).
> 
> Done.
> 
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
> File remoting/webapp/crd/js/server_log_entry.js (right):
> 
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
> remoting/webapp/crd/js/server_log_entry.js:234: if
> (connectionError.tag.isError()) {
> On 2015/03/11 22:16:38, Jamie wrote:
> > isError() is a method on Error, not Tag?
> 
> Done.

I think you forgot to upload the patch-set.

[John Williams, 2015-03-12 20:56:12]

On 2015/03/12 19:33:28, Jamie wrote:
> On 2015/03/12 02:46:20, John Williams wrote:
> > PTAL; I added a hasTag method for the sake of type safety, and some unit
tests
> > since there's enough code now to warrant them.
> > 
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
> > File remoting/webapp/crd/js/error.js (right):
> > 
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
> > remoting/webapp/crd/js/error.js:98: return new remoting.Error.none();
> > On 2015/03/11 22:16:38, Jamie wrote:
> > > Don't need new here or for unexpected() (in fact, I'm surprised this
> > compiles).
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
> > File remoting/webapp/crd/js/server_log_entry.js (right):
> > 
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
> > remoting/webapp/crd/js/server_log_entry.js:234: if
> > (connectionError.tag.isError()) {
> > On 2015/03/11 22:16:38, Jamie wrote:
> > > isError() is a method on Error, not Tag?
> > 
> > Done.
> 
> I think you forgot to upload the patch-set.

I didn't forget, but I did run "cl upload" from the wrong branch, and I assumed
there was just some delay in updating this page.  Do you know (1) if the code
review pages always show the most up-to-date data, and (2) if there is a way to
make the name of my git branch appear in the review tool?

[Jamie, 2015-03-12 21:18:57]

On 2015/03/12 20:56:12, John Williams wrote:
> On 2015/03/12 19:33:28, Jamie wrote:
> > On 2015/03/12 02:46:20, John Williams wrote:
> > > PTAL; I added a hasTag method for the sake of type safety, and some unit
> tests
> > > since there's enough code now to warrant them.
> > > 
> > >
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
> > > File remoting/webapp/crd/js/error.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
> > > remoting/webapp/crd/js/error.js:98: return new remoting.Error.none();
> > > On 2015/03/11 22:16:38, Jamie wrote:
> > > > Don't need new here or for unexpected() (in fact, I'm surprised this
> > > compiles).
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
> > > File remoting/webapp/crd/js/server_log_entry.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
> > > remoting/webapp/crd/js/server_log_entry.js:234: if
> > > (connectionError.tag.isError()) {
> > > On 2015/03/11 22:16:38, Jamie wrote:
> > > > isError() is a method on Error, not Tag?
> > > 
> > > Done.
> > 
> > I think you forgot to upload the patch-set.
> 
> I didn't forget, but I did run "cl upload" from the wrong branch, and I
assumed
> there was just some delay in updating this page.  Do you know (1) if the code
> review pages always show the most up-to-date data, and (2) if there is a way
to
> make the name of my git branch appear in the review tool?

(1) I've never had a problem with the site not display up-to-date information.
(2) No, I don't think that's possible.

FWIW, I'm still not seeing patch-set 2. Please LMK when it's uploaded.

[Jamie, 2015-03-12 21:19:46]

On 2015/03/12 21:18:57, Jamie wrote:
> On 2015/03/12 20:56:12, John Williams wrote:
> > On 2015/03/12 19:33:28, Jamie wrote:
> > > On 2015/03/12 02:46:20, John Williams wrote:
> > > > PTAL; I added a hasTag method for the sake of type safety, and some unit
> > tests
> > > > since there's enough code now to warrant them.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
> > > > File remoting/webapp/crd/js/error.js (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/erro...
> > > > remoting/webapp/crd/js/error.js:98: return new remoting.Error.none();
> > > > On 2015/03/11 22:16:38, Jamie wrote:
> > > > > Don't need new here or for unexpected() (in fact, I'm surprised this
> > > > compiles).
> > > > 
> > > > Done.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
> > > > File remoting/webapp/crd/js/server_log_entry.js (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1004513002/diff/1/remoting/webapp/crd/js/serv...
> > > > remoting/webapp/crd/js/server_log_entry.js:234: if
> > > > (connectionError.tag.isError()) {
> > > > On 2015/03/11 22:16:38, Jamie wrote:
> > > > > isError() is a method on Error, not Tag?
> > > > 
> > > > Done.
> > > 
> > > I think you forgot to upload the patch-set.
> > 
> > I didn't forget, but I did run "cl upload" from the wrong branch, and I
> assumed
> > there was just some delay in updating this page.  Do you know (1) if the
code
> > review pages always show the most up-to-date data, and (2) if there is a way
> to
> > make the name of my git branch appear in the review tool?
> 
> (1) I've never had a problem with the site not display up-to-date information.
> (2) No, I don't think that's possible.
> 
> FWIW, I'm still not seeing patch-set 2. Please LMK when it's uploaded.

I see it now.

[Jamie, 2015-03-12 21:40:11]

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

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:97: if
(error.hasTag(remoting.Error.Tag.CANCELLED)) {
TBH, I have no problem with == (or === since it seems like the old code wasn't
consistent) here, but if you feel it's better encapsulated by hasTag, then the
tag member should probably be private.

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

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:29: * using ==.
If you make tag_ private, there's no need for the second sentence (since this
will be the only way to check the type of an error).

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:42: remoting.Error.prototype.isError =
function() {
I think we should remove this method. It's not widely-used and it's equivalent
to hasTag(NONE). It's also not obvious to me that it should return true for
CANCELLED.

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xmpp_connection.js (right):

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xmpp_connection.js:236:
this.onError_(remoting.Error.NETWORK_FAILURE,
The change (and the next) looks like it's going in the wrong direction. How did
it pass the compiler?

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xmpp_login_handler_unittest.js (right):

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xmpp_login_handler_unittest.js:166:
remoting.Error.Tag.AUTHENTICATION_FAILED));
Does this work? Can calledWith match two error instances that have the same tag,
but were created with separate calls to new?

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/unittes...
File remoting/webapp/unittests/error_unittest.js (right):

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/unittes...
remoting/webapp/unittests/error_unittest.js:48:
QUnit.ok(remoting.Error.unexpected().isError());
What should isError(CANCELLED) return?

[John Williams, 2015-03-13 00:26:35]

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

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:97: if
(error.hasTag(remoting.Error.Tag.CANCELLED)) {
On 2015/03/12 21:40:11, Jamie wrote:
> TBH, I have no problem with == (or === since it seems like the old code wasn't
> consistent) here, but if you feel it's better encapsulated by hasTag, then the
> tag member should probably be private.

I wanted to discourage == or === in new code since it's easy to forget that
Error objects and error tags aren't comparable, and the compiler offers to help.

I had to add a getting and move some code from another file to make this work. 
What do you think of the result?

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

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:29: * using ==.
On 2015/03/12 21:40:11, Jamie wrote:
> If you make tag_ private, there's no need for the second sentence (since this
> will be the only way to check the type of an error).

Done.

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:42: remoting.Error.prototype.isError =
function() {
On 2015/03/12 21:40:11, Jamie wrote:
> I think we should remove this method. It's not widely-used and it's equivalent
> to hasTag(NONE). It's also not obvious to me that it should return true for
> CANCELLED.

I changed it to isNone to make it more clear, but I still think it's worth
having since it's called in 7 places, not counting unit tests.

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xmpp_connection.js (right):

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xmpp_connection.js:236:
this.onError_(remoting.Error.NETWORK_FAILURE,
On 2015/03/12 21:40:11, Jamie wrote:
> The change (and the next) looks like it's going in the wrong direction. How
did
> it pass the compiler?

Ugh. Long story. But you are correct.

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/xmpp_login_handler_unittest.js (right):

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/xmpp_login_handler_unittest.js:166:
remoting.Error.Tag.AUTHENTICATION_FAILED));
On 2015/03/12 21:40:11, Jamie wrote:
> Does this work? Can calledWith match two error instances that have the same
tag,
> but were created with separate calls to new?

The tests pass, so I assume this method is doing a deep comparison.

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/unittes...
File remoting/webapp/unittests/error_unittest.js (right):

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/unittes...
remoting/webapp/unittests/error_unittest.js:48:
QUnit.ok(remoting.Error.unexpected().isError());
On 2015/03/12 21:40:11, Jamie wrote:
> What should isError(CANCELLED) return?

Done.

[John Williams, 2015-03-13 17:51:46]

not LGTM

Let's see if I can revoke the LGTM on my own CL, since I'm not comfortable
submitting without a new LGTM.

[Jamie, 2015-03-13 17:56:17]

It doesn't look like your "not LGTM" worked, so here's one that will :)

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

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:97: if
(error.hasTag(remoting.Error.Tag.CANCELLED)) {
On 2015/03/13 00:26:35, John Williams wrote:
> On 2015/03/12 21:40:11, Jamie wrote:
> > TBH, I have no problem with == (or === since it seems like the old code
wasn't
> > consistent) here, but if you feel it's better encapsulated by hasTag, then
the
> > tag member should probably be private.
> 
> I wanted to discourage == or === in new code since it's easy to forget that
> Error objects and error tags aren't comparable, and the compiler offers to
help.

What does the compiler do if you try to use == between a Error and a tag?

> 
> I had to add a getting and move some code from another file to make this work.

> What do you think of the result?

I think a straight getter would be cleaner. See my comments on that file.

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_remoting.js:305: console.error('Connection
failed:', error);
This (and below) looks like another change in the wrong direction.

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/dns_blackhole_checker_unittest.js (right):

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/dns_blackhole_checker_unittest.js:8: * @suppress
{accessControls|checkTypes|checkVars|reportUnknownTypes|visibility}
Do you need to suppress accessControls if you're using hasTag?

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

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:30:
remoting.Error.prototype.getTagForLocalization = function() {
I think getTag is a better name, but then I'm not convinced that hasTag(x) is
any clearer or safer than getTag() == x. Is it just the confusion between == and
===?

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:37: remoting.Error.prototype.getTagForServerLog
= function() {
Another point in favour of calling the previous method getTag. I think this
mapping is better where it was.

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:82: remoting.Error.prototype.hasAnyTag =
function(tags) {
I think you can omit this method and just use varargs style for hasTag (which
would address my concern about it not being particularly useful in the presence
of getTag).

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/l10n.js (right):

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/l10n.js:32: *     an Error object containing the tag.
This should just be a string, and the caller should be responsible for
extracting the tag from an Error.

[John Williams, 2015-03-13 19:30:29]

OK, maybe the 6th time's a charm on this one.

BTW I'm sorry to waste your time with all these little missteps with git,
depot_tools, etc.

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

https://codereview.chromium.org/1004513002/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:97: if
(error.hasTag(remoting.Error.Tag.CANCELLED)) {
On 2015/03/13 17:56:16, Jamie wrote:
> On 2015/03/13 00:26:35, John Williams wrote:
> > On 2015/03/12 21:40:11, Jamie wrote:
> > > TBH, I have no problem with == (or === since it seems like the old code
> wasn't
> > > consistent) here, but if you feel it's better encapsulated by hasTag, then
> the
> > > tag member should probably be private.
> > 
> > I wanted to discourage == or === in new code since it's easy to forget that
> > Error objects and error tags aren't comparable, and the compiler offers to
> help.
> 
> What does the compiler do if you try to use == between a Error and a tag?

Nothing :-)  It compiles it just fine, but the comparison will always be false. 
Might be worth a feature request to the compiler team.

> 
> > 
> > I had to add a getting and move some code from another file to make this
work.
> 
> > What do you think of the result?
> 
> I think a straight getter would be cleaner. See my comments on that file.

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_remoting.js:305: console.error('Connection
failed:', error);
On 2015/03/13 17:56:17, Jamie wrote:
> This (and below) looks like another change in the wrong direction.

This one was deliberate, since I was trying to minimize usage of the getter.

I changed it to use a toString() method, but I'm not sure it there's really any
advantage other than keeping the log format identical to what it was before.  I
checked, and objects logged to the console (without %o) copy and paste just
fine.

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/dns_blackhole_checker_unittest.js (right):

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/dns_blackhole_checker_unittest.js:8: * @suppress
{accessControls|checkTypes|checkVars|reportUnknownTypes|visibility}
On 2015/03/13 17:56:17, Jamie wrote:
> Do you need to suppress accessControls if you're using hasTag?

Nope, meant to remove that.

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

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:30:
remoting.Error.prototype.getTagForLocalization = function() {
On 2015/03/13 17:56:17, Jamie wrote:
> I think getTag is a better name, but then I'm not convinced that hasTag(x) is
> any clearer or safer than getTag() == x. Is it just the confusion between ==
and
> ===?

No, == vs. === is not a major issue AFAICT.  What matters is that if someone
writes an expression like one of these, the comparison will always return false:

  error == remoting.Error.Tag.NONE
  error == remoting.Error.none()
  error.tag == remoting.Error.none()

This seems like a pretty easy mistake to make.

...

After some further travel down the JS semantics rathole, I've discovered that
comparing an error object to a string will actually work if the object's
toString method returns the tag, so maybe I should rename getTag to toString,
get rid of the toString method I wrote, and just log error objects as objects so
the detail field is logged.   Personally I'd rather not do that, since it's
obscure, and it conflicts with my Java-based intuition that the value returned
by a toString method is ONLY for producing human-readable output.

(BTW, nothing uses the detail field yet, but I plan on using it in new code, and
the existing remoting.XmppConnection.prototype.onError_ method looks like a good
place to use it as well.)

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:37: remoting.Error.prototype.getTagForServerLog
= function() {
On 2015/03/13 17:56:17, Jamie wrote:
> Another point in favour of calling the previous method getTag. I think this
> mapping is better where it was.

Done.

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:82: remoting.Error.prototype.hasAnyTag =
function(tags) {
On 2015/03/13 17:56:17, Jamie wrote:
> I think you can omit this method and just use varargs style for hasTag (which
> would address my concern about it not being particularly useful in the
presence
> of getTag).

Done.

[Jamie, 2015-03-13 20:26:01]

LGTM with nits.

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

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/error.js:30:
remoting.Error.prototype.getTagForLocalization = function() {
On 2015/03/13 19:30:29, John Williams wrote:
> On 2015/03/13 17:56:17, Jamie wrote:
> > I think getTag is a better name, but then I'm not convinced that hasTag(x)
is
> > any clearer or safer than getTag() == x. Is it just the confusion between ==
> and
> > ===?
> 
> No, == vs. === is not a major issue AFAICT.  What matters is that if someone
> writes an expression like one of these, the comparison will always return
false:
> 
>   error == remoting.Error.Tag.NONE
>   error == remoting.Error.none()
>   error.tag == remoting.Error.none()
> 
> This seems like a pretty easy mistake to make.
> 
> ...
> 
> After some further travel down the JS semantics rathole, I've discovered that
> comparing an error object to a string will actually work if the object's
> toString method returns the tag, so maybe I should rename getTag to toString,
> get rid of the toString method I wrote, and just log error objects as objects
so
> the detail field is logged.   Personally I'd rather not do that, since it's
> obscure, and it conflicts with my Java-based intuition that the value returned
> by a toString method is ONLY for producing human-readable output.
> 
> (BTW, nothing uses the detail field yet, but I plan on using it in new code,
and
> the existing remoting.XmppConnection.prototype.onError_ method looks like a
good
> place to use it as well.)

I'm inclined to agree with you, and the new form of hasTag that can take
multiple arguments adds enough value for me to withdraw my objection.

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/l10n.js (right):

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/l10n.js:32: *     an Error object containing the tag.
On 2015/03/13 17:56:17, Jamie wrote:
> This should just be a string, and the caller should be responsible for
> extracting the tag from an Error.

Please address this comment.

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_session.js:646: console.log('Failed to refresh
access token:', error.toString());
Nit: When all objects are strings, plus is better than comma. It used to be the
case that Chrome only echoed the first argument to the terminal is was launched
from, so if we're ever looking at browser test output, for example, we'll lose
information. That may no longer be true (I haven't checked in a long time), but
plus is more prevalent elsewhere. I think I would still prefer an explicit call
to toString, since I'm not a big fan of implicit conversions.

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

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/error.js:57: });
This is fine, but you could eliminate thisTag with an additional |this| argument
to some(). The extra JSCompile hassle might make it not worthwhile, however.

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/error_unittest.js (right):

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/error_unittest.js:57:
remoting.Error.fromHttpStatus(0).getTag(),
Use hasTag for consistency with the other tests?

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/host_daemon_facade.js (right):

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/host_daemon_facade.js:209: console.error('Error while
processing native message', e);
No change required: This comma is correct, I think, since we don't know the type
of e.

[John Williams, 2015-03-13 22:34:03]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-13 22:34:04]

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/1004513002/#ps140001
(title: "ftw")

[commit-bot: I haz the power, 2015-03-13 22:34:16]

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

[John Williams, 2015-03-13 22:40:31]

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/l10n.js (right):

https://codereview.chromium.org/1004513002/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/l10n.js:32: *     an Error object containing the tag.
On 2015/03/13 20:26:01, Jamie wrote:
> On 2015/03/13 17:56:17, Jamie wrote:
> > This should just be a string, and the caller should be responsible for
> > extracting the tag from an Error.
> 
> Please address this comment.

Done.

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_session.js:646: console.log('Failed to refresh
access token:', error.toString());
On 2015/03/13 20:26:01, Jamie wrote:
> Nit: When all objects are strings, plus is better than comma. It used to be
the
> case that Chrome only echoed the first argument to the terminal is was
launched
> from, so if we're ever looking at browser test output, for example, we'll lose
> information. That may no longer be true (I haven't checked in a long time),
but
> plus is more prevalent elsewhere. I think I would still prefer an explicit
call
> to toString, since I'm not a big fan of implicit conversions.

It still works the same way :(

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/error_unittest.js (right):

https://codereview.chromium.org/1004513002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/error_unittest.js:57:
remoting.Error.fromHttpStatus(0).getTag(),
On 2015/03/13 20:26:01, Jamie wrote:
> Use hasTag for consistency with the other tests?

I'd rather not.  This way the test will produce more informative error messages
if it ever breaks.

[commit-bot: I haz the power, 2015-03-13 23:33:14]

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

[commit-bot: I haz the power, 2015-03-13 23:33:15]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[John Williams, 2015-03-14 00:40:42]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-14 00:40: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/1004513002/#ps160001
(title: "re-sync")

[commit-bot: I haz the power, 2015-03-14 00:41:04]

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

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

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

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

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[John Williams, 2015-03-14 02:04:10]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-14 02:04:10]

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

[commit-bot: I haz the power, 2015-03-14 02:04:20]

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

[commit-bot: I haz the power, 2015-03-14 04:14:33]

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

[commit-bot: I haz the power, 2015-03-14 04:14:34]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[John Williams, 2015-03-16 19:20:52]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-16 19:20:52]

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/1004513002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-03-16 19:21:17]

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

[commit-bot: I haz the power, 2015-03-16 21:26:45]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-16 21:27:28]

Patchset 11 (id:??) landed as
https://crrev.com/745e53b571adbe742caf3b92cc8c692eb1f51b6e
Cr-Commit-Position: refs/heads/master@{#320795}