Added more typechecking functions and unit tests for existing code.

Added more typechecking functions and unit tests for existing code.

This change alters the functionality of getObjectAttr slightly: it no
longer accepts null or Array objects as "object" values.  I made this
change because the type checking code seems designed to mirror the
semantics of JSON, where each value belongs to exactly one of the
following categories: array, boolean, number, null, object, or string.

This change also changes the type of values thrown from strings to Error objects so that there are more likely to be useful stack traces when an error occurs.

BUG=

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

[John Williams, 2015-03-17 01:48:27]

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

[John Williams, 2015-03-17 19:50:36]

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

[kelvinp, 2015-03-17 21:41:48]

lgtm

PTAL

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

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck.js:13: throw new Error('Undefined value');
we have a function base.debug.assert which current doesn't throw but only logs
an error message.

I think we can probably modify that function to throw and use base.debug.assert
here.

Same for the functions below.

In general, we are moving away from global functions so please add the new
functions to the base namespace.

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck.js:22: if (value instanceof Array) {
Nit: Array.isArray(value) as it is more robust across iframes.
http://stackoverflow.com/questions/22289727/difference-between-using-array-is...

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck.js:84: * array value.
comment needs update. array value no longer applies.

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

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck_unittest.js:32: setup: function() {
empty setup, teardown is not required.

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck_unittest.js:39:
QUnit.strictEqual(assertDefined(anArray), anArray);
I have migrated to the latest syntax

QUnit.test('foo', function(assert) {
  assert.ok(true);
}

As assert.ok provides the actual callstack when it fails whereas QUnit.ok does
not.

[John Williams, 2015-03-18 17:23:01]

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

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck.js:13: throw new Error('Undefined value');
On 2015/03/17 21:41:48, kelvinp wrote:
> we have a function base.debug.assert which current doesn't throw but only logs
> an error message.
> 
> I think we can probably modify that function to throw and use
base.debug.assert
> here.
> 
> Same for the functions below.

I tried rewriting in terms of assert, but it won't make the code any shorter
without sacrificing informative error messages.

> 
> In general, we are moving away from global functions so please add the new
> functions to the base namespace.

Done.

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck.js:22: if (value instanceof Array) {
On 2015/03/17 21:41:48, kelvinp wrote:
> Nit: Array.isArray(value) as it is more robust across iframes.
>
http://stackoverflow.com/questions/22289727/difference-between-using-array-is...

Done.

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck.js:84: * array value.
On 2015/03/17 21:41:48, kelvinp wrote:
> comment needs update. array value no longer applies.

I just got rid of this function entirely.

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

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck_unittest.js:32: setup: function() {
On 2015/03/17 21:41:48, kelvinp wrote:
> empty setup, teardown is not required.

Done.

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck_unittest.js:39:
QUnit.strictEqual(assertDefined(anArray), anArray);
On 2015/03/17 21:41:48, kelvinp wrote:
> I have migrated to the latest syntax
> 
> QUnit.test('foo', function(assert) {
>   assert.ok(true);
> }
> 
> As assert.ok provides the actual callstack when it fails whereas QUnit.ok does
> not.

Done.

BTW, what would you think about switching to Jasmine
(http://jasmine.github.io/2.2/introduction.html) for new tests?  It has most of
the major features of both QUnit and Sinon, and it's possible to set up the unit
tests to run as part of the build.  It also reports useful stack traces much
more often than QUnit.

[garykac, 2015-03-18 17:28:37]

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

https://codereview.chromium.org/1015553003/diff/40001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck.js:10: 'use strict';
All of our files should have 'use strict' at global scope as the first line of
the file. It was missing from this file, but we should add it there rather than
inside this temp function.

https://codereview.chromium.org/1015553003/diff/40001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/typecheck_unittest.js (right):

https://codereview.chromium.org/1015553003/diff/40001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck_unittest.js:7: 'use strict';
Move to global scope.

[kelvinp, 2015-03-18 17:52:19]

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

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck_unittest.js:39:
QUnit.strictEqual(assertDefined(anArray), anArray);
On 2015/03/18 17:23:01, John Williams wrote:
> On 2015/03/17 21:41:48, kelvinp wrote:
> > I have migrated to the latest syntax
> > 
> > QUnit.test('foo', function(assert) {
> >   assert.ok(true);
> > }
> > 
> > As assert.ok provides the actual callstack when it fails whereas QUnit.ok
does
> > not.
> 
> Done.
> 
> BTW, what would you think about switching to Jasmine
> (http://jasmine.github.io/2.2/introduction.html) for new tests?  It has most
of
> the major features of both QUnit and Sinon, and it's possible to set up the
unit
> tests to run as part of the build.  It also reports useful stack traces much
> more often than QUnit.

Jasmine is one of the candidate when I investigate the JavaScript Unittest
framework for chromoting.  
See
https://docs.google.com/a/chromium.org/document/d/1SOacnv91zFWEqtietEG6ubh3qD...

I actually like it a lot.  At the end, we choose QUnit because the team doesn't
like Jasmine's BDD syntax and prefers QUnit's TDD syntax.

I think consistency here is the key and we should not introduce another
framework unless there are very very strong reasons.

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

https://codereview.chromium.org/1015553003/diff/40001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck.js:10: 'use strict';
On 2015/03/18 17:28:36, garykac wrote:
> All of our files should have 'use strict' at global scope as the first line of
> the file. It was missing from this file, but we should add it there rather
than
> inside this temp function.

The convention (as least imposed by jshint and jslint) for use strict is to be
placed in a function scope instead of the global scope.  The reason is a typical
page may include third party code that does not comply to the strict standard.

See http://yuiblog.com/blog/2010/12/14/strict-mode-is-coming-to-town/.

In our case, it probably doesn't matter because we don't load third party script
in our app within the same file.  But I would prefer we stick with open
conventions in this case.

[John Williams, 2015-03-18 20:20:19]

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

https://codereview.chromium.org/1015553003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck_unittest.js:39:
QUnit.strictEqual(assertDefined(anArray), anArray);
On 2015/03/18 17:52:19, kelvinp wrote:
> On 2015/03/18 17:23:01, John Williams wrote:
> > On 2015/03/17 21:41:48, kelvinp wrote:
> > > I have migrated to the latest syntax
> > > 
> > > QUnit.test('foo', function(assert) {
> > >   assert.ok(true);
> > > }
> > > 
> > > As assert.ok provides the actual callstack when it fails whereas QUnit.ok
> does
> > > not.
> > 
> > Done.
> > 
> > BTW, what would you think about switching to Jasmine
> > (http://jasmine.github.io/2.2/introduction.html) for new tests?  It has most
> of
> > the major features of both QUnit and Sinon, and it's possible to set up the
> unit
> > tests to run as part of the build.  It also reports useful stack traces much
> > more often than QUnit.
> 
> Jasmine is one of the candidate when I investigate the JavaScript Unittest
> framework for chromoting.  
> See
>
https://docs.google.com/a/chromium.org/document/d/1SOacnv91zFWEqtietEG6ubh3qD...
> 
> I actually like it a lot.  At the end, we choose QUnit because the team
doesn't
> like Jasmine's BDD syntax and prefers QUnit's TDD syntax.
> 
> I think consistency here is the key and we should not introduce another
> framework unless there are very very strong reasons.

Acknowledged.

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

https://codereview.chromium.org/1015553003/diff/40001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck.js:10: 'use strict';
On 2015/03/18 17:52:19, kelvinp wrote:
> On 2015/03/18 17:28:36, garykac wrote:
> > All of our files should have 'use strict' at global scope as the first line
of
> > the file. It was missing from this file, but we should add it there rather
> than
> > inside this temp function.
> 
> The convention (as least imposed by jshint and jslint) for use strict is to be
> placed in a function scope instead of the global scope.  The reason is a
typical
> page may include third party code that does not comply to the strict standard.
> 
> See http://yuiblog.com/blog/2010/12/14/strict-mode-is-coming-to-town/.
> 
> In our case, it probably doesn't matter because we don't load third party
script
> in our app within the same file.  But I would prefer we stick with open
> conventions in this case.

Since there's disagreement, I'm going to leave it as-is.

https://codereview.chromium.org/1015553003/diff/40001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/typecheck_unittest.js (right):

https://codereview.chromium.org/1015553003/diff/40001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/typecheck_unittest.js:7: 'use strict';
On 2015/03/18 17:28:37, garykac wrote:
> Move to global scope.

Since there's disagreement, I'm going to leave it as-is.

[John Williams, 2015-03-18 20:20:31]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-18 20:20:32]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1015553003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-03-18 20:21:38]

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

[commit-bot: I haz the power, 2015-03-18 20:26:21]

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

[commit-bot: I haz the power, 2015-03-18 20:26:23]

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-19 18:26:38]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-19 18:26:38]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1015553003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-03-19 18:26:54]

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

[commit-bot: I haz the power, 2015-03-19 18:29:53]

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

[commit-bot: I haz the power, 2015-03-19 18:29:54]

Try jobs failed on following builders:
  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-19 18:41:10]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-03-19 18:41:10]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1015553003/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-03-19 18:41:37]

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

[commit-bot: I haz the power, 2015-03-19 20:08:59]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-19 20:10:07]

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