[Chromoting] Enable jscompile for browser tests.

[Chromoting] Enable jscompile for browser tests.

This enables JScompile for Chromoting's browsertest code and fixes all
of the reported errors.

This cl also includes a few minor typos and jscompile inconsistencies in the non-test code.

BUG=

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

[garykac, 2015-02-18 00:20:35]

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

[garykac, 2015-02-18 00:20:37]

https://codereview.chromium.org/927373005/diff/1/remoting/webapp/browser_test...
File remoting/webapp/browser_test/mock_session_connector.js (left):

https://codereview.chromium.org/927373005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/mock_session_connector.js:79:
remoting.MockSessionConnector.prototype.requestPairing = function() {
Dead. Leftover from an old version of sessionconnector.

https://codereview.chromium.org/927373005/diff/40001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (left):

https://codereview.chromium.org/927373005/diff/40001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:124: 'remoting_webapp_js_signaling_files': [
Moved to fix alpha-order in this file.

https://codereview.chromium.org/927373005/diff/40001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:83: 'remoting_webapp_unittest_all_files': [
Ditto

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/bump_scroll_browser_test.js (left):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:82:
remoting.desktopConnectedView.pluginWidthForBumpScrollTesting =
These values shouldn't be accessed directly, hence the accessor.

[Jamie, 2015-02-18 00:51:33]

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:47:
browserTest.automationController_ = new window.DomAutomationController();
Can you just initialize this to null?

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_host_list_api.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_host_list_api.js:49: /** @type
{function():void} */ (onDone),
This doesn't look right. Is the issue simply that IIRC validateTokenAndCall can
handle arbitrary parameter lists, so the signature should be more like
function(...):void (I'm unsure of the exact syntax).

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_host_list_api.js:50: /** @type
{function():void} */ (onError),
IIRC, the error callback always has the same signature; there should be no need
to cast it.

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_identity.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_identity.js:70: * @param
{function(string,string)} onDone
This doesn't look like the right signature.

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_session_connector.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_session_connector.js:105: * @suppress
{reportUnknownTypes} Required by onStateChange.
Can you expand on why onStateChange needs this? Also, can the suppression be
more tightly scoped?

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/scrollbar_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:56: * @return {Promise}
Does Promise support a resolution type? In this case, Promise.<boolean> would be
appropriate, I think.

[garykac, 2015-02-18 02:06:49]

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:47:
browserTest.automationController_ = new window.DomAutomationController();
On 2015/02/18 00:51:33, Jamie wrote:
> Can you just initialize this to null?

Done.

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_host_list_api.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_host_list_api.js:49: /** @type
{function():void} */ (onDone),
On 2015/02/18 00:51:33, Jamie wrote:
> This doesn't look right. Is the issue simply that IIRC validateTokenAndCall
can
> handle arbitrary parameter lists, so the signature should be more like
> function(...):void (I'm unsure of the exact syntax).

Using @type {Function} on validateTokenAndCall makes these casts unnecessary.

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_host_list_api.js:50: /** @type
{function():void} */ (onError),
On 2015/02/18 00:51:33, Jamie wrote:
> IIRC, the error callback always has the same signature; there should be no
need
> to cast it.

Done.

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_identity.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_identity.js:70: * @param
{function(string,string)} onDone
On 2015/02/18 00:51:33, Jamie wrote:
> This doesn't look like the right signature.

jscompile is apparently confused by the extra params to bind().  Changed these
types to Function to fix.

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_session_connector.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_session_connector.js:105: * @suppress
{reportUnknownTypes} Required by onStateChange.
On 2015/02/18 00:51:33, Jamie wrote:
> Can you expand on why onStateChange needs this? Also, can the suppression be
> more tightly scoped?

I removed the bind() on onStateChange and switched it to use 'that = this;'.
That made jscompile able to figure out the types.

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/scrollbar_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:56: * @return {Promise}
On 2015/02/18 00:51:33, Jamie wrote:
> Does Promise support a resolution type? In this case, Promise.<boolean> would
be
> appropriate, I think.

Promise<boolean> works (as in, jscompile accepts it), but it also accepts
Promise<string> and Promise<number> here without complaining, so it doesn't seem
to provide any value.

[Jamie, 2015-02-18 02:23:49]

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

[Jamie, 2015-02-18 02:23:50]

lgtm

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_identity.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_identity.js:71: * @param {Function} onError
This one always has the same signature. We might as well be explicit about it.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_session_connector.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_session_connector.js:141: /** @type
{remoting.MockSessionConnector} */
Do you need @type here? I know we used to with the old compiler, but I've seen
new code since we upgraded that didn't specify the type of |that| (which should
be unnecessary).

[kelvinp, 2015-02-18 18:41:30]

https://codereview.chromium.org/927373005/diff/60001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:25: 
remove extra line on 25

https://codereview.chromium.org/927373005/diff/60001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:245:
'remoting_webapp_crd_main_html_js_files': [
Seems like JSCompile are not being run on these files.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:167: uiModeChanged, /** @type
{function(?=)} */ (onUIModeChanged));
Nit: I think it would be easier to simply change the definition of
removeEventListener in base to Function instead of changing every single call
site.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:343: * @param
{browserTest.TestableClass} testClass
This annotation is wrong.  |testClass| is not an instance of
browserTest.TestableClass but a constructor that can return an instance of
|browserTest.TestableClass|.  Function would be the correct annotation.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/bump_scroll_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:48: * @param {{pin:
string}} data
Nit: No space before string

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/cancel_pin_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:21: * @param {{pin:
string, accessCode: string}} data
s/{{pin: string, accessCode: string}}/{{pin:string}}
|data| is designed to be a free formed struct so that each test can passed in
what they need.
accessCode is not required here.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/invalid_pin_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/invalid_pin_browser_test.js:20: * @param {{pin:
string, accessCode: string}} data
s/{{pin: string, accessCode: string}}/{{pin:string}}

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/update_pin_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:22: * @param {{new_pin:
string, old_pin: string}} data
no space between : and string

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_connected_view.js:196:
this.pluginWidthForBumpScrollTesting = width;
make this.pluginWidthForBumpScrollTesting_ private?

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/js_proto...
File remoting/webapp/js_proto/test_proto.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/js_proto...
remoting/webapp/js_proto/test_proto.js:6: // WebKit- and Chrome-specific
properties and methods. It is used only with
s/WebKit- and Chrome-specific/test specific

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/unittest...
File remoting/webapp/unittests/chrome_mocks.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/chrome_mocks.js:109: * @return {Array.<string>}
dbeam recently switch all Array.<string> into Array<string>
https://codereview.chromium.org/917093003

[garykac, 2015-02-19 01:46:25]

New patchsets have been uploaded after l-g-t-m from jamiewalch@chromium.org

[garykac, 2015-02-19 01:48:45]

https://codereview.chromium.org/927373005/diff/60001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:245:
'remoting_webapp_crd_main_html_js_files': [
On 2015/02/18 18:41:29, kelvinp wrote:
> Seems like JSCompile are not being run on these files.

Yes, I'll be following up with code to auto-gen all the html files (and
jscompile them).

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:167: uiModeChanged, /** @type
{function(?=)} */ (onUIModeChanged));
On 2015/02/18 18:41:29, kelvinp wrote:
> Nit: I think it would be easier to simply change the definition of
> removeEventListener in base to Function instead of changing every single call
> site.

Done.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/bump_scroll_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:48: * @param {{pin:
string}} data
On 2015/02/18 18:41:29, kelvinp wrote:
> Nit: No space before string

Done.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/cancel_pin_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/cancel_pin_browser_test.js:21: * @param {{pin:
string, accessCode: string}} data
On 2015/02/18 18:41:29, kelvinp wrote:
> s/{{pin: string, accessCode: string}}/{{pin:string}}
> |data| is designed to be a free formed struct so that each test can passed in
> what they need.
> accessCode is not required here.

Done.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/invalid_pin_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/invalid_pin_browser_test.js:20: * @param {{pin:
string, accessCode: string}} data
On 2015/02/18 18:41:29, kelvinp wrote:
> s/{{pin: string, accessCode: string}}/{{pin:string}}

Done.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_identity.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_identity.js:71: * @param {Function} onError
On 2015/02/18 02:23:50, Jamie wrote:
> This one always has the same signature. We might as well be explicit about it.

Done.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_session_connector.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_session_connector.js:141: /** @type
{remoting.MockSessionConnector} */
On 2015/02/18 02:23:50, Jamie wrote:
> Do you need @type here? I know we used to with the old compiler, but I've seen
> new code since we upgraded that didn't specify the type of |that| (which
should
> be unnecessary).

Indeed. No longer necessary.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/update_pin_browser_test.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:22: * @param {{new_pin:
string, old_pin: string}} data
On 2015/02/18 18:41:29, kelvinp wrote:
> no space between : and string

Done.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_connected_view.js:196:
this.pluginWidthForBumpScrollTesting = width;
On 2015/02/18 18:41:29, kelvinp wrote:
> make this.pluginWidthForBumpScrollTesting_ private?

Done.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/js_proto...
File remoting/webapp/js_proto/test_proto.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/js_proto...
remoting/webapp/js_proto/test_proto.js:6: // WebKit- and Chrome-specific
properties and methods. It is used only with
On 2015/02/18 18:41:30, kelvinp wrote:
> s/WebKit- and Chrome-specific/test specific

Done.

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/unittest...
File remoting/webapp/unittests/chrome_mocks.js (right):

https://codereview.chromium.org/927373005/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/chrome_mocks.js:109: * @return {Array.<string>}
On 2015/02/18 18:41:30, kelvinp wrote:
> dbeam recently switch all Array.<string> into Array<string>
> https://codereview.chromium.org/917093003

Done.

[garykac, 2015-02-19 02:00:44]

The CQ bit was checked by garykac@chromium.org

[commit-bot: I haz the power, 2015-02-19 02:02:27]

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

[commit-bot: I haz the power, 2015-02-19 02:59:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-19 02:59:33]

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