[Chromoting] Enable jscompile for webapp unittests.

[Chromoting] Enable jscompile for webapp unittests.

This adds a target to run jscompile over the unittest files.

The save/restore functions of chromeMocks are removed since they are not
necessary. chromeMocks.reset() is called automatically before each test.

It also fixes most of the errors that jscompile produces. For the unittests
where all the errors have not been fixed, jscompile is disabled for those
files. Cases where errors were not fixed include those that require
refactoring base classes to expose an interface that the corresponding
mock can use -- these are best done is a separate, targetted cl.

Also, the it2me tests are not checked because they will be removed soon.

BUG=

Committed: https://crrev.com/818c8017bfe5555ab5093613bba737f520a73131
Cr-Commit-Position: refs/heads/master@{#318725}

[garykac, 2015-02-25 23:15:09]

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

[kelvinp, 2015-02-26 00:31:31]

Thank you for doing this change and battling with all the JSCompile errors.
Looks good in general, just an overall comment on we should use upper case for
types in protos and the $ should not be necessary.

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp.gypi
File remoting/remoting_webapp.gypi (right):

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp.gyp...
remoting/remoting_webapp.gypi:63: '<@(remoting_webapp_crd_js_files)',
Would '<@(_inputs)' work so you don't need to duplicate the file list?

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp.gyp...
remoting/remoting_webapp.gypi:83: '<@(remoting_webapp_crd_js_files)',
Same here

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp_fil...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp_fil...
remoting/remoting_webapp_files.gypi:89: 'webapp/js_proto/chrome_proto.js',
I thought we are getting rid of chrome_proto.js

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/crd/html/tem...
File remoting/webapp/crd/html/template_unittest.html (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/crd/html/tem...
remoting/webapp/crd/html/template_unittest.html:25: <script
src="test_start.js"></script>
rename to test_init?  I think the role of the file is to initialize any test
related logic.  I don't feel strongly either way though.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/qun...
File remoting/webapp/js_proto/qunit_proto.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/qun...
remoting/webapp/js_proto/qunit_proto.js:14: QUnit.$test = function() {};
Upper case for Types and drop $

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/qun...
remoting/webapp/js_proto/qunit_proto.js:20: QUnit.$clock = function() {};
upper case for types and no need to use $ as proto are meant to be private.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
File remoting/webapp/js_proto/sinon_proto.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:36: sinon.expectation = function() {};
Use upper case for type names s/expectation/Expectation

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:54: sinon.$mock = function() {};
Use upper case for type names s/$mock/Mock
Not need to use $ here as this file won't be included in unittest.html so
namespace conflict shouldn't be a problem.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:68: /** @type {function(...):Function}
*/
should it be 
/**
 * @param {...} var_args
 * @return {Function}
 */

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:82: sinon.$spy = function() {};
Use upper case for type names s/$spy/Spy

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:106: sinon.$testStub = function() {};
Use upper case for type names s/$testStub/TestStub

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
File remoting/webapp/js_proto/sinon_stub_proto.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_stub_proto.js:18: // in this file for Chrome
object you stub out this way. For example:
s/Chrome/any

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ba...
File remoting/webapp/unittests/base_unittest.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ba...
remoting/webapp/unittests/base_unittest.js:7: * TODO(garykac): Create interface
for base.Disposable.
base.Disposable is already an interface.  Am I missing something?

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ev...
File remoting/webapp/unittests/event_hook_unittest.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ev...
remoting/webapp/unittests/event_hook_unittest.js:31: new
base.DomEventHook(domElement, 'click', this.onClickEvent.bind(this),
should be element as it is passed through the constructor.  Or you can keep it
to be domElement and remove element from the constructor

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ip...
File remoting/webapp/unittests/ipc_unittest.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ip...
remoting/webapp/unittests/ipc_unittest.js:46: var handler = /** @type
{function(...?)} */ (sinon.spy());
You can simply change the definition ipc.js to be Function and that would saves
you all the modifications in this file other than setup on the top.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/si...
File remoting/webapp/unittests/sinon_helpers.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/si...
remoting/webapp/unittests/sinon_helpers.js:5: // This file contains various mock
objects for the chrome platform to make
update comments

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/te...
File remoting/webapp/unittests/test_start.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/te...
remoting/webapp/unittests/test_start.js:11: console.log("Running: ",
details.module, details.name);
Probably won't need to extra log spew to pollute the console.

[garykac, 2015-02-28 02:33:34]

ptal

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp.gypi
File remoting/remoting_webapp.gypi (right):

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp.gyp...
remoting/remoting_webapp.gypi:63: '<@(remoting_webapp_crd_js_files)',
On 2015/02/26 00:31:30, kelvinp wrote:
> Would '<@(_inputs)' work so you don't need to duplicate the file list?

Using _inputs is potentially "dangerous" because it is a list of *all* the
source dependencies for the action -- so we probably should have things like the
checker.py script listed there as well.

In this case, it just happens to be the same list, but that's why we don't do
that in general.

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp.gyp...
remoting/remoting_webapp.gypi:83: '<@(remoting_webapp_crd_js_files)',
On 2015/02/26 00:31:30, kelvinp wrote:
> Same here

ibid

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp_fil...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/959963002/diff/1/remoting/remoting_webapp_fil...
remoting/remoting_webapp_files.gypi:89: 'webapp/js_proto/chrome_proto.js',
On 2015/02/26 00:31:30, kelvinp wrote:
> I thought we are getting rid of chrome_proto.js

I removed the parts that needed to be moved out, but the majority of it would be
identical between the proto and the mock, so I left it for now.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/crd/html/tem...
File remoting/webapp/crd/html/template_unittest.html (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/crd/html/tem...
remoting/webapp/crd/html/template_unittest.html:25: <script
src="test_start.js"></script>
On 2015/02/26 00:31:30, kelvinp wrote:
> rename to test_init?  I think the role of the file is to initialize any test
> related logic.  I don't feel strongly either way though.

The function it implements is QUnit.testStart, so test_start seems more
appropriate.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/qun...
File remoting/webapp/js_proto/qunit_proto.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/qun...
remoting/webapp/js_proto/qunit_proto.js:14: QUnit.$test = function() {};
On 2015/02/26 00:31:30, kelvinp wrote:
> Upper case for Types and drop $

Done.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/qun...
remoting/webapp/js_proto/qunit_proto.js:20: QUnit.$clock = function() {};
On 2015/02/26 00:31:30, kelvinp wrote:
> upper case for types and no need to use $ as proto are meant to be private.

Done.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
File remoting/webapp/js_proto/sinon_proto.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:36: sinon.expectation = function() {};
On 2015/02/26 00:31:30, kelvinp wrote:
> Use upper case for type names s/expectation/Expectation

Done.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:54: sinon.$mock = function() {};
On 2015/02/26 00:31:30, kelvinp wrote:
> Use upper case for type names s/$mock/Mock
> Not need to use $ here as this file won't be included in unittest.html so
> namespace conflict shouldn't be a problem.

Done.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:68: /** @type {function(...):Function}
*/
On 2015/02/26 00:31:30, kelvinp wrote:
> should it be 
> /**
>  * @param {...} var_args
>  * @return {Function}
>  */

They're the same thing.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:82: sinon.$spy = function() {};
On 2015/02/26 00:31:30, kelvinp wrote:
> Use upper case for type names s/$spy/Spy

Done.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_proto.js:106: sinon.$testStub = function() {};
On 2015/02/26 00:31:30, kelvinp wrote:
> Use upper case for type names s/$testStub/TestStub

Done.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
File remoting/webapp/js_proto/sinon_stub_proto.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/js_proto/sin...
remoting/webapp/js_proto/sinon_stub_proto.js:18: // in this file for Chrome
object you stub out this way. For example:
On 2015/02/26 00:31:30, kelvinp wrote:
> s/Chrome/any

Done.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ba...
File remoting/webapp/unittests/base_unittest.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ba...
remoting/webapp/unittests/base_unittest.js:7: * TODO(garykac): Create interface
for base.Disposable.
On 2015/02/26 00:31:30, kelvinp wrote:
> base.Disposable is already an interface.  Am I missing something?

Nope. I missed it. Enabling jscompile for this file.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ev...
File remoting/webapp/unittests/event_hook_unittest.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ev...
remoting/webapp/unittests/event_hook_unittest.js:31: new
base.DomEventHook(domElement, 'click', this.onClickEvent.bind(this),
On 2015/02/26 00:31:30, kelvinp wrote:
> should be element as it is passed through the constructor.  Or you can keep it
> to be domElement and remove element from the constructor

I didn't see the 'element' param and thought it was undefined.

Reverted and added @param for element.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ip...
File remoting/webapp/unittests/ipc_unittest.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/ip...
remoting/webapp/unittests/ipc_unittest.js:46: var handler = /** @type
{function(...?)} */ (sinon.spy());
On 2015/02/26 00:31:30, kelvinp wrote:
> You can simply change the definition ipc.js to be Function and that would
saves
> you all the modifications in this file other than setup on the top.

Done.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/si...
File remoting/webapp/unittests/sinon_helpers.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/si...
remoting/webapp/unittests/sinon_helpers.js:5: // This file contains various mock
objects for the chrome platform to make
On 2015/02/26 00:31:30, kelvinp wrote:
> update comments

Done.

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/te...
File remoting/webapp/unittests/test_start.js (right):

https://codereview.chromium.org/959963002/diff/1/remoting/webapp/unittests/te...
remoting/webapp/unittests/test_start.js:11: console.log("Running: ",
details.module, details.name);
On 2015/02/26 00:31:30, kelvinp wrote:
> Probably won't need to extra log spew to pollute the console.

Done.

[kelvinp, 2015-02-28 03:03:52]

lgtm once comments are addressed.

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

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_init.js:46: /** @param {{email:string,
name:string}} userInfo */
change in this file shouldn't be necessary.

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/crd/js/a...
File remoting/webapp/crd/js/apps_v2_migration.js (right):

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/apps_v2_migration.js:92: /** @param {{email:string,
name:string}} userInfo */
Nit: Don't think we need the JSDoc at all, as the return type is defined in
remoting.identity.

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/crd/js/o...
File remoting/webapp/crd/js/oauth2.js (right):

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/crd/js/o...
remoting/webapp/crd/js/oauth2.js:27: * @extends {remoting.Identity} */
Nit: indent.

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/unittest...
File remoting/webapp/unittests/base_unittest.js (right):

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/base_unittest.js:119: /** @suppress
{reportUnknownTypes|checkVars|checkTypes} */
what is the unknown type here?

[garykac, 2015-02-28 03:28:23]

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

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/base/js/...
remoting/webapp/base/js/auth_init.js:46: /** @param {{email:string,
name:string}} userInfo */
On 2015/02/28 03:03:52, kelvinp wrote:
> change in this file shouldn't be necessary.

I agree.

Sadly, jscompile says it is. :-(

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/crd/js/a...
File remoting/webapp/crd/js/apps_v2_migration.js (right):

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/crd/js/a...
remoting/webapp/crd/js/apps_v2_migration.js:92: /** @param {{email:string,
name:string}} userInfo */
On 2015/02/28 03:03:52, kelvinp wrote:
> Nit: Don't think we need the JSDoc at all, as the return type is defined in
> remoting.identity.

This was added at the request of jscompile.

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/crd/js/o...
File remoting/webapp/crd/js/oauth2.js (right):

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/crd/js/o...
remoting/webapp/crd/js/oauth2.js:27: * @extends {remoting.Identity} */
On 2015/02/28 03:03:52, kelvinp wrote:
> Nit: indent.

Done.

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/unittest...
File remoting/webapp/unittests/base_unittest.js (right):

https://codereview.chromium.org/959963002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/base_unittest.js:119: /** @suppress
{reportUnknownTypes|checkVars|checkTypes} */
On 2015/02/28 03:03:52, kelvinp wrote:
> what is the unknown type here? 

'this'. And @this didn't help. I added a comment.

[garykac, 2015-02-28 03:28:31]

The CQ bit was checked by garykac@chromium.org

[garykac, 2015-02-28 03:28: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/959963002/#ps100001
(title: "Fix key_tester jscompile")

[commit-bot: I haz the power, 2015-02-28 03:29:25]

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

[commit-bot: I haz the power, 2015-02-28 05:07:50]

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

[commit-bot: I haz the power, 2015-02-28 05:07:51]

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_...)

[garykac, 2015-03-02 16:06:31]

The CQ bit was checked by garykac@chromium.org

[commit-bot: I haz the power, 2015-03-02 16:06:58]

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

[commit-bot: I haz the power, 2015-03-02 17:55:53]

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

[commit-bot: I haz the power, 2015-03-02 17:55:54]

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_...)

[garykac, 2015-03-02 17:56:47]

The CQ bit was checked by garykac@chromium.org

[garykac, 2015-03-02 17:56:48]

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/959963002/#ps120001
(title: "Fix key tester")

[commit-bot: I haz the power, 2015-03-02 17:57:43]

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

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

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-02 18:38:48]

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